Bug 102344

Summary: Implement CSSHostRule for @host @-rules.
Product: WebKit Reporter: Takashi Sakamoto <tasak>
Component: CSSAssignee: Takashi Sakamoto <tasak>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, allan.jensen, cmarcelo, dglazkov, eric.carlson, feature-media-reviews, glenn, gyuyoung.kim, haraken, japhet, macpherson, menard, morrita, ojan.autocc, rakuco, sgrekhov, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 102116, 106418    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
WIP
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Takashi Sakamoto 2012-11-14 23:59:23 PST
We need CSSHostRule to enable WebInsepector to show @host @-rules instead of "undefined".
We also need to discuss whether we need CSSHostRule.idl or not. However this depends on Shadow DOM spec. So in this bug, just implement CSSHostRule without any IDL files.
Comment 1 Takashi Sakamoto 2012-12-12 03:19:27 PST
Created attachment 179013 [details]
Patch
Comment 2 Takashi Sakamoto 2012-12-12 03:22:11 PST
Shadow DOM spec, http://dvcs.w3.org/hg/webcomponents/raw-file/tip/spec/shadow/index.html#css-host-rule-interface, says:

partial interface CSSRule {
    const unsigned short HOST_RULE = 1001;
};

However, only 5 bits are available for representing a style type in class StyleRuleBase. So I used "31" for HOST_RULE in my patch.

Best regards,
Takashi Sakamoto
Comment 3 Glenn Adams 2012-12-12 03:39:17 PST
(In reply to comment #2)
> partial interface CSSRule {
>     const unsigned short HOST_RULE = 1001;
> };
> 
> However, only 5 bits are available for representing a style type in class StyleRuleBase. So I used "31" for HOST_RULE in my patch.

Perhaps you can suggest to dimitri to change the spec to use a value from the standards based range in [1].

[1] http://wiki.csswg.org/spec/cssom-constants
Comment 4 Dimitri Glazkov (Google) 2012-12-12 14:03:01 PST
(In reply to comment #3)
> (In reply to comment #2)
> > partial interface CSSRule {
> >     const unsigned short HOST_RULE = 1001;
> > };
> > 
> > However, only 5 bits are available for representing a style type in class StyleRuleBase. So I used "31" for HOST_RULE in my patch.
> 
> Perhaps you can suggest to dimitri to change the spec to use a value from the standards based range in [1].
> 
> [1] http://wiki.csswg.org/spec/cssom-constants

I can't do that, since this constant hasn't been reviewed and approved by CSS WG group yet.
Comment 5 Glenn Adams 2012-12-12 14:29:40 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > partial interface CSSRule {
> > >     const unsigned short HOST_RULE = 1001;
> > > };
> > > 
> > > However, only 5 bits are available for representing a style type in class StyleRuleBase. So I used "31" for HOST_RULE in my patch.
> > 
> > Perhaps you can suggest to dimitri to change the spec to use a value from the standards based range in [1].
> > 
> > [1] http://wiki.csswg.org/spec/cssom-constants
> 
> I can't do that, since this constant hasn't been reviewed and approved by CSS WG group yet.

Do you plan to do request that review/approval? I would recommend you email the list, requesting an assigned value. Tab can help you if needed.
Comment 6 Dimitri Glazkov (Google) 2012-12-12 16:26:55 PST
(In reply to comment #5)
 
> Do you plan to do request that review/approval? I would recommend you email the list, requesting an assigned value. Tab can help you if needed.

Yup. It's in flight :)
Comment 7 Takashi Sakamoto 2012-12-12 20:40:56 PST
Comment on attachment 179013 [details]
Patch

I have to use "1001" as @host @-rules' type. I will update the patch.
Comment 8 Takashi Sakamoto 2012-12-13 02:57:16 PST
Created attachment 179240 [details]
Patch
Comment 9 Takashi Sakamoto 2012-12-13 03:00:10 PST
I added class StyleRareRuleBlock and changed StyleHostRule to inherit the StyleRareRuleBlock.
I think, number of @host @-rules is much smaller than number of style rules. It would be ok to add one more "unsigned" member variable to StyleHostRule via StyleRareRuleBlock.

Best regards,
Takashi Sakamoto
Comment 10 Early Warning System Bot 2012-12-13 03:04:52 PST
Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15323192
Comment 11 Build Bot 2012-12-13 03:38:15 PST
Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15320180
Comment 12 WebKit Review Bot 2012-12-13 04:04:01 PST
Attachment 179240 [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
Source/WebCore/css/CSSRule.h:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 13 Build Bot 2012-12-13 05:01:16 PST
Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15309259
Comment 14 Early Warning System Bot 2012-12-13 06:59:10 PST
Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15331005
Comment 15 Takashi Sakamoto 2012-12-13 18:14:58 PST
Created attachment 179393 [details]
Patch
Comment 16 WebKit Review Bot 2012-12-13 18:20:06 PST
Attachment 179393 [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
Source/WebCore/css/CSSRule.h:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 17 Dimitri Glazkov (Google) 2012-12-13 20:47:38 PST
Comment on attachment 179393 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        * CMakeLists.txt:

Would love to have a bit more detail on these.

> Source/WebCore/css/CSSHostRule.cpp:24
> +#include "CSSHostRule.h"

This whole file seems like a lot of boilerplate that's probably very similar to @region stuff and other similar at-rules handling. I sense a great code health improvement opportunity! :)

> Source/WebCore/css/StyleRule.h:251
> +class StyleRareRuleBlock : public StyleRuleBlock {

The use of a Rare pattern here seems like a bit of an overkill. We will only have these temporarily and probably won't have many at once. Tab and I should have @host become 17 shortly.

Can we just have the id be 17 internally and 1001 externally?
Comment 18 Takashi Sakamoto 2012-12-19 03:07:26 PST
*** Bug 105404 has been marked as a duplicate of this bug. ***
Comment 19 Takashi Sakamoto 2013-01-07 19:44:25 PST
Created attachment 181624 [details]
WIP
Comment 20 WebKit Review Bot 2013-01-07 19:48:55 PST
Attachment 181624 [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
Source/WebCore/css/CSSRule.h:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 26 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 21 Takashi Sakamoto 2013-01-08 02:30:50 PST
Created attachment 181675 [details]
Patch
Comment 22 Takashi Sakamoto 2013-01-08 02:34:10 PST
Comment on attachment 179393 [details]
Patch

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

Thank you for reviewing.

Best regards,
Takashi Sakamoto

>> Source/WebCore/ChangeLog:15
>> +        * CMakeLists.txt:
> 
> Would love to have a bit more detail on these.

I see. I tried to add comments.

>> Source/WebCore/css/CSSHostRule.cpp:24
>> +#include "CSSHostRule.h"
> 
> This whole file seems like a lot of boilerplate that's probably very similar to @region stuff and other similar at-rules handling. I sense a great code health improvement opportunity! :)

I agree that this code is almost the same as WebKitCSSRegionRule and CSSMediaRule. ... firstly I was thinking that it would be better to file a new bug for refactoring those classes. However I added CSSBlockRule to avoid just "copy and paste".

>> Source/WebCore/css/StyleRule.h:251
>> +class StyleRareRuleBlock : public StyleRuleBlock {
> 
> The use of a Rare pattern here seems like a bit of an overkill. We will only have these temporarily and probably won't have many at once. Tab and I should have @host become 17 shortly.
> 
> Can we just have the id be 17 internally and 1001 externally?

I see. Done.
Comment 23 WebKit Review Bot 2013-01-08 02:35:56 PST
Attachment 181675 [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
Source/WebCore/css/CSSRule.h:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Early Warning System Bot 2013-01-08 02:38:01 PST
Comment on attachment 181675 [details]
Patch

Attachment 181675 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15756105
Comment 25 Early Warning System Bot 2013-01-08 02:40:24 PST
Comment on attachment 181675 [details]
Patch

Attachment 181675 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15762054
Comment 26 Chris Dumez 2013-01-08 02:42:33 PST
Comment on attachment 181675 [details]
Patch

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

> Source/WebCore/css/CSSBlockRule.h:140
> +        result.append("  ");

nit: We could use StringBuilder::appendLiteral() here.

> Source/WebCore/css/CSSHostRule.cpp:47
> +    result.append("@host { \n");

Ditto.
Comment 27 Takashi Sakamoto 2013-01-08 02:43:15 PST
Created attachment 181676 [details]
Patch
Comment 28 WebKit Review Bot 2013-01-08 02:48:36 PST
Attachment 181676 [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
Source/WebCore/css/CSSRule.h:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 29 Takashi Sakamoto 2013-01-08 02:53:00 PST
Created attachment 181679 [details]
Patch
Comment 30 Takashi Sakamoto 2013-01-08 02:54:35 PST
Comment on attachment 181675 [details]
Patch

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

>> Source/WebCore/css/CSSBlockRule.h:140
>> +        result.append("  ");
> 
> nit: We could use StringBuilder::appendLiteral() here.

Thanks. Done.

>> Source/WebCore/css/CSSHostRule.cpp:47
>> +    result.append("@host { \n");
> 
> Ditto.

Done.
Comment 31 WebKit Review Bot 2013-01-08 03:00:15 PST
Attachment 181679 [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
Source/WebCore/css/CSSRule.h:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 32 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 32 Dimitri Glazkov (Google) 2013-01-08 09:13:51 PST
Comment on attachment 181679 [details]
Patch

This is good progress! Let's now split this patch up into two chunks:

1) Refactoring to add CSSBlockRule
2) Adding CSSHostRule.
Comment 33 Takashi Sakamoto 2013-01-11 01:35:44 PST
Created attachment 182296 [details]
Patch
Comment 34 WebKit Review Bot 2013-01-11 01:47:11 PST
Attachment 182296 [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
Source/WebCore/css/CSSRule.h:57:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 35 Dimitri Glazkov (Google) 2013-01-11 09:29:47 PST
Comment on attachment 182296 [details]
Patch

awesome.
Comment 36 WebKit Review Bot 2013-01-16 01:03:13 PST
Comment on attachment 182296 [details]
Patch

Rejecting attachment 182296 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 167605 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
52>At revision 167605.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/15904355
Comment 37 WebKit Review Bot 2013-01-16 10:19:10 PST
Comment on attachment 182296 [details]
Patch

Rejecting attachment 182296 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
re/css/StyleRule.h.rej
patching file Source/WebCore/css/StyleSheetContents.cpp
patching file Source/WebCore/page/DOMWindow.idl
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file LayoutTests/fast/dom/shadow/css-hostrule-api-expected.txt
patching file LayoutTests/fast/dom/shadow/css-hostrule-api.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force', '--reviewer', 'Dimitri Glazk..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/15905511
Comment 38 Takashi Sakamoto 2013-01-18 01:51:52 PST
Created attachment 183403 [details]
Patch
Comment 39 WebKit Review Bot 2013-01-18 01:56:35 PST
Attachment 183403 [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
Source/WebCore/css/CSSRule.h:60:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Source/WebCore/css/CSSRule.h:63:  enum members should use InterCaps with an initial capital letter.  [readability/enum_casing] [4]
Total errors found: 2 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 WebKit Review Bot 2013-01-18 02:29:37 PST
Comment on attachment 183403 [details]
Patch

Clearing flags on attachment: 183403

Committed r140115: <http://trac.webkit.org/changeset/140115>
Comment 41 WebKit Review Bot 2013-01-18 02:29:45 PST
All reviewed patches have been landed.  Closing bug.