Created attachment 92375 [details] Patch

Comment on attachment 92375 [details] Patch I'm confused why we'd want this change. It seems render style is a dumb storage class and CSSStyleSelector shoudl be where all the mapping logic is.

(In reply to comment #2) > (From update of attachment 92375 [details]) > I'm confused why we'd want this change. It seems render style is a dumb storage class and CSSStyleSelector shoudl be where all the mapping logic is. See: http://www.w3.org/TR/css3-multicol/#break-before-break-after-break-inside It boils down to "always" is never a valid thing to set for this property. It is only in the enum because the enum is being shared with break-before and break-after. The reason I want to remove it from CSSStyleSelector is that I am trying to make that code as homogeneous as possible so that we can have as much code sharing as possible when the property handling is moved into CSSStyleApplyProperty.

So can we ever be passed in the "always" value? For properties which have valid values, but they are invalid for specific elements we fix up the values in CSSStyleSelector::adjustRenderStyle. A good example is overflow "scroll" and "auto" in SVG content. Totally valid values (which might be set accidentally by CSS), but they don't have meanings in SVG so they get mapped to other values. Is that the case here?

If it's never possible to intentionally pass in "always" here, then we should be ASSERTing. :) And certainly commenting to explain what we're doing (as I think I noted in the other bug). Thanks again for the cleanup work!

Created attachment 92520 [details] Patch

Created attachment 92527 [details] Patch

ASSERT and comment added.

Comment on attachment 92527 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=92527&action=review > Source/WebCore/rendering/style/RenderStyle.h:1077 > + void setColumnBreakInside(EPageBreak p) { ASSERT(p == PBALWAYS || p == PBAVOID); SET_VAR(rareNonInheritedData.access()->m_multiCol, m_breakInside, p); } I thought PBALWAS was not allowed?

(In reply to comment #9) > (From update of attachment 92527 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=92527&action=review > > > Source/WebCore/rendering/style/RenderStyle.h:1077 > > + void setColumnBreakInside(EPageBreak p) { ASSERT(p == PBALWAYS || p == PBAVOID); SET_VAR(rareNonInheritedData.access()->m_multiCol, m_breakInside, p); } > > I thought PBALWAS was not allowed? Yeah, that's why I removed the r? until I had a chance to fix.

Created attachment 92534 [details] Patch

(In reply to comment #11) > Created an attachment (id=92534) [details] > Patch (In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 92527 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=92527&action=review > > > > > Source/WebCore/rendering/style/RenderStyle.h:1077 > > > + void setColumnBreakInside(EPageBreak p) { ASSERT(p == PBALWAYS || p == PBAVOID); SET_VAR(rareNonInheritedData.access()->m_multiCol, m_breakInside, p); } > > > > I thought PBALWAS was not allowed? > > Yeah, that's why I removed the r? until I had a chance to fix. Fixed.

Comment on attachment 92534 [details] Patch Thanks.

Comment on attachment 92534 [details] Patch Rejecting attachment 92534 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'land-a..." exit_code: 2 Last 500 characters of output: umpRenderTree/gtk/LayoutTestControllerGtk.cpp M Tools/DumpRenderTree/wx/LayoutTestControllerWx.cpp M Tools/DumpRenderTree/mac/LayoutTestControllerMac.mm M Tools/DumpRenderTree/LayoutTestController.cpp M Tools/DumpRenderTree/win/LayoutTestControllerWin.cpp M Tools/DumpRenderTree/LayoutTestController.h M Tools/ChangeLog r85925 = be02bf169625268c108523a7e9db4f1a365608d6 (refs/remotes/trunk) First, rewinding head to replay your work on top of it... Fast-forwarded master to refs/remotes/trunk. Full output: http://queues.webkit.org/results/8571716

Created attachment 92751 [details] Patch

Fixed ChangeLog.

The commit-queue encountered the following flaky tests while processing attachment 92751 [details]: http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: ap@webkit.org) The commit-queue is continuing to process your patch.

Comment on attachment 92751 [details] Patch Clearing flags on attachment: 92751 Committed r86040: <http://trac.webkit.org/changeset/86040>

All reviewed patches have been landed. Closing bug.

http://trac.webkit.org/changeset/86040 might have broken GTK Linux 32-bit Debug The following tests are not passing: svg/W3C-SVG-1.1/animate-elem-82-t.svg