New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 721517 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Not on Chrome anymore
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocked on:
issue 728379

Blocking:
issue 733909



Sign in to add a comment

Remove style sharing

Project Member Reported by esprehn@chromium.org, May 11 2017

Issue description

Per discussion here:
https://groups.google.com/a/chromium.org/forum/#!topic/style-dev/8BV0QYubjos

This was my patch, but it seems to fail some telemetry unit test in a mysterious way:
https://codereview.chromium.org/2858423002


 

Comment 1 Deleted

Labels: -Pri-3 Pri-2
Status: Available (was: Untriaged)

Comment 3 by nainar@chromium.org, May 15 2017

Labels: Update-Quarterly

Comment 4 by nainar@chromium.org, May 31 2017

Cc: -nainar@chromium.org
Owner: nainar@chromium.org
Status: Started (was: Available)
Blockedon: 728379

Comment 6 by nainar@chromium.org, Jun 15 2017

hmm dont know why bugdroid didnt comment here - but I landed the CL to disable style sharing on canary a while back here: https://codereview.chromium.org/2887593002

waiting to see canary results for further action. 

Comment 7 by nainar@chromium.org, Jun 15 2017

hmm dont know why bugdroid didnt comment here - but I landed the CL to disable style sharing on canary a while back here: https://codereview.chromium.org/2887593002

waiting to see canary results for further action. 

Comment 8 by meade@chromium.org, Jun 16 2017

Blocking: 733909
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fd7f99c9125b7f9b054b3a32c0100b526792f74c

commit fd7f99c9125b7f9b054b3a32c0100b526792f74c
Author: Naina Raisinghani <nainar@chromium.org>
Date: Mon Jul 24 05:21:35 2017

Remove code and tests using StyleSharing

This patch follows on from https://codereview.chromium.org/2887593002
where we disabled style sharing in M61, but left the code and
left it enabled in the layout tests.

This patch removes the code that refers to StyleSharing and removes the
tests for it as well.

Per the style-dev@ discussion and design doc:
https://groups.google.com/a/chromium.org/forum/#!topic/style-dev/8BV0QYubjos

Please note that this is not a web visible change and an 
implementation detail. 

Bug:  721517 
Change-Id: I1c6251c72a7a5c481a855045226f37a0cde0f034
Reviewed-on: https://chromium-review.googlesource.com/572489
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: dstockwell <dstockwell@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Commit-Queue: nainar <nainar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#488915}
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/LayoutTests/FlagExpectations/enable-blink-features=LayoutNG
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/cousin-stylesharing-adjacent-selector-expected.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/cousin-stylesharing-adjacent-selector.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/cousin-stylesharing-last-child-selector-expected.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/cousin-stylesharing-last-child-selector.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-adjacent-selector-expected.txt
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-adjacent-selector.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-attribute-selector-expected.txt
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-attribute-selector-with-pseudo-element-expected.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-attribute-selector-with-pseudo-element.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-attribute-selector.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-children-prevent-sharing-expected.txt
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-children-prevent-sharing.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-last-child-expected.txt
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-last-child.html
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-shadow-expected.txt
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/LayoutTests/fast/selectors/style-sharing-shadow.html
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/BUILD.gn
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/css/BUILD.gn
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.cpp
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/Source/core/css/resolver/SharedStyleFinder.h
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/Source/core/css/resolver/SharedStyleFinderTest.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/css/resolver/StyleResolver.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/css/resolver/StyleResolver.h
[delete] https://crrev.com/af68cffd17ef07ce6756e4aacb7c88a2f877f155/third_party/WebKit/Source/core/css/resolver/StyleSharingDepthScope.h
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/ContainerNode.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/ContainerNode.h
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/Element.h
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/ShadowRoot.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/StyleEngine.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/dom/StyleEngine.h
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/inspector/InspectorTraceEvents.h
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/svg/SVGElement.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/core/svg/SVGElementRareData.cpp
[modify] https://crrev.com/fd7f99c9125b7f9b054b3a32c0100b526792f74c/third_party/WebKit/Source/platform/RuntimeEnabledFeatures.json5

Status: Fixed (was: Started)
Closing as Fixed. Will reopen if there are any unseen changes in circumstances. 
Project Member

Comment 11 by bugdroid1@chromium.org, Aug 1 2017

Project Member

Comment 12 by bugdroid1@chromium.org, Aug 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ba7f9f05a118ab6663c7879571c2be90bec80434

commit ba7f9f05a118ab6663c7879571c2be90bec80434
Author: Rune Lillesveen <rune@opera.com>
Date: Tue Aug 01 14:09:16 2017

Remove stats for style sharing.

Style sharing code is removed and these stats will always be 0.

Bug:  721517 
Change-Id: Iae76778bd564ad22645d3d14709c1d8d13d796c6
Reviewed-on: https://chromium-review.googlesource.com/595744
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#490984}
[modify] https://crrev.com/ba7f9f05a118ab6663c7879571c2be90bec80434/third_party/WebKit/Source/core/css/resolver/StyleResolverStats.cpp
[modify] https://crrev.com/ba7f9f05a118ab6663c7879571c2be90bec80434/third_party/WebKit/Source/core/css/resolver/StyleResolverStats.h

Project Member

Comment 13 by bugdroid1@chromium.org, Aug 2 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/82f666a5da8e2e3855782541a9534299f541ebfe

commit 82f666a5da8e2e3855782541a9534299f541ebfe
Author: Rune Lillesveen <rune@opera.com>
Date: Wed Aug 02 12:12:38 2017

Remove rulesets used for style sharing.

After style sharing code is removed, we no longer need the sibling and
uncommon attribute rulesets to reject style sharing. This also means
RuleFeatureSet no longer needs to be traced, so some oilpan cruft could
be removed.

The StyleEngine API UsesSiblingRules() relied on the size of the sibling
ruleset, but it was only used as what looked like an optimization for
:empty selector updates where :empty is found in non-rightmost compound
selectors. However, the presence of :empty itself would add a sibling
selector in the previous code, and :empty in non-rightmost compound not
followed by an adjacent selector would only have de-generate cases like
":empty span" which could never match anything, or ":not(:empty) span"
which would always be true if the whole selector matches. Therefore, it
makes sense to just drop the API/check.

Bug:  721517 
Change-Id: I85100850cb8cec56b17947916d7755ebcf3f15ec
Reviewed-on: https://chromium-review.googlesource.com/597689
Reviewed-by: nainar <nainar@chromium.org>
Reviewed-by: Morten Stenshorne <mstensho@opera.com>
Commit-Queue: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#491344}
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/CSSDefaultStyleSheets.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/CSSGlobalRuleSet.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/CSSGlobalRuleSet.h
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/ElementRuleCollector.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/RuleFeature.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/RuleFeature.h
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/RuleFeatureSetTest.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/RuleSet.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/css/SelectorChecker.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/dom/ElementShadowV0.cpp
[modify] https://crrev.com/82f666a5da8e2e3855782541a9534299f541ebfe/third_party/WebKit/Source/core/dom/StyleEngine.h

Project Member

Comment 14 by bugdroid1@chromium.org, Aug 23 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d551501dfc213215e401be4e154e292c16e524f7

commit d551501dfc213215e401be4e154e292c16e524f7
Author: Rune Lillesveen <rune@opera.com>
Date: Wed Aug 23 06:14:46 2017

Remove style sharing workaround from StyleInvalidator.

Style sharing is no longer present in the codebase, so the SetStyle /
LocalStyleChange should no longer be necessary here.

Bug:  721517 

Change-Id: I0437f3c3c3188101299b053d81b36a7542baf610
Reviewed-on: https://chromium-review.googlesource.com/625880
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Reviewed-by: nainar <nainar@chromium.org>
Commit-Queue: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#496597}
[modify] https://crrev.com/d551501dfc213215e401be4e154e292c16e524f7/third_party/WebKit/Source/core/css/invalidation/StyleInvalidator.cpp

Project Member

Comment 15 by bugdroid1@chromium.org, Aug 24 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39fa617dc1e58ccea21f2747f16ff653984bb59f

commit 39fa617dc1e58ccea21f2747f16ff653984bb59f
Author: Rune Lillesveen <rune@opera.com>
Date: Thu Aug 24 05:21:50 2017

Remove Internals.isStyleSharing().

Not in use and style sharing is removed.

Bug:  721517 
Change-Id: I9eb72320ba001102f4c29f22240cf9dbd3a4f460
Reviewed-on: https://chromium-review.googlesource.com/628520
Reviewed-by: nainar <nainar@chromium.org>
Reviewed-by: meade_UTC10 <meade@chromium.org>
Commit-Queue: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#496957}
[modify] https://crrev.com/39fa617dc1e58ccea21f2747f16ff653984bb59f/third_party/WebKit/Source/core/testing/Internals.cpp
[modify] https://crrev.com/39fa617dc1e58ccea21f2747f16ff653984bb59f/third_party/WebKit/Source/core/testing/Internals.h
[modify] https://crrev.com/39fa617dc1e58ccea21f2747f16ff653984bb59f/third_party/WebKit/Source/core/testing/Internals.idl

Project Member

Comment 16 by bugdroid1@chromium.org, Aug 30 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a87ff17992abb8eea93e035b7e355824f5ff74d9

commit a87ff17992abb8eea93e035b7e355824f5ff74d9
Author: Rune Lillesveen <rune@opera.com>
Date: Wed Aug 30 11:16:30 2017

No need for forced SetStyleInternal for unchanged ComputedStyle.

This was done to avoid use of out-dated ComputedStyle from the style
sharing cache.

Also, removed unnecessary DCHECKs for style sharing sanity and fixed a
comment.

Bug:  721517 
Change-Id: I3a4117328b7a62dac84ef3e81017702f6f40315b
Reviewed-on: https://chromium-review.googlesource.com/628519
Reviewed-by: nainar <nainar@chromium.org>
Reviewed-by: Eric Willigers <ericwilligers@chromium.org>
Commit-Queue: Rune Lillesveen <rune@opera.com>
Cr-Commit-Position: refs/heads/master@{#498417}
[modify] https://crrev.com/a87ff17992abb8eea93e035b7e355824f5ff74d9/third_party/WebKit/Source/core/dom/Element.cpp
[modify] https://crrev.com/a87ff17992abb8eea93e035b7e355824f5ff74d9/third_party/WebKit/Source/core/layout/LayoutObject.cpp
[modify] https://crrev.com/a87ff17992abb8eea93e035b7e355824f5ff74d9/third_party/WebKit/Source/core/style/ComputedStyle.h

Project Member

Comment 17 by bugdroid1@chromium.org, Aug 9

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/867cc42af9d29a9f15e3ebad344afc5fdc181b97

commit 867cc42af9d29a9f15e3ebad344afc5fdc181b97
Author: Rune Lillesveen <futhark@chromium.org>
Date: Thu Aug 09 11:10:06 2018

Remove unused ScopedStyleResolver::HaveSameStyles.

This method was used for style sharing which was removed for issue
721517. Removed unit tests for ScopedStyleResolver which only tested
this API.

Bug:  721517 
Change-Id: Idebecd6b6b66bbf6a229bcf35669e6c4c5bda055
Reviewed-on: https://chromium-review.googlesource.com/1169015
Reviewed-by: Anders Ruud <andruud@chromium.org>
Commit-Queue: Rune Lillesveen <futhark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#581861}
[modify] https://crrev.com/867cc42af9d29a9f15e3ebad344afc5fdc181b97/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/867cc42af9d29a9f15e3ebad344afc5fdc181b97/third_party/blink/renderer/core/css/resolver/scoped_style_resolver.cc
[modify] https://crrev.com/867cc42af9d29a9f15e3ebad344afc5fdc181b97/third_party/blink/renderer/core/css/resolver/scoped_style_resolver.h
[delete] https://crrev.com/68a491b72aabaa1bf694050e164759ab5b6cfe8e/third_party/blink/renderer/core/css/resolver/scoped_style_resolver_test.cc

Sign in to add a comment