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

Takashi Sakamoto
Reported 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.
Attachments
Patch (38.37 KB, patch)
2012-12-12 03:19 PST, Takashi Sakamoto
no flags
Patch (42.59 KB, patch)
2012-12-13 02:57 PST, Takashi Sakamoto
no flags
Patch (42.62 KB, patch)
2012-12-13 18:14 PST, Takashi Sakamoto
no flags
WIP (41.07 KB, patch)
2013-01-07 19:44 PST, Takashi Sakamoto
no flags
Patch (60.03 KB, patch)
2013-01-08 02:30 PST, Takashi Sakamoto
no flags
Patch (60.07 KB, patch)
2013-01-08 02:43 PST, Takashi Sakamoto
no flags
Patch (60.23 KB, patch)
2013-01-08 02:53 PST, Takashi Sakamoto
no flags
Patch (37.05 KB, patch)
2013-01-11 01:35 PST, Takashi Sakamoto
no flags
Patch (36.96 KB, patch)
2013-01-18 01:51 PST, Takashi Sakamoto
no flags
Takashi Sakamoto
Comment 1 2012-12-12 03:19:27 PST
Takashi Sakamoto
Comment 2 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
Glenn Adams
Comment 3 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
Dimitri Glazkov (Google)
Comment 4 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.
Glenn Adams
Comment 5 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.
Dimitri Glazkov (Google)
Comment 6 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 :)
Takashi Sakamoto
Comment 7 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.
Takashi Sakamoto
Comment 8 2012-12-13 02:57:16 PST
Takashi Sakamoto
Comment 9 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
Early Warning System Bot
Comment 10 2012-12-13 03:04:52 PST
Build Bot
Comment 11 2012-12-13 03:38:15 PST
WebKit Review Bot
Comment 12 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.
Build Bot
Comment 13 2012-12-13 05:01:16 PST
Early Warning System Bot
Comment 14 2012-12-13 06:59:10 PST
Takashi Sakamoto
Comment 15 2012-12-13 18:14:58 PST
WebKit Review Bot
Comment 16 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.
Dimitri Glazkov (Google)
Comment 17 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?
Takashi Sakamoto
Comment 18 2012-12-19 03:07:26 PST
*** Bug 105404 has been marked as a duplicate of this bug. ***
Takashi Sakamoto
Comment 19 2013-01-07 19:44:25 PST
WebKit Review Bot
Comment 20 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.
Takashi Sakamoto
Comment 21 2013-01-08 02:30:50 PST
Takashi Sakamoto
Comment 22 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.
WebKit Review Bot
Comment 23 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.
Early Warning System Bot
Comment 24 2013-01-08 02:38:01 PST
Early Warning System Bot
Comment 25 2013-01-08 02:40:24 PST
Chris Dumez
Comment 26 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.
Takashi Sakamoto
Comment 27 2013-01-08 02:43:15 PST
WebKit Review Bot
Comment 28 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.
Takashi Sakamoto
Comment 29 2013-01-08 02:53:00 PST
Takashi Sakamoto
Comment 30 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.
WebKit Review Bot
Comment 31 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.
Dimitri Glazkov (Google)
Comment 32 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.
Takashi Sakamoto
Comment 33 2013-01-11 01:35:44 PST
WebKit Review Bot
Comment 34 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.
Dimitri Glazkov (Google)
Comment 35 2013-01-11 09:29:47 PST
Comment on attachment 182296 [details] Patch awesome.
WebKit Review Bot
Comment 36 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
WebKit Review Bot
Comment 37 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
Takashi Sakamoto
Comment 38 2013-01-18 01:51:52 PST
WebKit Review Bot
Comment 39 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.
WebKit Review Bot
Comment 40 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>
WebKit Review Bot
Comment 41 2013-01-18 02:29:45 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.