Bug 60235 – Make RenderStyle::setPageBreakInside() reject unsupported enum values.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Luke Macpherson]

Make RenderStyle::setPageBreakInside() reject unsupported enum values.

[Luke Macpherson]

Created attachment 92355 [details]
Patch

[Luke Macpherson]

See http://www.w3.org/TR/CSS21/page.html#page-break-props for context.

[Luke Macpherson]

Created attachment 92379 [details]
Patch

[Luke Macpherson]

Added inline keyword.

[Eric Seidel (no email)]

Comment on attachment 92379 [details]
Patch

Still need to understand why PBAlways is not a valid value here. You mentioned in another bug that the enums were "too loose".  Is it possible to ever call this with PBALWAYS?  or should we ASSERT not?  In either case we should have a comment her to explain why this check is needed/wanted.

I don't think you actually need the inline.  The compiler implicitly adds inline to all declaration-inline definitions was my understanding.  (But I'm not a c++ expert!)

[Luke Macpherson]

(In reply to comment #5)
> (From update of attachment 92379 [details])
> Still need to understand why PBAlways is not a valid value here. You mentioned in another bug that the enums were "too loose".  Is it possible to ever call this with PBALWAYS?  or should we ASSERT not?  In either case we should have a comment her to explain why this check is needed/wanted.

If you look at the spec I linked earlier you'll see that this is not a valid value for this property. It is used for 
'page-break-after' and 'page-break-before' properties, but not 'page-break-inside'.

To answer the question of whether to ASSERT or not, I'll need to see what the parser does, as it is possible the parser accepts the full set of enum values. In that case a user could put page-break-inline: always to trigger the assertion.

> I don't think you actually need the inline.  The compiler implicitly adds inline to all declaration-inline definitions was my understanding.  (But I'm not a c++ expert!)

dglazkov seems pretty adamant about it on 60246, though I think more for expression of intent than because it will change the compiler behavior. I haven't been around long enough to take a stand on issues of accepted style, so I'm just trying to do what I'm told.

[Luke Macpherson]

I checked the parser code, and indeed it looks like the parser considers these values invalid, so they can be safely changed to assertions. Expect another patch shortly :)

[Eric Seidel (no email)]

(In reply to comment #6)
> > I don't think you actually need the inline.  The compiler implicitly adds inline to all declaration-inline definitions was my understanding.  (But I'm not a c++ expert!)
> 
> dglazkov seems pretty adamant about it on 60246, though I think more for expression of intent than because it will change the compiler behavior. I haven't been around long enough to take a stand on issues of accepted style, so I'm just trying to do what I'm told.

Thanks!  I'm certainly not trying to argue with dglazkov through you. :)  Just seeking to share what I know about compiler behavior. :)

[Luke Macpherson]

Created attachment 92509 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 92509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=92509&action=review

> Source/WebCore/rendering/style/RenderStyle.h:1016
> +    void setPageBreakInside(EPageBreak b) { ASSERT(b != PBALWAYS); noninherited_flags._page_break_inside = b; }

So now I ask myself: "self, how many other values for EPageBreak are there?  Why are we blacklisting a single one, instead of whitelisting the supported ones?"

I still think this deserves a comment.  something as simple as:
// EPageBreak supports more values than page-break-inside allows, make sure we're only ever setting valid values.
or something like that.

Thank you for leading this difficult cleanup task!

[Luke Macpherson]

Created attachment 92529 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 92529 [details]
Patch

Clearing flags on attachment: 92529

Committed r85936: <http://trac.webkit.org/changeset/85936>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.