Change did scroll callback to be based on ElementIds instead of a raw pointer |
||
Issue descriptionScrollableArea's DidScroll is used as a callback from cc::Layer for scrolling. We learned in crbug.com/747719 that this is dangerous if the scrollable area is deleted. For SPV1 a workaround was added to clear the cc::Layer callback when the scrollable area is deleted. SPV2 does not have a clean way to access the associated cc::Layer. We should refactor this code to use ElementIds instead of raw pointers.
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1920aec474240e9a328962f5d20520b2effe356b commit 1920aec474240e9a328962f5d20520b2effe356b Author: Philip Rogers <pdr@chromium.org> Date: Mon Aug 28 16:45:10 2017 Set DidScroll callback (WebScrollLayerClient) once in PAC The DidScroll callback was stored once per ScrollPaintPropertyNode but is static for all of PaintArtifactCompositor (PAC). This patch moves the WebScrollLayerClient to PaintArtifactCompositor and removes it from ScrollPaintPropertyNode. The WebScrollLayerClient is stored on the Page which owns PaintArtifactCompositor, so there are no lifetime issues. This also removes one more difference between blink and cc property trees. Bug: 758360 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I3bf5700b360f1ff7983fc4923f1a39a759b6646c Reviewed-on: https://chromium-review.googlesource.com/636698 Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#497778} [modify] https://crrev.com/1920aec474240e9a328962f5d20520b2effe356b/third_party/WebKit/Source/core/frame/LocalFrameView.cpp [modify] https://crrev.com/1920aec474240e9a328962f5d20520b2effe356b/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp [modify] https://crrev.com/1920aec474240e9a328962f5d20520b2effe356b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp [modify] https://crrev.com/1920aec474240e9a328962f5d20520b2effe356b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h [modify] https://crrev.com/1920aec474240e9a328962f5d20520b2effe356b/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositorTest.cpp [modify] https://crrev.com/1920aec474240e9a328962f5d20520b2effe356b/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.cpp [modify] https://crrev.com/1920aec474240e9a328962f5d20520b2effe356b/third_party/WebKit/Source/platform/graphics/paint/ScrollPaintPropertyNode.h
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8f212abdd8fefeea395d04f69fd140a299ed73bb commit 8f212abdd8fefeea395d04f69fd140a299ed73bb Author: Philip Rogers <pdr@chromium.org> Date: Mon Aug 28 21:24:30 2017 Add a SPV2 test of scrollable area changes that affect cc::Layers This patch adds a test with slimming paint V2 enabled where a scrollable area is deleted and impl-side DidScroll callbacks to not crash. Before [1], the DidScroll callback was on ScrollableArea and deleting a ScrollableArea without updating compositing could lead to a crash if an impl-side scroll tried to notify a deleted ScrollableArea. [1] https://chromium.googlesource.com/chromium/src/+/e7b75b84514adffc3f0a494e82c7de22708b0ec6 Bug: 758028 , 758360 , 747719 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I61489c5138c0b54b55d841f9ab9ff61fe5b9b3eb Reviewed-on: https://chromium-review.googlesource.com/636056 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Philip Rogers <pdr@chromium.org> Cr-Commit-Position: refs/heads/master@{#497879} [modify] https://crrev.com/8f212abdd8fefeea395d04f69fd140a299ed73bb/third_party/WebKit/Source/core/exported/WebFrameTest.cpp [modify] https://crrev.com/8f212abdd8fefeea395d04f69fd140a299ed73bb/third_party/WebKit/Source/core/frame/LocalFrameView.h [modify] https://crrev.com/8f212abdd8fefeea395d04f69fd140a299ed73bb/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp [modify] https://crrev.com/8f212abdd8fefeea395d04f69fd140a299ed73bb/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
,
Aug 28 2017
,
Aug 28 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24a13a10bede5ea482343990bd14b72ca1d742c3 commit 24a13a10bede5ea482343990bd14b72ca1d742c3 Author: Tarun Bansal <tbansal@chromium.org> Date: Mon Aug 28 22:22:40 2017 Speculative revert "Add a SPV2 test of scrollable area changes that affect cc::Layers" This reverts commit 8f212abdd8fefeea395d04f69fd140a299ed73bb. Reason for revert: Breaks build: https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Win%20x64%20Builder%20%28dbg%29/builds/112482 Original change's description: > Add a SPV2 test of scrollable area changes that affect cc::Layers > > This patch adds a test with slimming paint V2 enabled where a scrollable > area is deleted and impl-side DidScroll callbacks to not crash. Before > [1], the DidScroll callback was on ScrollableArea and deleting a > ScrollableArea without updating compositing could lead to a crash if > an impl-side scroll tried to notify a deleted ScrollableArea. > > [1] https://chromium.googlesource.com/chromium/src/+/e7b75b84514adffc3f0a494e82c7de22708b0ec6 > > Bug: 758028 , 758360 , 747719 > Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 > Change-Id: I61489c5138c0b54b55d841f9ab9ff61fe5b9b3eb > Reviewed-on: https://chromium-review.googlesource.com/636056 > Reviewed-by: Chris Harrelson <chrishtr@chromium.org> > Commit-Queue: Philip Rogers <pdr@chromium.org> > Cr-Commit-Position: refs/heads/master@{#497879} TBR=pdr@chromium.org,chrishtr@chromium.org Change-Id: Ib72b249e2f81f42acda9d28263e37413fbae9fe1 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 758028 , 758360 , 747719 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Reviewed-on: https://chromium-review.googlesource.com/639166 Reviewed-by: Tarun Bansal <tbansal@chromium.org> Commit-Queue: Tarun Bansal <tbansal@chromium.org> Cr-Commit-Position: refs/heads/master@{#497899} [modify] https://crrev.com/24a13a10bede5ea482343990bd14b72ca1d742c3/third_party/WebKit/Source/core/exported/WebFrameTest.cpp [modify] https://crrev.com/24a13a10bede5ea482343990bd14b72ca1d742c3/third_party/WebKit/Source/core/frame/LocalFrameView.h [modify] https://crrev.com/24a13a10bede5ea482343990bd14b72ca1d742c3/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp [modify] https://crrev.com/24a13a10bede5ea482343990bd14b72ca1d742c3/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h
,
Aug 29 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/dfcf5438f0c1282eba7b4798ea30da5528bde235 commit dfcf5438f0c1282eba7b4798ea30da5528bde235 Author: pdr <pdr@chromium.org> Date: Tue Aug 29 03:52:33 2017 [Reland] Add a SPV2 test of scrollable area changes that affect cc::Layers This patch adds a test with slimming paint V2 enabled where a scrollable area is deleted and impl-side DidScroll callbacks to not crash. Before [1], the DidScroll callback was on ScrollableArea and deleting a ScrollableArea without updating compositing could lead to a crash if an impl-side scroll tried to notify a deleted ScrollableArea. This patch is a reland of [2] which was rolled out in [3] for a compile failure on Windows. This patch is equal to [2] except a PLATFORM_EXPORT has been added in PaintArtifactCompositor.h. [1] https://chromium.googlesource.com/chromium/src/+/e7b75b84514adffc3f0a494e82c7de22708b0ec6 [2] https://chromium.googlesource.com/chromium/src/+/8f212abdd8fefeea395d04f69fd140a299ed73bb [3] https://chromium.googlesource.com/chromium/src/+/24a13a10bede5ea482343990bd14b72ca1d742c3 Bug: 758028 , 758360 , 747719 Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Change-Id: I15b92abada84eb84eed18d4f9fa2f0d093f90e33 Reviewed-on: https://chromium-review.googlesource.com/639750 Reviewed-by: Chris Harrelson <chrishtr@chromium.org> Commit-Queue: Chris Harrelson <chrishtr@chromium.org> Cr-Commit-Position: refs/heads/master@{#498015} [modify] https://crrev.com/dfcf5438f0c1282eba7b4798ea30da5528bde235/third_party/WebKit/Source/core/exported/WebFrameTest.cpp [modify] https://crrev.com/dfcf5438f0c1282eba7b4798ea30da5528bde235/third_party/WebKit/Source/core/frame/LocalFrameView.h [modify] https://crrev.com/dfcf5438f0c1282eba7b4798ea30da5528bde235/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.cpp [modify] https://crrev.com/dfcf5438f0c1282eba7b4798ea30da5528bde235/third_party/WebKit/Source/platform/graphics/compositing/PaintArtifactCompositor.h |
||
►
Sign in to add a comment |
||
Comment 1 by bugdroid1@chromium.org
, Aug 25 2017