Make RenderStyle::setPageBreakInside() reject unsupported enum values.
Created attachment 92355 [details] Patch
See http://www.w3.org/TR/CSS21/page.html#page-break-props for context.
Created attachment 92379 [details] Patch
Added inline keyword.
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!)
(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.
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 :)
(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. :)
Created attachment 92509 [details] Patch
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!
Created attachment 92529 [details] Patch
Comment on attachment 92529 [details] Patch Clearing flags on attachment: 92529 Committed r85936: <http://trac.webkit.org/changeset/85936>
All reviewed patches have been landed. Closing bug.