Bug 60249 - Make RenderStyle::setColumnBreakInside() reject unsupported enum values.
Summary: Make RenderStyle::setColumnBreakInside() reject unsupported enum values.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-04 22:51 PDT by Luke Macpherson
Modified: 2011-05-08 22:47 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.22 KB, patch)
2011-05-04 22:52 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (3.32 KB, patch)
2011-05-05 18:25 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (3.31 KB, patch)
2011-05-05 18:44 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (3.32 KB, patch)
2011-05-05 20:36 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff
Patch (3.33 KB, patch)
2011-05-08 16:37 PDT, Luke Macpherson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Luke Macpherson 2011-05-04 22:51:53 PDT
Make RenderStyle::setColumnBreakInside() reject unsupported enum values.
Comment 1 Luke Macpherson 2011-05-04 22:52:53 PDT
Created attachment 92375 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-05-04 22:55:42 PDT
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.
Comment 3 Luke Macpherson 2011-05-04 23:42:04 PDT
(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.
Comment 4 Eric Seidel (no email) 2011-05-04 23:48:21 PDT
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?
Comment 5 Eric Seidel (no email) 2011-05-04 23:49:06 PDT
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!
Comment 6 Luke Macpherson 2011-05-05 18:25:49 PDT
Created attachment 92520 [details]
Patch
Comment 7 Luke Macpherson 2011-05-05 18:44:23 PDT
Created attachment 92527 [details]
Patch
Comment 8 Luke Macpherson 2011-05-05 18:47:35 PDT
ASSERT and comment added.
Comment 9 Eric Seidel (no email) 2011-05-05 19:06:43 PDT
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?
Comment 10 Luke Macpherson 2011-05-05 20:16:42 PDT
(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.
Comment 11 Luke Macpherson 2011-05-05 20:36:18 PDT
Created attachment 92534 [details]
Patch
Comment 12 Luke Macpherson 2011-05-05 20:37:48 PDT
(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 13 Eric Seidel (no email) 2011-05-05 22:24:07 PDT
Comment on attachment 92534 [details]
Patch

Thanks.
Comment 14 WebKit Commit Bot 2011-05-06 00:32:10 PDT
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
Comment 15 Luke Macpherson 2011-05-08 16:37:05 PDT
Created attachment 92751 [details]
Patch
Comment 16 Luke Macpherson 2011-05-08 16:37:56 PDT
Fixed ChangeLog.
Comment 17 WebKit Commit Bot 2011-05-08 19:15:31 PDT
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 18 WebKit Commit Bot 2011-05-08 19:16:52 PDT
Comment on attachment 92751 [details]
Patch

Clearing flags on attachment: 92751

Committed r86040: <http://trac.webkit.org/changeset/86040>
Comment 19 WebKit Commit Bot 2011-05-08 19:16:57 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 WebKit Review Bot 2011-05-08 22:47:26 PDT
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