Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1127)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 325663003: Remove scoped styles (retry) (Closed)

Created:
6 years, 6 months ago by kochi
Modified:
6 years, 6 months ago
Reviewers:
esprehn, tasak
CC:
blink-reviews, blink-reviews-html_chromium.org, arv+blink, webcomponents-bugzilla_chromium.org, blink-reviews-css, sof, eae+blinkwatch, ed+blinkwatch_opera.com, blink-reviews-dom_chromium.org, dglazkov+blink, apavlov+blink_chromium.org, Inactive, darktears, rune+blink, watchdog-blink-watchlist_google.com, rwlbuis, hayato
Visibility:
Public.

Description

Remove scoped styles (retry) Removes <style scoped>. This was discussed and approved with LGTMs in blink-dev: https://groups.google.com/a/chromium.org/d/msg/blink-dev/R1x18ZLS5qQ/Bjuh_cENhlQJ Chrome status dashboard entry: http://www.chromestatus.com/features/5374137958662144 The previous one failed to land due to stale generated file issues and browser test failures (3 times): https://codereview.chromium.org/310443002/ (original CL) http://crrev.com/175550 -> http://crrev.com/175552 (stale generated file issue) http://crrev.com/175555 -> http://crrev.com/175561 (stale generated file issue) http://crrev.com/175720 -> http://crrev.com/175754 (browser test failures) Browser test failure was due to HTMLStyleElement refcounting mismatch in ShadowRoot. This CL fixes the issue and adds a layout test fast/css/shadow-style-removed-out-of-document.html to catch the condition. Note: This is based on esprehn's original cl: https://codereview.chromium.org/214693002/ BUG=379096 TEST=pass all layout tests and do not break any existing tests Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175993

Patch Set 1 : patch from 310443002 #

Patch Set 2 : Add m_hasRegistered flag #

Patch Set 3 : Rebase. #

Patch Set 4 : Revert the previous fix, add a failing layout test. #

Patch Set 5 : Reorder inDocument() check so that register/unregister should balance. #

Total comments: 4

Patch Set 6 : update layouttest for addressing tasak's comments. #

Total comments: 4

Patch Set 7 : Fix layout test (and expectation) #

Unified diffs Side-by-side diffs Delta from patch set Stats (+50 lines, -2276 lines) Patch
A LayoutTests/fast/css/shadow-style-removed-out-of-document.html View 1 2 3 4 5 6 1 chunk +9 lines, -0 lines 0 comments Download
A + LayoutTests/fast/css/shadow-style-removed-out-of-document-expected.txt View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/basic-attribute.html View 1 chunk +0 lines, -122 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/basic-attribute-expected.txt View 1 chunk +0 lines, -62 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/changing-scoped-attribute-asan-crash.html View 1 chunk +0 lines, -46 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/changing-scoped-attribute-asan-crash-expected.txt View 1 chunk +0 lines, -1 line 0 comments Download
D LayoutTests/fast/css/style-scoped/registering.html View 1 chunk +0 lines, -142 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/registering-expected.txt View 1 chunk +0 lines, -59 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/registering-shadowroot.html View 1 chunk +0 lines, -70 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/registering-shadowroot-expected.txt View 1 chunk +0 lines, -31 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-attach.html View 1 chunk +0 lines, -70 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-attach-expected.txt View 1 chunk +0 lines, -27 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-basic.html View 1 chunk +0 lines, -88 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-basic-expected.txt View 1 chunk +0 lines, -44 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow.html View 1 chunk +0 lines, -304 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-change-scoped-in-shadow-expected.txt View 1 chunk +0 lines, -55 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-detach.html View 1 chunk +0 lines, -72 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-detach-expected.txt View 1 chunk +0 lines, -27 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-in-shadow.html View 1 chunk +0 lines, -41 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-in-shadow-expected.txt View 1 chunk +0 lines, -14 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-keyframes.html View 1 chunk +0 lines, -60 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-keyframes-expected.txt View 1 chunk +0 lines, -2 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-nested.html View 1 chunk +0 lines, -46 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-nested-expected.txt View 1 chunk +0 lines, -13 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-remove-scoped.html View 1 chunk +0 lines, -77 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-remove-scoped-expected.txt View 1 chunk +0 lines, -42 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-scoping-nodes-different-order.html View 1 chunk +0 lines, -42 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-scoping-nodes-different-order-expected.txt View 1 chunk +0 lines, -14 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-set-scoped.html View 1 chunk +0 lines, -77 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-set-scoped-expected.txt View 1 chunk +0 lines, -42 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-shadow-crash.html View 1 chunk +0 lines, -28 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-shadow-crash-expected.txt View 1 chunk +0 lines, -2 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-with-dom-operation.html View 1 chunk +0 lines, -52 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-with-dom-operation-expected.txt View 1 chunk +0 lines, -19 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-with-important-rule.html View 1 chunk +0 lines, -83 lines 0 comments Download
D LayoutTests/fast/css/style-scoped/style-scoped-with-important-rule-expected.txt View 1 chunk +0 lines, -21 lines 0 comments Download
D LayoutTests/fast/dom/shadow/content-pseudo-element-scoped.html View 1 chunk +0 lines, -27 lines 0 comments Download
D LayoutTests/fast/dom/shadow/content-pseudo-element-scoped-expected.html View 1 chunk +0 lines, -12 lines 0 comments Download
M LayoutTests/fast/dom/shadow/shadow-tree-styles-select-host.html View 1 chunk +0 lines, -22 lines 0 comments Download
M LayoutTests/fast/dom/shadow/shadow-tree-styles-select-host-expected.txt View 1 chunk +0 lines, -2 lines 0 comments Download
D LayoutTests/fast/dom/shadow/style-scoped-not-enabled.html View 1 chunk +0 lines, -34 lines 0 comments Download
D LayoutTests/fast/dom/shadow/style-scoped-not-enabled-expected.txt View 1 chunk +0 lines, -16 lines 0 comments Download
M Source/core/css/invalidation/StyleSheetInvalidationAnalysis.cpp View 2 chunks +12 lines, -17 lines 0 comments Download
M Source/core/css/resolver/ScopedStyleResolver.cpp View 1 chunk +4 lines, -11 lines 0 comments Download
M Source/core/css/resolver/ScopedStyleTree.cpp View 2 3 2 chunks +2 lines, -3 lines 0 comments Download
M Source/core/css/resolver/SharedStyleFinder.cpp View 2 chunks +0 lines, -4 lines 0 comments Download
M Source/core/css/resolver/StyleResolver.cpp View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/dom/ContextFeatures.h View 2 chunks +0 lines, -2 lines 0 comments Download
M Source/core/dom/ContextFeatures.cpp View 1 chunk +0 lines, -7 lines 0 comments Download
M Source/core/dom/Element.cpp View 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/dom/Node.h View 3 chunks +1 line, -11 lines 0 comments Download
M Source/core/dom/Node.cpp View 1 chunk +0 lines, -22 lines 0 comments Download
M Source/core/dom/StyleSheetScopingNodeList.cpp View 2 chunks +1 line, -3 lines 0 comments Download
M Source/core/dom/shadow/ShadowRoot.h View 2 3 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/dom/shadow/ShadowRoot.cpp View 1 chunk +1 line, -3 lines 0 comments Download
M Source/core/html/HTMLStyleElement.h View 2 3 2 chunks +0 lines, -26 lines 0 comments Download
M Source/core/html/HTMLStyleElement.cpp View 1 2 3 4 5 chunks +16 lines, -126 lines 0 comments Download
M Source/core/html/HTMLStyleElement.idl View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/testing/InternalSettings.h View 2 chunks +0 lines, -2 lines 0 comments Download
M Source/core/testing/InternalSettings.cpp View 3 chunks +0 lines, -7 lines 0 comments Download
M Source/core/testing/InternalSettings.idl View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/testing/Internals.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/testing/Internals.cpp View 1 2 1 chunk +0 lines, -9 lines 0 comments Download
M Source/core/testing/Internals.idl View 1 chunk +0 lines, -1 line 0 comments Download
M Source/platform/RuntimeEnabledFeatures.in View 1 chunk +0 lines, -1 line 0 comments Download
M Source/web/ContextFeaturesClientImpl.cpp View 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
kochi
Hi Takashi, Elliott, This is a fix from the failed previous style scoped removal CL: ...
6 years, 6 months ago (2014-06-09 13:27:50 UTC) #1
kochi
Hi Takashi, As we discussed offline, I reverted the previous fix and changed inDocument() check ...
6 years, 6 months ago (2014-06-10 07:59:09 UTC) #2
kochi
On 2014/06/10 07:59:09, Takayoshi Kochi wrote: > Hi Takashi, > > As we discussed offline, ...
6 years, 6 months ago (2014-06-10 08:09:50 UTC) #3
tasak
https://codereview.chromium.org/325663003/diff/160001/LayoutTests/fast/css/shadow-style-removed-out-of-document.html File LayoutTests/fast/css/shadow-style-removed-out-of-document.html (right): https://codereview.chromium.org/325663003/diff/160001/LayoutTests/fast/css/shadow-style-removed-out-of-document.html#newcode3 LayoutTests/fast/css/shadow-style-removed-out-of-document.html:3: <head> We don't need <html>, <head> and <body> if ...
6 years, 6 months ago (2014-06-10 08:51:26 UTC) #4
kochi
Addressed your comments in the layout test. PTAL. https://codereview.chromium.org/325663003/diff/160001/LayoutTests/fast/css/shadow-style-removed-out-of-document.html File LayoutTests/fast/css/shadow-style-removed-out-of-document.html (right): https://codereview.chromium.org/325663003/diff/160001/LayoutTests/fast/css/shadow-style-removed-out-of-document.html#newcode3 LayoutTests/fast/css/shadow-style-removed-out-of-document.html:3: <head> ...
6 years, 6 months ago (2014-06-10 09:09:05 UTC) #5
tasak
https://codereview.chromium.org/325663003/diff/180001/LayoutTests/fast/css/shadow-style-removed-out-of-document.html File LayoutTests/fast/css/shadow-style-removed-out-of-document.html (right): https://codereview.chromium.org/325663003/diff/180001/LayoutTests/fast/css/shadow-style-removed-out-of-document.html#newcode1 LayoutTests/fast/css/shadow-style-removed-out-of-document.html:1: <script src="../../resources/js-test.js"></script> I'm sorry. We still need <!DOCTYPE html>. ...
6 years, 6 months ago (2014-06-10 09:13:30 UTC) #6
kochi
Thanks for the review. I also updated the expectation, which I failed in the previous ...
6 years, 6 months ago (2014-06-10 09:30:31 UTC) #7
tasak
lgtm (if linux_chromium_chromeos_dbg passes).
6 years, 6 months ago (2014-06-10 10:19:16 UTC) #8
kochi
On 2014/06/10 10:19:16, tasak wrote: > lgtm (if linux_chromium_chromeos_dbg passes). Thanks Takashi, I'll make sure ...
6 years, 6 months ago (2014-06-11 01:28:29 UTC) #9
kochi
On 2014/06/11 01:28:29, Takayoshi Kochi wrote: > On 2014/06/10 10:19:16, tasak wrote: > > lgtm ...
6 years, 6 months ago (2014-06-11 05:41:08 UTC) #10
kochi
On 2014/06/11 05:41:08, Takayoshi Kochi wrote: > On 2014/06/11 01:28:29, Takayoshi Kochi wrote: > > ...
6 years, 6 months ago (2014-06-11 23:14:13 UTC) #11
esprehn
rslgtm, what did you need to change this time?
6 years, 6 months ago (2014-06-11 23:30:43 UTC) #12
kochi
On 2014/06/11 23:30:43, esprehn wrote: > rslgtm, what did you need to change this time? ...
6 years, 6 months ago (2014-06-12 01:40:59 UTC) #13
kochi
The CQ bit was checked by kochi@chromium.org
6 years, 6 months ago (2014-06-12 04:24:09 UTC) #14
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/kochi@chromium.org/325663003/200001
6 years, 6 months ago (2014-06-12 04:25:24 UTC) #15
esprehn
Can you add a layout test (in a separate patch is fine) that tests that ...
6 years, 6 months ago (2014-06-12 04:41:51 UTC) #16
kochi
On 2014/06/12 04:41:51, esprehn wrote: > Can you add a layout test (in a separate ...
6 years, 6 months ago (2014-06-12 04:44:42 UTC) #17
kochi
On 2014/06/12 04:44:42, Takayoshi Kochi wrote: > On 2014/06/12 04:41:51, esprehn wrote: > > Can ...
6 years, 6 months ago (2014-06-12 04:45:55 UTC) #18
commit-bot: I haz the power
6 years, 6 months ago (2014-06-12 05:45:20 UTC) #19
Message was sent while issue was closed. 
Change committed as 175993

Powered by Google App Engine
This is Rietveld 408576698