Bug 203060 – WKWebView specific bug: WKWebView as the contents of a SceneKit/ARKit node doesn't work (UIWebView works)

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Dean Jackson]

WKWebView specific bug: WKWebView as the contents of a SceneKit/ARKit node doesn't work (UIWebView works)

[Dean Jackson]

rdar://47481972

[Dean Jackson]

Created attachment 381131 [details]
Patch

[Anders Carlsson]

Shouldn't this be SPI on WKWebView instead?

[Tim Horton]

Comment on attachment 381131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381131&action=review

> Source/WebKit/ChangeLog:19
> +        SceneKit will still need to adopt this API for the WKWebView to show
> +        up in the rendering.

Kinda sad that it's a pref that needs adopting instead of magic, and not sure that that is actually OK for them?

> Source/WebKit/UIProcess/WebPageProxy.cpp:1970
> +    // If we've swapped to using mapped IOSurfaces, then we need to tell

I think this whole thing should get tucked into RemoteLayerTreeDrawingAreaProxy. Maybe even just add a generic "preferencesChanged" to DrawingAreaProxy and do this in RLTDAProxy's implementation.
WebPageProxy doesn't need to know about crazy things like this.

[Tim Horton]

(In reply to Anders Carlsson from comment #3)
> Shouldn't this be SPI on WKWebView instead?

Also a very reasonable thought. Still would like a version that doesn't need any SPI at all though.

[Dean Jackson]

(In reply to Anders Carlsson from comment #3)
> Shouldn't this be SPI on WKWebView instead?

I don't know. I think this way would allow us to set the preference as we create the view.

But honestly I don't mind.

[Simon Fraser (smfr)]

Comment on attachment 381131 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=381131&action=review

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm:92
> +    auto layerContentsType = (m_drawingArea->hasDebugIndicator() || m_drawingArea->page().preferences().useIOSurfaceBackingStoreForRemoteHosting()) ? RemoteLayerBackingStore::LayerContentsType::IOSurface : RemoteLayerBackingStore::LayerContentsType::CAMachPort;

Would be cleaner with m_drawingArea->hasDebugIndicator() || m_drawingArea->page().preferences().useIOSurfaceBackingStoreForRemoteHosting() in a lambda.

[Dean Jackson]

Comment on attachment 381131 [details]
Patch

Holding off on this for the moment in the hope that CoreAnimation can fix it on their side, which would avoid us having to futz with our layers.

[Radar WebKit Bug Importer]

<rdar://problem/82899923>

[Tim Horton]

Created attachment 437688 [details]
Patch

[Tim Horton]

Created attachment 437689 [details]
Patch

[Dean Jackson]

Comment on attachment 437688 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437688&action=review

> Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm:78
> +    if (m_drawingArea->page().windowKind() == WindowKind::SceneKitSnapshotting)

Would it be better to have a more abstract name for this kind of snapshotting, and only reference SceneKit when you're looking at the class type?

> Source/WebKit/UIProcess/WindowKind.h:33
> +    SceneKitSnapshotting,

e.g. here.

[Tim Horton]

(In reply to Dean Jackson from comment #12)
> Comment on attachment 437688 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437688&action=review
> 
> > Source/WebKit/UIProcess/RemoteLayerTree/RemoteLayerTreeHost.mm:78
> > +    if (m_drawingArea->page().windowKind() == WindowKind::SceneKitSnapshotting)
> 
> Would it be better to have a more abstract name for this kind of
> snapshotting, and only reference SceneKit when you're looking at the class
> type?

Maybe! I should look at what they're actually doing and see if there's a way to name it more generically.

[Tim Horton]

Created attachment 437690 [details]
Patch

[Tim Horton]

Created attachment 437691 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 437691 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437691&action=review

> Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:201
> +    if ([window isKindOfClass:NSClassFromString(@"_SCNSnapshotWindow")])

Yucky

> Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:202
> +        return WindowKind::InProcessSnapshotting;

Not sure why SceneKit implies snapshotting.

[Tim Horton]

(In reply to Simon Fraser (smfr) from comment #16)
> Comment on attachment 437691 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437691&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:201
> > +    if ([window isKindOfClass:NSClassFromString(@"_SCNSnapshotWindow")])
> 
> Yucky

Mmmhmm.

> > Source/WebKit/UIProcess/Cocoa/PageClientImplCocoa.mm:202
> > +        return WindowKind::InProcessSnapshotting;
> 
> Not sure why SceneKit implies snapshotting.

When you set a view as the contents of a SceneKit material, they adopt your view tree and use a custom CARenderer to flatten it into a texture. Maybe snapshotting isn't the right word because it's a fresh snapshot every frame? 🤷‍♂️

[EWS]

Committed r282189 (241476@main): <https://commits.webkit.org/241476@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437691 [details].