Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1841353002:
Implement ShadowRoot.mode (Closed)
|
Created:
4 years, 8 months ago by yuzuchan Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement ShadowRoot.mode
This CL implements ShadowRoot.mode, which returns open or closed for V1, and open for V0 shadowroots.
See the spec here: https://dom.spec.whatwg.org/#interface-shadowroot
BUG=596770
Committed: https://crrev.com/e107da1b11c8418569fa52ee4e3d4d0feb60d027
Cr-Commit-Position: refs/heads/master@{#384528}
Patch Set 1 #Patch Set 2 : Modify test results #
Total comments: 4
Patch Set 3 : Add a testcase and V1 flag #Patch Set 4 : Rebase #
Total comments: 6
Patch Set 5 : Add a comment and refactor the layout test #
Total comments: 1
Messages
Total messages: 23 (8 generated)
Description was changed from ========== Clean up Implement ShadowRoot.mode This CL implements ShadowRoot.mode, which returns open or closed for V1, and open for V0 shadowroots. See the spec here: https://dom.spec.whatwg.org/#interface-shadowroot BUG=596770 ========== to ========== Implement ShadowRoot.mode This CL implements ShadowRoot.mode, which returns open or closed for V1, and open for V0 shadowroots. See the spec here: https://dom.spec.whatwg.org/#interface-shadowroot BUG=596770 ==========
The CQ bit was checked by yuzus@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1841353002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1841353002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
yuzus@chromium.org changed reviewers: + hayato@chromium.org, kochi@chromium.org
PTAL
https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.h (right): https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.h:82: String mode() const { return (type() == ShadowRootType::V0 || type() == ShadowRootType::Open) ? "open" : "closed"; }; If type() returns ShadowRootType::UserAgent, this returns "closed" ... is it okay? As user agent shadow root is not exposed to web, probably it doesn't break the web, but we should be clear about the expectation. I'd suggest you add another layout test, using window.shadowRoot(element) to get user-agent shadow root from e.g. <input> element. (see fast/dom/shadow/shadowroot-type.html for example.) https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.idl (right): https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.idl:30: readonly attribute ShadowRootMode mode; We should restrict exposing this attribute only when ShadowDOMV1 flag is enabled. Otherwise this accidentally exposes 'mode' attribute to V0 shadow, which breaks compatibility.
PTAL This is how I feel about this patch: http://i.imgur.com/cLeIcgZ.gif https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.h (right): https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.h:82: String mode() const { return (type() == ShadowRootType::V0 || type() == ShadowRootType::Open) ? "open" : "closed"; }; On 2016/03/31 at 06:19:15, kochi wrote: > If type() returns ShadowRootType::UserAgent, this returns "closed" ... is it okay? > As user agent shadow root is not exposed to web, probably it doesn't break the web, > but we should be clear about the expectation. > > I'd suggest you add another layout test, using window.shadowRoot(element) to get > user-agent shadow root from e.g. <input> element. > (see fast/dom/shadow/shadowroot-type.html for example.) Okay, I added a test case for UA shadow root :) https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/shadow/ShadowRoot.idl (right): https://codereview.chromium.org/1841353002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/shadow/ShadowRoot.idl:30: readonly attribute ShadowRootMode mode; On 2016/03/31 at 06:19:15, kochi wrote: > We should restrict exposing this attribute only when ShadowDOMV1 flag is enabled. > Otherwise this accidentally exposes 'mode' attribute to V0 shadow, which breaks compatibility. Done.
https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html (right): https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html:7: <div id = "host3"></div> nit: Please be consistent on whitespaces around "=". https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html:12: var host1 = document.getElementById('host1'); nit: it's okay to explicitly have this and following lines, but HTML parser once parsed an element with id which doesn't conflict in the "window.*" namespace and doesn't have invalid character as a JS identifier, the variable is automatically created. So without line 12-15 this test should run. Just FYI - I personally think depending on this behavior in tests isn't a good practice, but this technique is often used in short layout tests. https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html:21: assert_equals(host3.shadowRoot.mode, 'open'); Strictly speaking, mode for V0 shadow root and UA shadow root is not (and will not be) specified anywhere. Could you add a comment here like: // Note: mode for V0 shadow and UA shadow is not specified anywhere, // Blink returns reasonable default values for these.
Thanks for the review, PTAL again :) https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html (right): https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html:7: <div id = "host3"></div> On 2016/03/31 at 08:38:42, kochi wrote: > nit: Please be consistent on whitespaces around "=". Done. https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html:12: var host1 = document.getElementById('host1'); On 2016/03/31 at 08:38:42, kochi wrote: > nit: it's okay to explicitly have this and following lines, but HTML parser > once parsed an element with id which doesn't conflict in the "window.*" > namespace and doesn't have invalid character as a JS identifier, the variable > is automatically created. So without line 12-15 this test should run. > > Just FYI - I personally think depending on this behavior in tests isn't > a good practice, but this technique is often used in short layout tests. Done. https://codereview.chromium.org/1841353002/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html:21: assert_equals(host3.shadowRoot.mode, 'open'); On 2016/03/31 at 08:38:42, kochi wrote: > Strictly speaking, mode for V0 shadow root and UA shadow root > is not (and will not be) specified anywhere. > > Could you add a comment here like: > // Note: mode for V0 shadow and UA shadow is not specified anywhere, > // Blink returns reasonable default values for these. Done.
lgtm https://codereview.chromium.org/1841353002/diff/80001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html (right): https://codereview.chromium.org/1841353002/diff/80001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/shadowroot-mode.html:17: assert_equals(host3.shadowRoot.mode, 'open'); nit: moving this line (17) below the comment would be better.
lgtm
> This is how I feel about this patch: http://i.imgur.com/cLeIcgZ.gif Looks pretty good to me
The CQ bit was checked by yuzus@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1841353002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1841353002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_TIMED_OUT, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by yuzus@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1841353002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1841353002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Implement ShadowRoot.mode This CL implements ShadowRoot.mode, which returns open or closed for V1, and open for V0 shadowroots. See the spec here: https://dom.spec.whatwg.org/#interface-shadowroot BUG=596770 ========== to ========== Implement ShadowRoot.mode This CL implements ShadowRoot.mode, which returns open or closed for V1, and open for V0 shadowroots. See the spec here: https://dom.spec.whatwg.org/#interface-shadowroot BUG=596770 Committed: https://crrev.com/e107da1b11c8418569fa52ee4e3d4d0feb60d027 Cr-Commit-Position: refs/heads/master@{#384528} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/e107da1b11c8418569fa52ee4e3d4d0feb60d027 Cr-Commit-Position: refs/heads/master@{#384528} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
