Bug 60249 – Make RenderStyle::setColumnBreakInside() reject unsupported enum values.

• Component: New Bugs
• Status: RESOLVED FIXED

[Luke Macpherson]

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

[Luke Macpherson]

Created attachment 92375 [details]
Patch

[Eric Seidel (no email)]

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.

[Luke Macpherson]

(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.

[Eric Seidel (no email)]

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?

[Eric Seidel (no email)]

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!

[Luke Macpherson]

Created attachment 92520 [details]
Patch

[Luke Macpherson]

Created attachment 92527 [details]
Patch

[Luke Macpherson]

ASSERT and comment added.

[Eric Seidel (no email)]

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?

[Luke Macpherson]

(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.

[Luke Macpherson]

Created attachment 92534 [details]
Patch

[Luke Macpherson]

(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.

[Eric Seidel (no email)]

Comment on attachment 92534 [details]
Patch

Thanks.

[WebKit Commit Bot]

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

[Luke Macpherson]

Created attachment 92751 [details]
Patch

[Luke Macpherson]

Fixed ChangeLog.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 92751 [details]:

http/tests/xmlhttprequest/cross-origin-no-authorization.html bug 33357 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 92751 [details]
Patch

Clearing flags on attachment: 92751

Committed r86040: <http://trac.webkit.org/changeset/86040>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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