Cocoa WebGL support UI side compositing Currently WebGL needs IOKit in WP
<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?