Bug 227443 – [Model] [iOS] Add support for displaying <model> in fullscreen

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

[Antoine Quint]

[Model] [iOS] Add support for displaying <model> in fullscreen

[Radar WebKit Bug Importer]

<rdar://problem/79859937>

[Antoine Quint]

Created attachment 432385 [details]
Patch

[Dean Jackson]

Comment on attachment 432385 [details]
Patch

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

> Source/WebCore/ChangeLog:10
> +        Expose a new enterFullscreen() method on HTMLModelElement allowing to enter a fullscreen AR experience
> +        where the resource may be displayed in the user's environment.

Why wouldn't this just be the normal requestFullscreen on the element?

> Source/WebCore/Modules/model-element/HTMLModelElement.idl:35
> +    undefined enterFullscreen();

This should use the normal DOM API, not something new.

[Antoine Quint]

(In reply to Dean Jackson from comment #3)
> Comment on attachment 432385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432385&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Expose a new enterFullscreen() method on HTMLModelElement allowing to enter a fullscreen AR experience
> > +        where the resource may be displayed in the user's environment.
> 
> Why wouldn't this just be the normal requestFullscreen on the element?
> 
> > Source/WebCore/Modules/model-element/HTMLModelElement.idl:35
> > +    undefined enterFullscreen();
> 
> This should use the normal DOM API, not something new.

I think I should clarify the intent of this patch. This is about exposing the fullscreen capabilities of the ASVInlinePreview SPI, and the easiest path forward to me was to expose a method similar to what we have for <video> on iOS and macOS when we want to provide a built-in fullscreen experience, which is different from the media element is displayed fullscreen using the Element.requestFullscreen() API.

While we may be land on using requestFullscreen() as the way to make a <model> element enter the AR fullscreen experience, it may be in the form of additional parameters to that method or a whole new method like what I propose in this patch.

At any rate, what I suggest we do now is to use a new, dedicated method that is just about providing the built-in AR fullscreen experience and think through how it should fit with HTML fullscreen features as we iterate on this experiment.

[Tim Horton]

Comment on attachment 432385 [details]
Patch

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

>>> Source/WebCore/ChangeLog:10
>>> +        where the resource may be displayed in the user's environment.
>> 
>> Why wouldn't this just be the normal requestFullscreen on the element?
> 
> I think I should clarify the intent of this patch. This is about exposing the fullscreen capabilities of the ASVInlinePreview SPI, and the easiest path forward to me was to expose a method similar to what we have for <video> on iOS and macOS when we want to provide a built-in fullscreen experience, which is different from the media element is displayed fullscreen using the Element.requestFullscreen() API.
> 
> While we may be land on using requestFullscreen() as the way to make a <model> element enter the AR fullscreen experience, it may be in the form of additional parameters to that method or a whole new method like what I propose in this patch.
> 
> At any rate, what I suggest we do now is to use a new, dedicated method that is just about providing the built-in AR fullscreen experience and think through how it should fit with HTML fullscreen features as we iterate on this experiment.

I'm going to defer to Dean and Sam on this part.

> Source/WebCore/loader/EmptyClients.cpp:1101
> +};

Stray semicolon

> Source/WebCore/loader/EmptyFrameLoaderClient.h:207
> +    void takeModelElementFullscreen(WebCore::GraphicsLayer::PlatformLayerID) const final;

How did this end up on FrameLoaderClient instead of ChromeClient??

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:65
> +    auto *presentingViewController = m_webPageProxy.uiClient().presentingViewController();

Is this the right one to present on? (I can never remember, we have a bit of a mess around the parent VCs). I thought it had "for full screen presentation" or something like that in its name.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:91
> +            [preview observeDismissFullscreenWithCompletionHandler:^(CAFenceHandle *dismissFenceHandle, NSDictionary *payload, NSError *error) {

I wonder if we should be using CAFenceHandle in other places where we currently pass fence ports around as MachSendRights.

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:94
> +                        LOG(ModelElement, "Unable to get fence handle when dismissing fullscreen instance: %@", [error localizedDescription]);

Error message oddly clumps two failure modes together

> Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:100
> +                    [CATransaction setCompletionBlock:^{

I think that setting a completion block on the implicit transaction is a bad idea (because there's only one per-transaction per-thread, and you don't own the transaction), you maybe want an explicit transaction here?

[Antoine Quint]

(In reply to Tim Horton from comment #5)
> Comment on attachment 432385 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432385&action=review
> 
> > Source/WebCore/loader/EmptyFrameLoaderClient.h:207
> > +    void takeModelElementFullscreen(WebCore::GraphicsLayer::PlatformLayerID) const final;
> 
> How did this end up on FrameLoaderClient instead of ChromeClient??

Ignorance, but now I know what to do. 

> > Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:65
> > +    auto *presentingViewController = m_webPageProxy.uiClient().presentingViewController();
> 
> Is this the right one to present on? (I can never remember, we have a bit of
> a mess around the parent VCs). I thought it had "for full screen
> presentation" or something like that in its name.

So I used that to follow what SystemPreviewControllerCocoa uses. It's also what VideoFullscreenModelContext::presentingViewController() returns, and looking at UIDelegate.h, this looks like the only UIViewController available.

> > Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:94
> > +                        LOG(ModelElement, "Unable to get fence handle when dismissing fullscreen instance: %@", [error localizedDescription]);
> 
> Error message oddly clumps two failure modes together
> 
> > Source/WebKit/UIProcess/Cocoa/ModelElementControllerCocoa.mm:100
> > +                    [CATransaction setCompletionBlock:^{
> 
> I think that setting a completion block on the implicit transaction is a bad
> idea (because there's only one per-transaction per-thread, and you don't own
> the transaction), you maybe want an explicit transaction here?

Will do. Thank for the comments, will file a patch with those modifications shortly. Then hopefully Sam and/or Dean can comment on the IDL side of things.

[Antoine Quint]

Created attachment 432577 [details]
Patch

[Sam Weinig]

Comment on attachment 432577 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.h:54
> +    RenderModel* renderer() const;

I don't particularly like this idiom, where this is implemented in RenderModel.h. That is just really hard to find. Is it needed?

> Source/WebCore/rendering/RenderModel.h:57
> +    return downcast<RenderModel>(HTMLElement::renderer());

What happens if a <model> has a different kind of renderer? like via -webkit-appearance or something like that?

[Simon Fraser (smfr)]

Comment on attachment 432577 [details]
Patch

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

> Source/WebCore/Modules/model-element/HTMLModelElement.idl:35
> +    undefined enterFullscreen();

Why is this needed, rather than using HTML fullscreen API?

[Antoine Quint]

(In reply to Simon Fraser (smfr) from comment #9)
> Comment on attachment 432577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432577&action=review
> 
> > Source/WebCore/Modules/model-element/HTMLModelElement.idl:35
> > +    undefined enterFullscreen();
> 
> Why is this needed, rather than using HTML fullscreen API?

See https://bugs.webkit.org/show_bug.cgi?id=227443#c4.

[Antoine Quint]

(In reply to Sam Weinig from comment #8)
> Comment on attachment 432577 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=432577&action=review
> 
> > Source/WebCore/Modules/model-element/HTMLModelElement.h:54
> > +    RenderModel* renderer() const;
> 
> I don't particularly like this idiom, where this is implemented in
> RenderModel.h. That is just really hard to find. Is it needed?

I thought that was the established idiom, but if not, let's implement this method inline within HTMLModelElement.h.
 
> > Source/WebCore/rendering/RenderModel.h:57
> > +    return downcast<RenderModel>(HTMLElement::renderer());
> 
> What happens if a <model> has a different kind of renderer? like via
> -webkit-appearance or something like that?

I hadn't considered this. I suppose we could return nullptr. Can you think of something better?

[Antoine Quint]

Talking some more with Sam, we can do away with HTMLModelElement::renderer() and simply downcast to the appropriate subclass as needed.

[Antoine Quint]

Committed r279420 (239281@main): <https://commits.webkit.org/239281@main>