Bug 178089

Summary: Call setFullscreenClient on iOS.
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: WebKit2Assignee: Jeremy Jones <jeremyj-wk>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Jeremy Jones 2017-10-09 11:12:22 PDT
Call setFullscreenClient on all platforms, not just Mac.
Comment 1 Jeremy Jones 2017-10-09 11:16:22 PDT
Created attachment 323191 [details]
Patch
Comment 2 Tim Horton 2017-10-09 16:37:01 PDT
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??
Comment 3 Jeremy Jones 2017-10-12 19:53:29 PDT
Created attachment 323623 [details]
Patch
Comment 4 Jeremy Jones 2017-10-16 18:35:33 PDT
Created attachment 323967 [details]
Patch
Comment 5 Jeremy Jones 2017-10-17 11:43:49 PDT
Created attachment 324034 [details]
Patch
Comment 6 Jeremy Jones 2017-10-25 13:09:34 PDT
Created attachment 324874 [details]
Patch
Comment 7 Jeremy Jones 2017-10-25 13:10:02 PDT
(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.
Comment 8 Simon Fraser (smfr) 2017-10-26 12:55:34 PDT
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.
Comment 9 Jeremy Jones 2017-10-26 16:26:40 PDT
(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.
Comment 10 WebKit Commit Bot 2017-10-26 16:30:03 PDT
Comment on attachment 324874 [details]
Patch

Clearing flags on attachment: 324874

Committed r224063: <https://trac.webkit.org/changeset/224063>
Comment 11 WebKit Commit Bot 2017-10-26 16:30:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-11-15 13:08:22 PST
<rdar://problem/35568855>