Bug 60235

Summary: Make RenderStyle::setPageBreakInside() reject unsupported enum values.
Product: WebKit Reporter: Luke Macpherson <macpherson>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, dglazkov, eric, macpherson
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Luke Macpherson 2011-05-04 18:25:36 PDT
Make RenderStyle::setPageBreakInside() reject unsupported enum values.
Comment 1 Luke Macpherson 2011-05-04 18:29:40 PDT
Created attachment 92355 [details]
Patch
Comment 2 Luke Macpherson 2011-05-04 18:31:53 PDT
See http://www.w3.org/TR/CSS21/page.html#page-break-props for context.
Comment 3 Luke Macpherson 2011-05-04 23:10:26 PDT
Created attachment 92379 [details]
Patch
Comment 4 Luke Macpherson 2011-05-04 23:10:42 PDT
Added inline keyword.
Comment 5 Eric Seidel (no email) 2011-05-05 16:13:42 PDT
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!)
Comment 6 Luke Macpherson 2011-05-05 16:30:01 PDT
(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.
Comment 7 Luke Macpherson 2011-05-05 16:48:38 PDT
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 :)
Comment 8 Eric Seidel (no email) 2011-05-05 16:50:47 PDT
(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. :)
Comment 9 Luke Macpherson 2011-05-05 17:20:42 PDT
Created attachment 92509 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-05-05 17:49:11 PDT
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!
Comment 11 Luke Macpherson 2011-05-05 19:12:17 PDT
Created attachment 92529 [details]
Patch
Comment 12 WebKit Commit Bot 2011-05-06 01:28:05 PDT
Comment on attachment 92529 [details]
Patch

Clearing flags on attachment: 92529

Committed r85936: <http://trac.webkit.org/changeset/85936>
Comment 13 WebKit Commit Bot 2011-05-06 01:28:11 PDT
All reviewed patches have been landed.  Closing bug.