Bug 231009

<rdar://83437059>

Created attachment 441214 [details] Patch for feedback

Created attachment 441215 [details] Patch for feedback

I like the idea of a "content provider". Can it be a function on GraphicsLayerClient?

> I like the idea of a "content provider". Can it be a function on GraphicsLayerClient? In a way "yes", considering that current GraphicsLayerClient can provide contents via painting via "draws content". However, in practice "no", as the GraphicsLayerClient is general purpose and closed, the "content provider" is platform-specific and open-ended. I'd view it the other way, in that eventually GraphicsLayerClient::paintContents could be expressed as a "content provider", and RenderLayerBacking (or RenderLayers?) could be the instance that returns "paintContents content provider". RenderLayerBacking does now: m_layer->setDrawsContent(true) Instead it would do: m_layer->setContentFromProvider(this); RenderLayerBacking does now: m_layer->setDrawsContent(false); Instead it would do: m_layer->setContentFromProvider(nullptr); (or ->setNoContents()); However, it's probably not within my expertise to implement that part, and it'd be a refactor without much benefit..

Created attachment 444701 [details] Patch

Comment on attachment 444701 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=444701&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:525 > virtual void setContentsToPlatformLayer(PlatformLayer*, ContentsLayerPurpose) { } Do you have a plan to remove setContentsToPlatformLayer? > Source/WebCore/platform/graphics/GraphicsLayer.h:526 > + virtual void setContentsFromDisplayDelegate(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose); This name sounds inconsistent to me. Other methods setting the contents layer are named setContentsTo*. I prefer setContentsToDisplayDelegate.

(In reply to Kimmo Kinnunen from comment #5) > RenderLayerBacking does now: m_layer->setDrawsContent(true) > Instead it would do: m_layer->setContentFromProvider(this); setDrawsContent makes the primary layer paintable, not the content layer. I think it should be setPrimaryProvider or setPrimaryToProvider for the consistency.

Created attachment 444791 [details] Patch

Created attachment 444804 [details] Patch

Created attachment 444807 [details] Patch

Created attachment 444961 [details] Patch

Created attachment 444976 [details] Patch

Created attachment 445007 [details] Patch

Created attachment 445014 [details] Patch

Created attachment 445021 [details] Patch

Created attachment 445067 [details] Patch

Created attachment 445089 [details] Patch

Created attachment 445537 [details] Patch

Created attachment 445550 [details] Patch

Created attachment 445561 [details] Patch

Created attachment 445569 [details] Patch

Created attachment 445576 [details] Patch

Created attachment 445594 [details] Patch

Created attachment 445686 [details] Patch

Created attachment 445695 [details] Patch

Created attachment 445702 [details] Patch

Created attachment 445713 [details] Patch

Created attachment 445716 [details] Patch

Created attachment 445717 [details] Patch

Comment on attachment 445717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445717&action=review > Source/WebCore/platform/graphics/GraphicsLayer.h:526 > + virtual void setContentsToDisplayDelegate(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose); The name is a bit odd. Maybe setContentsDisplayDelegate? Does calling setContentsToSolidColor() or setContentsToPlatformLayer() remove the delegate? > Source/WebCore/platform/graphics/GraphicsLayerContentsDisplayDelegate.h:47 > + virtual void willDelegateDisplay(PlatformCALayer&); This name is ambiguous. On first reading I expect it to return a bool. Does it mean "prepare to render a frame"? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1363 > + m_contentsLayer = createPlatformCALayer(PlatformCALayer::LayerTypeContentsProvidedLayer, this); Naively I would expect that the GraphicsLayerContentsDisplayDelegate would do the layer creation? > Source/WebCore/platform/graphics/ca/GraphicsLayerCA.cpp:1369 > + m_contentsLayer->setBackingStoreAttached(true); > + m_contentsLayer->setAcceleratesDrawing(true); I feel like these should be under delegate control too? > Source/WebCore/platform/graphics/cocoa/WebProcessGraphicsContextGLCocoa.mm:49 > + } > + void display(PlatformCALayer& layer) final Blank lines between functions please. > Source/WebCore/platform/graphics/mac/WebLayer.mm:86 > + if (!layer->owner()->platformCALayerDrawsContent() && !layer->owner()->platformCALayerDelegatesDisplay(layer.get())) Maybe we should add a function that does both these checks. > Source/WebKit/WebProcess/GPU/graphics/cocoa/RemoteGraphicsContextGLProxyCocoa.mm:56 > + } > + void display(WebCore::PlatformCALayer& layer) final Blank lines please

Comment on attachment 445717 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=445717&action=review >> Source/WebCore/platform/graphics/GraphicsLayer.h:526 >> + virtual void setContentsToDisplayDelegate(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose); > > The name is a bit odd. Maybe setContentsDisplayDelegate? > > Does calling setContentsToSolidColor() or setContentsToPlatformLayer() remove the delegate? Just a idea. What about using overloading for the methods? virtual void setContents(Image*) { } virtual void setContents(const Color&) { } virtual void setContents(RefPtr<GraphicsLayerContentsDisplayDelegate>&&, ContentsLayerPurpose); virtual void setContents(RefPtr<Model>&&) { } > Source/WebKit/WebProcess/GPU/graphics/wc/RemoteGraphicsContextGLProxyWC.cpp:73 > + , m_layerContentsDisplayDelegate(PlatformLayerDisplayDelegate::create(makeUnique<WCPlatformLayerGCGL>(m_graphicsContextGLIdentifier))) We no longer need WCPlatformLayerGCGL. I will remove it in follow-up patch.

Created attachment 445846 [details] Patch

Created attachment 445872 [details] Patch

Created attachment 445990 [details] Patch * Fixed WinCairo build

Thanks Fujii!

Created attachment 446150 [details] Patch for landing

Created attachment 446151 [details] Patch for landing

Committed r286596 (?): <https://commits.webkit.org/r286596> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446151 [details].

I believe the changes in https://trac.webkit.org/changeset/286596/webkit Caused these two tests to timeout on iOS: compositing/tiling/tiled-reflection-inwindow.html fast/forms/ios/focus-select-and-switch-tabs.html I don't think EWS caught this because it was considered flaky during the run. current history shows a very clear regression point though History: https://results.webkit.org/?suite=layout-tests&suite=layout-tests&test=editing%2Fpasteboard%2Fdom-paste%2Fdom-paste-requires-user-gesture.html&test=fast%2Fforms%2Fios%2Ffocus-select-and-switch-tabs.html&platform=ios Tracking in: https://bugs.webkit.org/show_bug.cgi?id=234032

Reverted r286596 for reason: broke 2 tests on iOS Committed r286709 (244986@trunk): <https://commits.webkit.org/244986@trunk>

Created attachment 446713 [details] Patch

Created attachment 446723 [details] Patch

Created attachment 446726 [details] Patch

Created attachment 446730 [details] Patch

Comment on attachment 446730 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=446730&action=review > Source/WebCore/platform/graphics/cocoa/WebProcessGraphicsContextGLCocoa.mm:89 > + const bool m_isOpaque; > + const float m_contentsScale; > + std::unique_ptr<IOSurface> m_displayBuffer; Doesn't really matter but I would flip the ordering to avoid padding. > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.h:140 > + // FIXME: This should be removed and m_bufferHandle should be used to ref the buffer once ShareableBitmap::Handle > + // can be encoded multiple times. File a bug to track? > Source/WebKit/Shared/RemoteLayerTree/RemoteLayerBackingStore.mm:117 > + // FIXME: For simplicity this should be moved to the end of display() once the buffer handles can be created once > + // and stored in m_bufferHandle. Reference the same bug. > Source/WebKit/WebProcess/GPU/graphics/cocoa/RemoteGraphicsContextGLProxyCocoa.mm:91 > + const bool m_isOpaque; > + const float m_contentsScale; > + MachSendRight m_displayBuffer; Ditto.

Created attachment 446789 [details] Patch for landing

Tools/Scripts/svn-apply failed to apply attachment 446789 [details] to trunk. Please resolve the conflicts and upload a new patch.

Created attachment 446976 [details] Patch for landing

Committed r286943 (245168@main): <https://commits.webkit.org/245168@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 446976 [details].

Comment on attachment 446976 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=446976&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:33104 > + 7B582DD82716F55B004B92D0 /* (null) in Headers */, What?