Bug 178089 – Call setFullscreenClient on iOS.

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Call setFullscreenClient on all platforms, not just Mac.

[Jeremy Jones]

Created attachment 323191 [details]
Patch

[Tim Horton]

Comment on attachment 323191 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:-1308
> -    m_page->setFullscreenClient(std::make_unique<WebKit::FullscreenClient>(view));

How does removing this not break WKView??

[Jeremy Jones]

Created attachment 323623 [details]
Patch

[Jeremy Jones]

Created attachment 323967 [details]
Patch

[Jeremy Jones]

Created attachment 324034 [details]
Patch

[Jeremy Jones]

Created attachment 324874 [details]
Patch

[Jeremy Jones]

(In reply to Tim Horton from comment #2)
> Comment on attachment 323191 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=323191&action=review
> 
> > Source/WebKit/UIProcess/Cocoa/WebViewImpl.mm:-1308
> > -    m_page->setFullscreenClient(std::make_unique<WebKit::FullscreenClient>(view));
> 
> How does removing this not break WKView??

I put that back in and just added it for iOS in WKContentView.

[Simon Fraser (smfr)]

Comment on attachment 324874 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKContentView.mm:205
> +    _page->setFullscreenClient(std::make_unique<WebKit::FullscreenClient>(_webView));

This API seems a bit weird. Normally you don't pass ownership of a client to the thing that it's a client of. You hold ownership, and the client relationship is weak.

[Jeremy Jones]

(In reply to Simon Fraser (smfr) from comment #8)
> Comment on attachment 324874 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=324874&action=review
> 
> > Source/WebKit/UIProcess/ios/WKContentView.mm:205
> > +    _page->setFullscreenClient(std::make_unique<WebKit::FullscreenClient>(_webView));
> 
> This API seems a bit weird. Normally you don't pass ownership of a client to
> the thing that it's a client of. You hold ownership, and the client
> relationship is weak.

A regular expression search for '_page->set[A-Za-z]*Client\(' shows that we always do this. I've filed https://bugs.webkit.org/show_bug.cgi?id=178893 to investigate this pattern.

[WebKit Commit Bot]

Comment on attachment 324874 [details]
Patch

Clearing flags on attachment: 324874

Committed r224063: <https://trac.webkit.org/changeset/224063>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/35568855>