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

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 2472973002: Link stylesheets in shadow trees do not belong to document scope. (Closed)

Created:
4 years, 1 month ago by rune
Modified:
4 years, 1 month ago
Reviewers:
hayato, kochi
CC:
chromium-reviews, blink-reviews-style_chromium.org, sof, eae+blinkwatch, blink-reviews-dom_chromium.org, dglazkov+blink, blink-reviews, rwlbuis
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Link stylesheets in shadow trees do not belong to document scope. We have incorrectly kept DCHECKs checking that stylesheets in shadow trees come from style elements. That is no longer true, and modifying link elements in shadow trees would trigger some of these DCHECKs. Also, we simply used Document as the TreeScope handling link elements. Always use the treeScope() from the associated node instead. Using the wrong TreeScope in these cases would cause missing updates of active stylesheets in ShadowTreeStyleSheetCollections for AnalyzedStyleUpdate. I have not been able to find a triggering test case for this. R=hayato@chromium.org,kochi@chromium.org BUG=661914 Committed: https://crrev.com/7187fe2ac3d98d0219268914aeddf4eb03c5f42b Cr-Commit-Position: refs/heads/master@{#429877}

Patch Set 1 #

Patch Set 2 : Rebased onto dependency #

Total comments: 2

Patch Set 3 : Moved test. #

Patch Set 4 : Rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -18 lines) Patch
A third_party/WebKit/LayoutTests/shadow-dom/crashes/link-style-change-href-assert.html View 1 2 1 chunk +17 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/core/dom/StyleEngine.cpp View 1 2 3 5 chunks +4 lines, -18 lines 0 comments Download

Messages

Total messages: 23 (11 generated)
rune
ptal
4 years, 1 month ago (2016-11-03 10:51:28 UTC) #3
kochi
https://codereview.chromium.org/2472973002/diff/20001/third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html File third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html (right): https://codereview.chromium.org/2472973002/diff/20001/third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html#newcode1 third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html:1: <!DOCTYPE html> Could you move this test to LayoutTests/shadow-dom/crash? ...
4 years, 1 month ago (2016-11-04 03:12:52 UTC) #6
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2472973002/20001
4 years, 1 month ago (2016-11-04 07:29:04 UTC) #8
commit-bot: I haz the power
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full ...
4 years, 1 month ago (2016-11-04 07:29:05 UTC) #10
rune
https://codereview.chromium.org/2472973002/diff/20001/third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html File third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html (right): https://codereview.chromium.org/2472973002/diff/20001/third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html#newcode1 third_party/WebKit/LayoutTests/shadow-dom/link-style-change-href-assert.html:1: <!DOCTYPE html> On 2016/11/04 03:12:51, kochi wrote: > Could ...
4 years, 1 month ago (2016-11-04 09:45:14 UTC) #12
kochi
lgtm
4 years, 1 month ago (2016-11-04 09:53:39 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2472973002/60001
4 years, 1 month ago (2016-11-04 09:56:43 UTC) #15
rune
On 2016/11/04 09:53:39, kochi wrote: > lgtm thanks for the prompt review!
4 years, 1 month ago (2016-11-04 09:56:44 UTC) #16
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/328964)
4 years, 1 month ago (2016-11-04 11:43:29 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2472973002/60001
4 years, 1 month ago (2016-11-04 12:38:05 UTC) #20
commit-bot: I haz the power
Committed patchset #4 (id:60001)
4 years, 1 month ago (2016-11-04 13:42:49 UTC) #21
commit-bot: I haz the power
4 years, 1 month ago (2016-11-04 13:47:04 UTC) #23
Message was sent while issue was closed. 
Patchset 4 (id:??) landed as
https://crrev.com/7187fe2ac3d98d0219268914aeddf4eb03c5f42b
Cr-Commit-Position: refs/heads/master@{#429877}

Powered by Google App Engine
This is Rietveld 408576698