Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1415143005:
Preparation for enabling slimming paint synchronized painting (Closed)
DescriptionPreparation for enabling slimming paint synchronized painting
Fix incomplete GraphicsLayer tree traversal in FrameView::synchronizedPaint().
Also traverse frame scrollbar layers, mask layers and replica layers.
Make unit tests ready.
BUG=536999
Committed: https://crrev.com/3f4d76a9c6735a47c77b973f452c88ef3ab467df
Cr-Commit-Position: refs/heads/master@{#357212}
Patch Set 1 #Patch Set 2 : #Patch Set 3 : #Patch Set 4 : #
Total comments: 15
Patch Set 5 : Rebase and fix webkit_unit_tests #
Total comments: 23
Patch Set 6 : #Patch Set 7 : #Patch Set 8 : Fix spv2 failures #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : Slimming CL #
Total comments: 2
Patch Set 12 : #
Total comments: 2
Patch Set 13 : Add comments for the loop finding real root GraphicsLayer #Messages
Total messages: 26 (7 generated)
wangxianzhu@chromium.org changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
Ptal. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/TestExpectations (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/TestExpectations:1012: crbug.com/548705 fast/css/first-line-hover-001.html [ Failure ] These bugs are exposed by synchronized painting, not caused by it. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/virtual/threaded/inspector/tracing/frame-model-instrumentation-expected.txt (left): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/virtual/threaded/inspector/tracing/frame-model-instrumentation-expected.txt:3: paints: present With synchronized painting, this test no longer generates different result with virtual/threaded. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:230: return; We need to check frame throttling in frame parent-child hierarchy, so use recursion here. A child frame of a throttling frame isn't explicitly marked as throttling. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2462: if (RuntimeEnabledFeatures::slimmingPaintSynchronizedPaintingEnabled() && !m_frame->document()->printing()) { Without this check, we'll mistakenly paint onto the graphics layers for screen with printing enabled. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2502: rootGraphicsLayer = parent; We need to start from the top-most graphics layer to paint the (inner and outer) frame scrollbars, scroll corners, etc. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2522: synchronizedPaintRecursively(replicaLayer, interestRect); These GraphicsLayers are not in the normal parent-child hierarchy. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2632: if (lifecycle().state() < DocumentLifecycle::PaintInvalidationClean) This check is moved out of the loop below, so that we can also check it for this frame. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/LayerClipRecorderTest.cpp (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/LayerClipRecorderTest.cpp:63: rootPaintController().invalidateAll(); Synchronized painting generates display item for the first layout of about:blank before the first line of the test is executed. This is to avoid the cache from affecting the test below. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/LayoutObjectDrawingRecorderTest.cpp (left): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/LayoutObjectDrawingRecorderTest.cpp:21: using LayoutObjectDrawingRecorderTestForSlimmingPaintV2 = PaintControllerPaintTestForSlimmingPaintV2; This was not used. We also don't need to test for v2 because the tests cover common code. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintControllerPaintTest.h:72: class PaintControllerPaintTestForSlimmingPaintV1AndV2 With this we can run the same test for both v1 and v2. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayer.cpp (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayer.cpp:2678: markAncestorChainForNeedsRepaint(); This is simplified because now we recursively clear repaint flags of all layers after painting. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp:481: && m_paintLayer.isSelfPaintingLayer() This avoids unnecessary subsequence display items for non-self-painting layer having self-painting descendants. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp (left): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp:21: using PaintLayerPainterTestForSlimmingPaintV2 = PaintControllerPaintTestForSlimmingPaintV2; This test suite tests synchronized painting, covering the common code of v1 and v2. In addition, it's already parameterized for root-layer-scrolls, so can't combine another parameter for v1 and v2. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/GraphicsLayer.cpp:310: #endif These are copied from GraphicsLayer::paint(). Perhaps we can combine paintIfNeeded() and paint(). I have another CL for this (WIP): https://codereview.chromium.org/1428643004. https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/LinkHighlightImplTest.cpp (right): https://codereview.chromium.org/1415143005/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/LinkHighlightImplTest.cpp:62: FrameTestHelpers::UseMockScrollbarSettings mockScrollbarSettings; This is added because we can't paint scrollbars using the native theme which requires chromium-side resource which is missing during test. We didn't encounter the issue before because we didn't paint in the tests.
https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2633: if (childFrameView->lifecycle().state() < DocumentLifecycle::PaintInvalidationClean) Why these changes? What was wrong? https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2632: if (lifecycle().state() < DocumentLifecycle::PaintInvalidationClean) Why this change? https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:163: SlimmingPaintSynchronizedPainting implied_by=SlimmingPaintV2, status=stable Revert this, and instead put any needed conditional logic behind the flag. A separate CL can flip the flag. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:127: // Slimming paint v1 CompositedLayerMapping may invalidate client on extra layers. v2 or v1? https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/ImeOnFocusTest.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/ImeOnFocusTest.cpp:74: FrameTestHelpers::UseMockScrollbarSettings m_mockScrollbarSettings; What are all these scrollbar settings diffs?
https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:227: void FrameView::forAllNonThrottlingFrameViews(Function function) supernit: forAllNonThrottledFrameViews https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:283: || (m_newDisplayItemList.size() == 2 && DisplayItem::nonCachedType(m_newDisplayItemList[0].type()) == DisplayItem::DebugRedFill)); Why does synchronous painting cause us to hit this codepath with cached debug red fills whereas we didn't before? https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebGraphicsContextImpl.h (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebGraphicsContextImpl.h:37: DisplayItemCacheSkipper m_cacheSkipper; I wouldn't expect WebGraphicsContext to disable caching. Is this just for PageOverlay? If so, can we explicitly disable caching in PageOverlay?
https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2633: if (childFrameView->lifecycle().state() < DocumentLifecycle::PaintInvalidationClean) On 2015/10/28 21:12:58, chrishtr wrote: > Why these changes? What was wrong? This check is moved out, so that we also check it for the first FrameView. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:227: void FrameView::forAllNonThrottlingFrameViews(Function function) On 2015/10/29 04:06:10, pdr wrote: > supernit: forAllNonThrottledFrameViews Done. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:163: SlimmingPaintSynchronizedPainting implied_by=SlimmingPaintV2, status=stable On 2015/10/28 21:12:58, chrishtr wrote: > Revert this, and instead put any needed conditional logic behind the flag. A > separate CL can flip the flag. Done. This was included in this and previous patch sets to see try results. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:127: // Slimming paint v1 CompositedLayerMapping may invalidate client on extra layers. On 2015/10/28 21:12:58, chrishtr wrote: > v2 or v1? v1. For example, in invalidatePaintIncludingNonCompoisitingDescendants(), we may invalidate a fixed-pos object on a paint invalidation container which the fixed-pos object doesn't belong to. Here we add into m_invalidations only if the object has previously cached to avoid ASSERT(m_invalidations.isEmpty()) failure when a whole GraphicsLayer is cached but something unrelated was invalidated on the layer. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:283: || (m_newDisplayItemList.size() == 2 && DisplayItem::nonCachedType(m_newDisplayItemList[0].type()) == DisplayItem::DebugRedFill)); On 2015/10/29 04:06:10, pdr wrote: > Why does synchronous painting cause us to hit this codepath with cached debug > red fills whereas we didn't before? This assert is only for CachedDisplayItemList which is a sync painting feature. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebGraphicsContextImpl.h (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebGraphicsContextImpl.h:37: DisplayItemCacheSkipper m_cacheSkipper; On 2015/10/29 04:06:10, pdr wrote: > I wouldn't expect WebGraphicsContext to disable caching. Is this just for > PageOverlay? If so, can we explicitly disable caching in PageOverlay? WebGraphicsContext seems to expect that no cache exists (in WebGraphicsContextImpl::beginDrawing(): https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) I encountered assertion failure in WebGraphicsContextImpl::beginDrawing(). Should we change beginDrawing() to allow caching? https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/ImeOnFocusTest.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/ImeOnFocusTest.cpp:74: FrameTestHelpers::UseMockScrollbarSettings m_mockScrollbarSettings; On 2015/10/28 21:12:58, chrishtr wrote: > What are all these scrollbar settings diffs? This is to let the tests paint scrollbars using the mock theme instead of the native theme. The native theme is implemented at chromium side. It requires some resources that are not initialized or not available for webkit tests and will crash. Before sync painting, the tests didn't trigger paint and the problem.
Description was changed from ========== Enable slimming paint synchronized painting This also enables subsequence caching under the same flag. BUG=536999 ========== to ========== Preparation for enabling slimming paint synchronized painting This CL fixes most failures that enabling slimming paint synchronized painting would cause: - subsequence caching failures, about incorrect marking of needsRepaint, unnecessary subsequence caching for overlay scrollbars, etc. - Scrollbar painted in native theme in unit tests which will DCHECK failure in chromium code when required resources are not found. This is a problem with sync painting because we'll do more painting that the unit tests didn't do before. - make unit tests ready. BUG=536999 ==========
https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2633: if (childFrameView->lifecycle().state() < DocumentLifecycle::PaintInvalidationClean) On 2015/10/29 at 04:22:01, Xianzhu wrote: > On 2015/10/28 21:12:58, chrishtr wrote: > > Why these changes? What was wrong? > > This check is moved out, so that we also check it for the first FrameView. Not sure what you mean.. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2632: if (lifecycle().state() < DocumentLifecycle::PaintInvalidationClean) On 2015/10/28 at 21:12:58, chrishtr wrote: > Why this change? Is this the same reason as the comment thread below? https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:127: // Slimming paint v1 CompositedLayerMapping may invalidate client on extra layers. On 2015/10/29 at 04:22:01, Xianzhu wrote: > On 2015/10/28 21:12:58, chrishtr wrote: > > v2 or v1? > > v1. For example, in invalidatePaintIncludingNonCompoisitingDescendants(), we may invalidate a fixed-pos object on a paint invalidation container which the fixed-pos object doesn't belong to. Here we add into m_invalidations only if the object has previously cached to avoid ASSERT(m_invalidations.isEmpty()) failure when a whole GraphicsLayer is cached but something unrelated was invalidated on the layer. Yikes. How does this situation avoid causing the whole GraphicsLayer caching to fail? https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebGraphicsContextImpl.h (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebGraphicsContextImpl.h:37: DisplayItemCacheSkipper m_cacheSkipper; On 2015/10/29 at 04:22:01, Xianzhu wrote: > On 2015/10/29 04:06:10, pdr wrote: > > I wouldn't expect WebGraphicsContext to disable caching. Is this just for > > PageOverlay? If so, can we explicitly disable caching in PageOverlay? > > WebGraphicsContext seems to expect that no cache exists (in WebGraphicsContextImpl::beginDrawing(): https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...) > > I encountered assertion failure in WebGraphicsContextImpl::beginDrawing(). > > Should we change beginDrawing() to allow caching? Let's delete the beginDrawing()/endDrawing() methods and inline the skipper and DrawingRecorder in WebViewImpl::paintPageOverlay. That's the only call site. Also it's safe to use toWebGraphicsContextImpl() there to cast since the only call site uses that class. InspectorOverlay does the same thing already. Please also break some of these changes into new patches. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/ImeOnFocusTest.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/ImeOnFocusTest.cpp:74: FrameTestHelpers::UseMockScrollbarSettings m_mockScrollbarSettings; On 2015/10/29 at 04:22:01, Xianzhu wrote: > On 2015/10/28 21:12:58, chrishtr wrote: > > What are all these scrollbar settings diffs? > > This is to let the tests paint scrollbars using the mock theme instead of the native theme. > > The native theme is implemented at chromium side. It requires some resources that are not initialized or not available for webkit tests and will crash. > > Before sync painting, the tests didn't trigger paint and the problem. I see. You should tell schenney@ about that since he is investigating getting rid of the mock theme, but it seems this is a reason to keep it.
With several CLs separated out, this CL becomes much smaller. https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (left): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:2633: if (childFrameView->lifecycle().state() < DocumentLifecycle::PaintInvalidationClean) On 2015/10/29 20:22:45, chrishtr wrote: > On 2015/10/29 at 04:22:01, Xianzhu wrote: > > On 2015/10/28 21:12:58, chrishtr wrote: > > > Why these changes? What was wrong? > > > > This check is moved out, so that we also check it for the first FrameView. > > Not sure what you mean.. The above line is moved to new line 2632. Previously we checked this for descendants only, not the FrameView of the top level invalidateTreeIfNeededRecursive(). https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:127: // Slimming paint v1 CompositedLayerMapping may invalidate client on extra layers. On 2015/10/29 20:22:45, chrishtr wrote: > On 2015/10/29 at 04:22:01, Xianzhu wrote: > > On 2015/10/28 21:12:58, chrishtr wrote: > > > v2 or v1? > > > > v1. For example, in invalidatePaintIncludingNonCompoisitingDescendants(), we > may invalidate a fixed-pos object on a paint invalidation container which the > fixed-pos object doesn't belong to. Here we add into m_invalidations only if the > object has previously cached to avoid ASSERT(m_invalidations.isEmpty()) failure > when a whole GraphicsLayer is cached but something unrelated was invalidated on > the layer. > > Yikes. How does this situation avoid causing the whole GraphicsLayer caching to > fail? Sometimes somehow we also invalidate the object in other paths on the correct paint invalidation container, so here we ignore the extra one. Sometimes it's a bug (crbug.com/548495 -- to be fixed by https://codereview.chromium.org/1413823005/ (WIP)).
Description was changed from ========== Preparation for enabling slimming paint synchronized painting This CL fixes most failures that enabling slimming paint synchronized painting would cause: - subsequence caching failures, about incorrect marking of needsRepaint, unnecessary subsequence caching for overlay scrollbars, etc. - scrollbar painted in native theme in unit tests which will DCHECK failure in chromium code when required resources are not found. This is a problem with sync painting because we'll do more painting that the unit tests didn't do before. - incomplete GraphicsLayer tree traversal in FrameView::synchronizedPaint(). Should also traverse frame scrollbar layers, mask layers and replica layers. Also make unit tests ready. BUG=536999 ========== to ========== Preparation for enabling slimming paint synchronized painting Fix incomplete GraphicsLayer tree traversal in FrameView::synchronizedPaint(). Also traverse frame scrollbar layers, mask layers and replica layers. Make unit tests ready. BUG=536999 ==========
https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp (right): https://codereview.chromium.org/1415143005/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp:127: // Slimming paint v1 CompositedLayerMapping may invalidate client on extra layers. On 2015/10/30 00:03:57, Xianzhu wrote: > On 2015/10/29 20:22:45, chrishtr wrote: > > On 2015/10/29 at 04:22:01, Xianzhu wrote: > > > On 2015/10/28 21:12:58, chrishtr wrote: > > > > v2 or v1? > > > > > > v1. For example, in invalidatePaintIncludingNonCompoisitingDescendants(), we > > may invalidate a fixed-pos object on a paint invalidation container which the > > fixed-pos object doesn't belong to. Here we add into m_invalidations only if > the > > object has previously cached to avoid ASSERT(m_invalidations.isEmpty()) > failure > > when a whole GraphicsLayer is cached but something unrelated was invalidated > on > > the layer. > > > > Yikes. How does this situation avoid causing the whole GraphicsLayer caching > to > > fail? > > Sometimes somehow we also invalidate the object in other paths on the correct > paint invalidation container, so here we ignore the extra one. > > Sometimes it's a bug (crbug.com/548495 -- to be fixed by > https://codereview.chromium.org/1413823005/ (WIP)). Also crbug.com/416535.
Looking good modulo dependent CLs that I think are still in flight. https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2506: while (GraphicsLayer* parent = rootGraphicsLayer->parent()) Why this while loop?
This CL doesn't actually enable sync painting (description updated), so it's safe to land, not depending other CLs. https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2506: while (GraphicsLayer* parent = rootGraphicsLayer->parent()) On 2015/10/30 19:01:13, chrishtr wrote: > Why this while loop? There may be other paintable layers (e.g. frame scrollbar layers, virtual viewport layers) not in the layer subtree of the LayoutView and we also need to paint them. I'm not sure what these layers will be for spv2. Added a TODO in the 'if (spv2)' block.
On 2015/10/30 at 19:28:57, wangxianzhu wrote: > This CL doesn't actually enable sync painting (description updated), so it's safe to land, not depending other CLs. > > https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameView.cpp:2506: while (GraphicsLayer* parent = rootGraphicsLayer->parent()) > On 2015/10/30 19:01:13, chrishtr wrote: > > Why this while loop? > > There may be other paintable layers (e.g. frame scrollbar layers, virtual viewport layers) not in the layer subtree of the LayoutView and we also need to paint them. > > I'm not sure what these layers will be for spv2. Added a TODO in the 'if (spv2)' block. Why not just use the root layer from the WebViewImpl then? It must be available there somehow, e.g. as the rootLayer of the root PaintLayerCompositor.
On 2015/10/30 20:48:48, chrishtr wrote: > On 2015/10/30 at 19:28:57, wangxianzhu wrote: > > This CL doesn't actually enable sync painting (description updated), so it's > safe to land, not depending other CLs. > > > > > https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > > > > https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/frame/FrameView.cpp:2506: while (GraphicsLayer* > parent = rootGraphicsLayer->parent()) > > On 2015/10/30 19:01:13, chrishtr wrote: > > > Why this while loop? > > > > There may be other paintable layers (e.g. frame scrollbar layers, virtual > viewport layers) not in the layer subtree of the LayoutView and we also need to > paint them. > > > > I'm not sure what these layers will be for spv2. Added a TODO in the 'if > (spv2)' block. > > Why not just use the root layer from the WebViewImpl then? It must be available > there somehow, e.g. as the rootLayer of the root PaintLayerCompositor. The rootLayer of the root PaintLayerCompositor is still not the actual root. Virtual viewport may create more paintable layers (e.g. scrollbar layers overriding the scrollbar layers created by the root PaintLayerCompositor) above the compositor()->rootGraphicsLayer(), and we still need a loop to find the real root.
On 2015/10/30 20:56:52, Xianzhu wrote: > On 2015/10/30 20:48:48, chrishtr wrote: > > On 2015/10/30 at 19:28:57, wangxianzhu wrote: > > > This CL doesn't actually enable sync painting (description updated), so it's > > safe to land, not depending other CLs. > > > > > > > > > https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... > > > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > > > > > > > > https://codereview.chromium.org/1415143005/diff/200001/third_party/WebKit/Sou... > > > third_party/WebKit/Source/core/frame/FrameView.cpp:2506: while > (GraphicsLayer* > > parent = rootGraphicsLayer->parent()) > > > On 2015/10/30 19:01:13, chrishtr wrote: > > > > Why this while loop? > > > > > > There may be other paintable layers (e.g. frame scrollbar layers, virtual > > viewport layers) not in the layer subtree of the LayoutView and we also need > to > > paint them. > > > > > > I'm not sure what these layers will be for spv2. Added a TODO in the 'if > > (spv2)' block. > > > > Why not just use the root layer from the WebViewImpl then? It must be > available > > there somehow, e.g. as the rootLayer of the root PaintLayerCompositor. > > The rootLayer of the root PaintLayerCompositor is still not the actual root. > Virtual viewport may create more paintable layers (e.g. scrollbar layers > overriding the scrollbar layers created by the root PaintLayerCompositor) above > the compositor()->rootGraphicsLayer(), and we still need a loop to find the real > root. We could find the actual root layer through frameView->page()->frameHost().visualViewport(), but I feel that the current method has the least dependency on other things which might change (I'm not sure how) for spv2.
lgtm https://codereview.chromium.org/1415143005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2507: while (GraphicsLayer* parent = rootGraphicsLayer->parent()) Ok can you add a comment here explaining?
https://codereview.chromium.org/1415143005/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/1415143005/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameView.cpp:2507: while (GraphicsLayer* parent = rootGraphicsLayer->parent()) On 2015/10/30 21:12:57, chrishtr wrote: > Ok can you add a comment here explaining? Done.
The CQ bit was checked by wangxianzhu@chromium.org
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/1415143005/#ps240001 (title: "Add comments for the loop finding real root GraphicsLayer")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415143005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415143005/240001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by wangxianzhu@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1415143005/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1415143005/240001
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/3f4d76a9c6735a47c77b973f452c88ef3ab467df Cr-Commit-Position: refs/heads/master@{#357212} |
