Summary: | Implement CSSHostRule for @host @-rules. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Takashi Sakamoto <tasak> | ||||||||||||||||||||
Component: | CSS | Assignee: | 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
Takashi Sakamoto
2012-11-14 23:59:23 PST
Created attachment 179013 [details]
Patch
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 (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 (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. (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. (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 on attachment 179013 [details]
Patch
I have to use "1001" as @host @-rules' type. I will update the patch.
Created attachment 179240 [details]
Patch
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 on attachment 179240 [details] Patch Attachment 179240 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15323192 Comment on attachment 179240 [details] Patch Attachment 179240 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/15320180 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 on attachment 179240 [details] Patch Attachment 179240 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15309259 Comment on attachment 179240 [details] Patch Attachment 179240 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15331005 Created attachment 179393 [details]
Patch
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 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? *** Bug 105404 has been marked as a duplicate of this bug. *** Created attachment 181624 [details]
WIP
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.
Created attachment 181675 [details]
Patch
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. 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 on attachment 181675 [details] Patch Attachment 181675 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/15756105 Comment on attachment 181675 [details] Patch Attachment 181675 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/15762054 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. Created attachment 181676 [details]
Patch
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.
Created attachment 181679 [details]
Patch
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. 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 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.
Created attachment 182296 [details]
Patch
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 on attachment 182296 [details]
Patch
awesome.
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 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 Created attachment 183403 [details]
Patch
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 on attachment 183403 [details] Patch Clearing flags on attachment: 183403 Committed r140115: <http://trac.webkit.org/changeset/140115> All reviewed patches have been landed. Closing bug. |