Bug 102344 – Implement CSSHostRule for @host @-rules.

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Takashi Sakamoto]

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.

[Takashi Sakamoto]

Created attachment 179013 [details]
Patch

[Takashi Sakamoto]

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

[Glenn Adams]

(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

[Dimitri Glazkov (Google)]

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

[Glenn Adams]

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

[Dimitri Glazkov (Google)]

(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 :)

[Takashi Sakamoto]

Comment on attachment 179013 [details]
Patch

I have to use "1001" as @host @-rules' type. I will update the patch.

[Takashi Sakamoto]

Created attachment 179240 [details]
Patch

[Takashi Sakamoto]

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

[Early Warning System Bot]

Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15323192

[Build Bot]

Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15320180

[WebKit Review Bot]

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.

[Build Bot]

Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15309259

[Early Warning System Bot]

Comment on attachment 179240 [details]
Patch

Attachment 179240 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15331005

[Takashi Sakamoto]

Created attachment 179393 [details]
Patch

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

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?

[Takashi Sakamoto]

*** Bug 105404 has been marked as a duplicate of this bug. ***

[Takashi Sakamoto]

Created attachment 181624 [details]
WIP

[WebKit Review Bot]

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.

[Takashi Sakamoto]

Created attachment 181675 [details]
Patch

[Takashi Sakamoto]

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.

[WebKit Review Bot]

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.

[Early Warning System Bot]

Comment on attachment 181675 [details]
Patch

Attachment 181675 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15756105

[Early Warning System Bot]

Comment on attachment 181675 [details]
Patch

Attachment 181675 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15762054

[Chris Dumez]

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.

[Takashi Sakamoto]

Created attachment 181676 [details]
Patch

[WebKit Review Bot]

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.

[Takashi Sakamoto]

Created attachment 181679 [details]
Patch

[Takashi Sakamoto]

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.

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

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.

[Takashi Sakamoto]

Created attachment 182296 [details]
Patch

[WebKit Review Bot]

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.

[Dimitri Glazkov (Google)]

Comment on attachment 182296 [details]
Patch

awesome.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Takashi Sakamoto]

Created attachment 183403 [details]
Patch

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 183403 [details]
Patch

Clearing flags on attachment: 183403

Committed r140115: <http://trac.webkit.org/changeset/140115>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.