Bug 89421

Summary: [CSSRegions]Change WEBKIT_REGION_RULE value to 16
Product: WebKit Reporter: Mihnea Ovidenie <mihnea>
Component: CSSAssignee: Mihnea Ovidenie <mihnea>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andreionea3000, cmarcelo, macpherson, menard, ojan, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
tony: review-, tony: commit-queue-
patch
none
patch
tony: review-, buildbot: commit-queue-
patch none

Description Mihnea Ovidenie 2012-06-18 22:26:05 PDT
Change WEBKIT_REGION_RULE from 10 to 16. See http://dev.w3.org/csswg/css3-regions/#region-style-rule-interface and http://wiki.csswg.org/spec/cssom-constants.
Comment 1 Andrei Onea 2012-06-19 07:01:46 PDT
Created attachment 148326 [details]
patch
Comment 2 Tony Chang 2012-06-19 10:04:32 PDT
Comment on attachment 148326 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148326&action=review

Just some minor nits to make sure we don't make mistakes in the future.

> Source/WebCore/css/StyleRule.h:48
> -        Region
> +        Region = 16

If it's important for these to be kept in sync with the values in CSSRule.h, you could use a COMPILE_ASSERT.  E.g., in CSSRule.cpp, COMPILE_ASSERT(StyleRule::Region == CSSRule::WEBKIT_REGION_RULE, enums_should_match);.

> Source/WebCore/css/StyleRule.h:87
> -    unsigned m_type : 4;
> -    signed m_sourceLine : 28;
> +    unsigned m_type : 5;
> +    signed m_sourceLine : 27;

Can you add a compile assert for the size in StyleRule.cpp? See CSSRule.cpp for an example.
Comment 3 Andrei Onea 2012-06-20 05:10:09 PDT
Thanks for your review! I'll resubmit the patch with your suggested modifications.
Comment 4 Andrei Onea 2012-06-20 07:44:04 PDT
Created attachment 148565 [details]
patch
Comment 5 WebKit Review Bot 2012-06-20 07:46:31 PDT
Attachment 148565 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:14:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:17:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:22:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:25:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 6 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Andrei Onea 2012-06-20 07:57:15 PDT
Created attachment 148567 [details]
patch
Comment 7 Build Bot 2012-06-20 08:53:31 PDT
Comment on attachment 148567 [details]
patch

Attachment 148567 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13002221
Comment 8 Gyuyoung Kim 2012-06-20 11:31:05 PDT
Comment on attachment 148567 [details]
patch

Attachment 148567 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13001259
Comment 9 Tony Chang 2012-06-20 11:33:28 PDT
Comment on attachment 148567 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148567&action=review

> Source/WebCore/css/CSSRule.cpp:49
> +COMPILE_ASSERT(StyleRuleBase::Region == static_cast<StyleRuleBase::Type>(CSSRule::WEBKIT_REGION_RULE), enums_should_match);

Looks like this needs to be in an #if ENABLE(CSS_REGIONS) block.
Comment 10 Andrei Onea 2012-06-21 01:11:51 PDT
Created attachment 148743 [details]
patch
Comment 11 WebKit Review Bot 2012-06-21 10:54:13 PDT
Comment on attachment 148743 [details]
patch

Clearing flags on attachment: 148743

Committed r120943: <http://trac.webkit.org/changeset/120943>
Comment 12 WebKit Review Bot 2012-06-21 10:54:29 PDT
All reviewed patches have been landed.  Closing bug.