Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2846513002:
More targeted resource-switching mechanism for SVG selection painting (Closed)
|
Created:
3 years, 8 months ago by fs Modified:
3 years, 7 months ago CC:
fs, blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, krit, eae+blinkwatch, fmalita+watch_chromium.org, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMore targeted resource-switching mechanism for SVG selection painting
The mechanism by which resources are generated for painting using
selection style for SVG text is a bit too heavy-handed, and can end up
invalidating both layout and other things. All that is needed is looking
up any <paint> ('fill' or 'stroke') references and invalidating any
state from the non-selection style.
Use a reduced/tailored version of SVGResourcesCache::ClientStyleChanged
that only recreates/swaps the SVGResources object for the LayoutObject
and wrap that mechanism in a scope object.
BUG=713024
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2846513002
Cr-Commit-Position: refs/heads/master@{#468658}
Committed: https://chromium.googlesource.com/chromium/src/+/9eba37a11e54d4f0808f43db1371dc2e062efb8d
Patch Set 1 #Patch Set 2 : Less hammer #Patch Set 3 : Rebased onto dependent patch(es) #
Total comments: 12
Patch Set 4 : testharness test #Patch Set 5 : Use rALAP instead of first rAF #
Messages
Total messages: 41 (27 generated)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Description was changed from ========== Propagate the 'needs layout' flag when invalidating resource children Pass the |needs_layout| flag argument through to RemoveAllClientsFromCache to get an invalidation matching what was requested by the caller of MarkForLayoutAndParentResourceInvalidation. This prevents marking the layout tree for layout while temporarily switching out style-derived information while painting text using a selection pseudo element style. BUG=713024 ========== to ========== Propagate the 'needs layout' flag when invalidating resource children Pass the |needs_layout| flag argument through to RemoveAllClientsFromCache to get an invalidation matching what was requested by the caller of MarkForLayoutAndParentResourceInvalidation. This prevents marking the layout tree for layout while temporarily switching out style-derived information while painting text using a selection pseudo element style. BUG=713024 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from
==========
Propagate the 'needs layout' flag when invalidating resource children
Pass the |needs_layout| flag argument through to
RemoveAllClientsFromCache to get an invalidation matching what was
requested by the caller of MarkForLayoutAndParentResourceInvalidation.
This prevents marking the layout tree for layout while temporarily
switching out style-derived information while painting text using a
selection pseudo element style.
BUG=713024
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
More targeted resource-switching mechanism for SVG selection painting
The mechanism by which resources are generated for painting using
selection style for SVG text is a bit too heavy-handed, and can end up
invalidating both layout and other things. All that is needed is looking
up any <paint> ('fill' or 'stroke') references and invalidating any
state from the non-selection style.
Use a reduced/tailored version of SVGResourcesCache::ClientStyleChanged
that only recreates/swaps the SVGResources object for the LayoutObject
and wrap that mechanism in a scope object.
BUG=713024
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fs@opera.com changed reviewers: + chrishtr@chromium.org, schenney@chromium.org
So this CL is at the end of a queue of patches to fix the referenced bug. This fix alone uncovered an additional DCHECK-consistency check failing (see PS2 testresults), fixed by https://codereview.chromium.org/2847133002, and to make that fix not-too-hacky looking I refactored the code a bit in https://codereview.chromium.org/2851753002. Review in any order you like =).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Android failure seems unrelated; I don't think you have regressed memory in any significant way. I have a minor issue with the test because I like it to be clear exactly which conditions are needed to hit the crash. https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash.html (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash.html:13: document.body.offsetTop; This might be to force layout. Can it be switched to runAfterLayoutAndPaint and still crash?
One more query. https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/SVGResourcesCache.cpp (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/SVGResourcesCache.cpp:206: cache.AddResourcesFromLayoutObject(&layout_object_, style); A non-trivial chunk of invalidation logic is gone now. I presume that's a good things because we do not want any invalidation for this style change. Am I right?
https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash.html (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash.html:13: document.body.offsetTop; On 2017/04/28 at 16:12:31, Stephen Chennney wrote: > This might be to force layout. Can it be switched to runAfterLayoutAndPaint and still crash? Yes, we want layout to be clean before adding the filter (and selection) in the following line(s), then the rAF change will trigger style recalc (for the document) and subsequent paint. rALAP might work too since it should provide a layout, but I didn't not attempt to use it there. https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/layout/svg/SVGResourcesCache.cpp (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/layout/svg/SVGResourcesCache.cpp:206: cache.AddResourcesFromLayoutObject(&layout_object_, style); On 2017/04/28 at 16:16:40, Stephen Chennney wrote: > A non-trivial chunk of invalidation logic is gone now. I presume that's a good things because we do not want any invalidation for this style change. Am I right? That is correct. Optimally we'd like to just produce fill/stroke (as needed) from the selection style and use that - without any side-effects. As things are right now, we rely on the 1:1 mapping LayoutObject<->SVGResources, and this is submerged in the "fetch <paint>" code (i.e goes through the HashMap), so we need to swap out the SVGResources. (Unfortunately this swaps out more than we'd want, hence the dependent CLs.) The invalidation bit on the other hand does us no good (since we're not actually mutating the element/LayoutObject.)
https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt:1: PASS if no crash Use testharness.js to avoid the expectation file. Example in https://codereview.chromium.org/2844593002 https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:525: SVGResourcesCache::TemporaryStyleScope scope(ParentInlineLayoutObject(), How about instead teaching the SVG painting code to paint without a cache if the styles disagree? Cache key would be LayoutObject+style.
https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt:1: PASS if no crash On 2017/04/28 at 18:39:23, chrishtr wrote: > Use testharness.js to avoid the expectation file. Example in https://codereview.chromium.org/2844593002 Not a big fan of using testharness just to be able to get rid of the expected file TBH. Granted it's a bit silly that we should be needing expectation files for tests like this... I can rewrite it though if you prefer. https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:525: SVGResourcesCache::TemporaryStyleScope scope(ParentInlineLayoutObject(), On 2017/04/28 at 18:39:23, chrishtr wrote: > How about instead teaching the SVG painting code to paint without a cache > if the styles disagree? Cache key would be LayoutObject+style. Yes, that's what I'd like to be able to do eventually (generate a paint without having to go through a HashMap in the paint server LayoutObject.) We're ways off from being able to do that though.
https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt:1: PASS if no crash On 2017/04/28 at 18:59:52, fs wrote: > On 2017/04/28 at 18:39:23, chrishtr wrote: > > Use testharness.js to avoid the expectation file. Example in https://codereview.chromium.org/2844593002 > > Not a big fan of using testharness just to be able to get rid of the expected file TBH. Granted it's a bit silly that we should be needing expectation files for tests like this... I can rewrite it though if you prefer. Well I just was reminded about it yesterday via a code review from Morten. Since it is just including two script files and calling test() it seems like a good thing to do to me. https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/SVGInlineTextBoxPainter.cpp:525: SVGResourcesCache::TemporaryStyleScope scope(ParentInlineLayoutObject(), On 2017/04/28 at 18:59:52, fs wrote: > On 2017/04/28 at 18:39:23, chrishtr wrote: > > How about instead teaching the SVG painting code to paint without a cache > > if the styles disagree? Cache key would be LayoutObject+style. > > Yes, that's what I'd like to be able to do eventually (generate a paint without having to go through a HashMap in the paint server LayoutObject.) We're ways off from being able to do that though. ok
lgtm
The CQ bit was checked by fs@opera.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash-expected.txt:1: PASS if no crash On 2017/04/28 at 19:59:39, chrishtr wrote: > On 2017/04/28 at 18:59:52, fs wrote: > > On 2017/04/28 at 18:39:23, chrishtr wrote: > > > Use testharness.js to avoid the expectation file. Example in https://codereview.chromium.org/2844593002 > > > > Not a big fan of using testharness just to be able to get rid of the expected file TBH. Granted it's a bit silly that we should be needing expectation files for tests like this... I can rewrite it though if you prefer. > > Well I just was reminded about it yesterday via a code review from Morten. Since it is just including > two script files and calling test() it seems like a good thing to do to me. Convert to testharness and removed -expected.txt file.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fs@opera.com to run a CQ dry run
https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash.html (right): https://codereview.chromium.org/2846513002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/svg/text/selection-pseudo-resource-invalidation-crash.html:13: document.body.offsetTop; On 2017/04/28 at 18:16:33, fs wrote: > On 2017/04/28 at 16:12:31, Stephen Chennney wrote: > > This might be to force layout. Can it be switched to runAfterLayoutAndPaint and still crash? > > Yes, we want layout to be clean before adding the filter (and selection) in the following line(s), then the rAF change will trigger style recalc (for the document) and subsequent paint. rALAP might work too since it should provide a layout, but I didn't not attempt to use it there. I tried inserting a runAfterLayoutAndPaint to replace the offsetTop, and then the crash no longer reproduced. I noticed that I needed to replace the first rAF with rALAP after the testharness conversion though, so did that in PS5.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm. I think everyone is on board now.
The CQ bit was checked by fs@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from chrishtr@chromium.org Link to the patchset: https://codereview.chromium.org/2846513002/#ps80001 (title: "Use rALAP instead of first rAF")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493740731880840,
"parent_rev": "9acc325d119247a27d0a007d56fc6cc2fa23edc7", "commit_rev":
"9eba37a11e54d4f0808f43db1371dc2e062efb8d"}
Message was sent while issue was closed.
Description was changed from
==========
More targeted resource-switching mechanism for SVG selection painting
The mechanism by which resources are generated for painting using
selection style for SVG text is a bit too heavy-handed, and can end up
invalidating both layout and other things. All that is needed is looking
up any <paint> ('fill' or 'stroke') references and invalidating any
state from the non-selection style.
Use a reduced/tailored version of SVGResourcesCache::ClientStyleChanged
that only recreates/swaps the SVGResources object for the LayoutObject
and wrap that mechanism in a scope object.
BUG=713024
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
More targeted resource-switching mechanism for SVG selection painting
The mechanism by which resources are generated for painting using
selection style for SVG text is a bit too heavy-handed, and can end up
invalidating both layout and other things. All that is needed is looking
up any <paint> ('fill' or 'stroke') references and invalidating any
state from the non-selection style.
Use a reduced/tailored version of SVGResourcesCache::ClientStyleChanged
that only recreates/swaps the SVGResources object for the LayoutObject
and wrap that mechanism in a scope object.
BUG=713024
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2846513002
Cr-Commit-Position: refs/heads/master@{#468658}
Committed:
https://chromium.googlesource.com/chromium/src/+/9eba37a11e54d4f0808f43db1371...
==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://chromium.googlesource.com/chromium/src/+/9eba37a11e54d4f0808f43db1371... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
