An "inclusive" compositing layer is one that can paint a set of other (usually sibling) layers that would otherwise be composited for overlap. They will reduce the amount of backing store, and the need for scrolling tree nodes in some cases.
Created attachment 369082[details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369083[details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 369085[details]
Archive of layout-test-results from ews114 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369087[details]
Archive of layout-test-results from ews214 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 369088[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369080[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=369080&action=review> Source/WebCore/rendering/RenderLayer.cpp:1817
> + RenderLayer* repaintTarget;
> + if (includeSelf == IncludeSelf && (repaintTarget = repaintTargetForLayer(*this)))
> + return repaintTarget;
No reason to leave repaintTarget uninitialized.
> Source/WebCore/rendering/RenderLayer.h:818
> + // If non,null, a non-ancestor composited layer that this layer paints into (it is sharing its backing store with this layer).
"non-null"
> Source/WebCore/rendering/RenderLayer.h:822
> + RenderLayer* sharedLayer() const { return m_sharedLayer; }
> + void setSharedLayer(RenderLayer* sharedLayer) { m_sharedLayer = sharedLayer; }
> +
> + bool paintsIntoSharedLayer() const { return m_sharedLayer; }
"Shared layer" does not really describe the feature. I feel it would be clearer to express this in terms of "shared backing" in code. This might be just a matter of renaming and probably also switching to RenderLayerBacking ptrs here.
I wonder if there some sort of future refactoring here that actually makes RenderLayerBacking a shared object?
> Source/WebCore/rendering/RenderLayer.h:1268
> + // If non,null, a non-ancestor composited layer that this layer paints into.
"non-null"
> Source/WebCore/rendering/RenderLayerBacking.cpp:264
> + for (auto* layer : m_sharingLayers)
> + layer->setSharedLayer(nullptr);
This kind of stuff is scary. Layer tree should use WeakPtrs.
It would be better to call clearSharingLayers() to avoid duplication.
> Source/WebCore/rendering/RenderLayerBacking.cpp:2678
> + for (auto* layer : m_sharingLayers)
> + paintOneLayer(*layer, sharingLayerPaintFlags);
I suppose region gathering code needs to replicate this logic too. Please file a bug.
> Source/WebCore/rendering/RenderLayerBacking.h:398
> + // When this layer is a shared layer, sharing layers other than m_owningLayer are in this list, and paint into our backing store.
This is a confusing sentence.
(In reply to Antti Koivisto from comment #15)
> Comment on attachment 369080[details]
> Patch
>
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=369080&action=review
>
> > Source/WebCore/rendering/RenderLayer.cpp:1817
> > + RenderLayer* repaintTarget;
> > + if (includeSelf == IncludeSelf && (repaintTarget = repaintTargetForLayer(*this)))
> > + return repaintTarget;
>
> No reason to leave repaintTarget uninitialized.
>
> > Source/WebCore/rendering/RenderLayer.h:818
> > + // If non,null, a non-ancestor composited layer that this layer paints into (it is sharing its backing store with this layer).
>
> "non-null"
>
> > Source/WebCore/rendering/RenderLayer.h:822
> > + RenderLayer* sharedLayer() const { return m_sharedLayer; }
> > + void setSharedLayer(RenderLayer* sharedLayer) { m_sharedLayer = sharedLayer; }
> > +
> > + bool paintsIntoSharedLayer() const { return m_sharedLayer; }
>
> "Shared layer" does not really describe the feature. I feel it would be
> clearer to express this in terms of "shared backing" in code. This might be
> just a matter of renaming and probably also switching to RenderLayerBacking
> ptrs here.
I agree that "shared" isn't too descriptive. Other implementations have called them "inclusive" which has clearer directionality ("inclusive, included") but Zalan didn't like that.
I don't want to use RLB pointers because I don't want to introduce a dependency on when backing actually gets created.
> I wonder if there some sort of future refactoring here that actually makes
> RenderLayerBacking a shared object?
Perhaps, though there is still a clear "primary" owner.
> > Source/WebCore/rendering/RenderLayer.h:1268
> > + // If non,null, a non-ancestor composited layer that this layer paints into.
>
> "non-null"
>
> > Source/WebCore/rendering/RenderLayerBacking.cpp:264
> > + for (auto* layer : m_sharingLayers)
> > + layer->setSharedLayer(nullptr);
>
> This kind of stuff is scary. Layer tree should use WeakPtrs.
Will fix.
> It would be better to call clearSharingLayers() to avoid duplication.
>
> > Source/WebCore/rendering/RenderLayerBacking.cpp:2678
> > + for (auto* layer : m_sharingLayers)
> > + paintOneLayer(*layer, sharingLayerPaintFlags);
>
> I suppose region gathering code needs to replicate this logic too. Please
> file a bug.
Good point.
> > Source/WebCore/rendering/RenderLayerBacking.h:398
> > + // When this layer is a shared layer, sharing layers other than m_owningLayer are in this list, and paint into our backing store.
>
> This is a confusing sentence.
Will fix.
Created attachment 369243[details]
Archive of layout-test-results from ews101 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369245[details]
Archive of layout-test-results from ews117 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369246[details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 369251[details]
Archive of layout-test-results from ews123 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Created attachment 369294[details]
Archive of layout-test-results from ews210 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Created attachment 369316[details]
Archive of layout-test-results from ews122 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Created attachment 369320[details]
Archive of layout-test-results from ews115 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369359[details]
Archive of layout-test-results from ews112 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 369360[details]
Archive of layout-test-results from ews126 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.4
Comment on attachment 369357[details]
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=369357&action=review
Looks fine. Fixing updateEventRegion shouldn't be difficult either (same thing as paintIntoLayer) but it can be done separately if you prefer.
> Source/WebCore/rendering/RenderLayer.cpp:1774
> +// ASSERT_IMPLIES(backingProvider, !isComposited());
Commented-out code.
> Source/WebCore/rendering/RenderLayerBacking.cpp:679
> + if (!layerWeakPtr) {
> + ASSERT_NOT_REACHED();
> + continue;
> + }
This seems overly defensive. We should just crash with nullptr if the logic is supposed to guarantee that these are non-null (here and elsewhere).
Created attachment 369364[details]
Archive of layout-test-results from ews210 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
[885/2246] Building CXX object Source/WebCore/CMakeFiles/...es/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp.o
In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-043dd90b-8.cpp:5:
../../Source/WebCore/rendering/RenderLayer.cpp: In member function ‘WTF::Ref<WebCore::ClipRects> WebCore::RenderLayer::updateClipRects(const WebCore::RenderLayer::ClipRectsContext&)’:
../../Source/WebCore/rendering/RenderLayer.cpp:5503:15: warning: unused variable ‘parentLayer’ [-Wunused-variable]
5503 | if (auto* parentLayer = (clipRectsContext.rootLayer != this ? parent() : nullptr))
| ^~~~~~~~~~~
This condition here:
RefPtr<ClipRects> parentClipRects;
// For transformed layers, the root layer was shifted to be us, so there is no need to
// examine the parent. We want to cache clip rects with us as the root.
if (auto* parentLayer = (clipRectsContext.rootLayer != this ? parent() : nullptr))
parentClipRects = this->parentClipRects(clipRectsContext);
Should now be simplified to:
RefPtr<ClipRects> parentClipRects;
// For transformed layers, the root layer was shifted to be us, so there is no need to
// examine the parent. We want to cache clip rects with us as the root.
if (clipRectsContext.rootLayer != this)
parentClipRects = this->parentClipRects(clipRectsContext);
2019-05-04 19:28 PDT, Simon Fraser (smfr)
2019-05-04 20:37 PDT, EWS Watchlist
2019-05-04 20:48 PDT, EWS Watchlist
2019-05-04 21:01 PDT, EWS Watchlist
2019-05-04 21:23 PDT, EWS Watchlist
2019-05-04 21:33 PDT, EWS Watchlist
2019-05-06 10:35 PDT, Simon Fraser (smfr)
2019-05-06 22:23 PDT, Simon Fraser (smfr)
2019-05-06 22:27 PDT, Simon Fraser (smfr)
2019-05-06 22:35 PDT, Simon Fraser (smfr)
2019-05-06 23:34 PDT, EWS Watchlist
2019-05-06 23:52 PDT, EWS Watchlist
2019-05-06 23:57 PDT, EWS Watchlist
2019-05-07 00:30 PDT, EWS Watchlist
2019-05-07 09:19 PDT, EWS Watchlist
2019-05-07 10:57 PDT, Simon Fraser (smfr)
2019-05-07 12:57 PDT, EWS Watchlist
2019-05-07 13:05 PDT, EWS Watchlist
2019-05-07 20:51 PDT, Simon Fraser (smfr)
2019-05-07 21:03 PDT, Simon Fraser (smfr)
ews-watchlist: commit-queue-
2019-05-07 22:53 PDT, EWS Watchlist
2019-05-07 23:04 PDT, EWS Watchlist
2019-05-08 01:44 PDT, EWS Watchlist