Bug 172048

Summary: Add objc version of WK2 UIPageClient hasVideoInPictureInPictureDidChange
Product: WebKit Reporter: Jeremy Jones <jeremyj-wk>
Component: WebKit2Assignee: Jeremy Jones <jeremyj-wk>
Status: NEW ---    
Severity: Normal CC: commit-queue, jlewis3, simon.fraser, thorton, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 172023, 172121    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
simon.fraser: review+, jeremyj-wk: commit-queue-
Patch for landing. none

Description Jeremy Jones 2017-05-12 14:10:19 PDT
Add objc version of WK2 UIPageClient setHasVideoInPictureInPicture
Comment 1 Jeremy Jones 2017-05-12 14:11:48 PDT
rdar://problem/32163054
Comment 2 Jeremy Jones 2017-05-12 14:20:11 PDT
Created attachment 309944 [details]
Patch
Comment 3 Simon Fraser (smfr) 2017-05-12 14:30:47 PDT
Comment on attachment 309944 [details]
Patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:63
> +- (void)_webView:(WKWebView *)webView setHasVideoInPictureInPicture:(BOOL)hasVideoInPictureInPicture WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));

This is a weird signature for a delegate callback. It should be more like:

- (void)_webViewDidGainPictureInPictureVideo:(WKWebView *)webView
- (void)_webViewDidLosePictureInPictureVideo:(WKWebView *)webView

or 
- (void)_webView:(WKWebView *)webView hasVideoInPictureInPictureChangedTo:(BOOL)hasPIP

or something.

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:114
> +        void setHasVideoInPictureInPicture(WebKit::WebPageProxy*, bool) override;

Should be be a "set".
Comment 4 Jeremy Jones 2017-05-12 14:48:55 PDT
Comment on attachment 309944 [details]
Patch

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

>> Source/WebKit2/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:63
>> +- (void)_webView:(WKWebView *)webView setHasVideoInPictureInPicture:(BOOL)hasVideoInPictureInPicture WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> This is a weird signature for a delegate callback. It should be more like:
> 
> - (void)_webViewDidGainPictureInPictureVideo:(WKWebView *)webView
> - (void)_webViewDidLosePictureInPictureVideo:(WKWebView *)webView
> 
> or 
> - (void)_webView:(WKWebView *)webView hasVideoInPictureInPictureChangedTo:(BOOL)hasPIP
> 
> or something.

How about 
-_webView:hasVideoInPictureInPictureDidChange:

>> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:114
>> +        void setHasVideoInPictureInPicture(WebKit::WebPageProxy*, bool) override;
> 
> Should be be a "set".

I don't understand.
Comment 5 Jeremy Jones 2017-05-12 14:58:19 PDT
Created attachment 309949 [details]
Patch
Comment 6 Simon Fraser (smfr) 2017-05-12 15:06:05 PDT
Comment on attachment 309949 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:114
> +        void setHasVideoInPictureInPicture(WebKit::WebPageProxy*, bool) override;

This still doesn't feel the right name for a delegate callback. It should be a "didChange" rather than a "setHas"
Comment 7 Simon Fraser (smfr) 2017-05-12 15:08:19 PDT
(In reply to Jeremy Jones from comment #4)
> >> Source/WebKit2/UIProcess/Cocoa/UIDelegate.h:114
> >> +        void setHasVideoInPictureInPicture(WebKit::WebPageProxy*, bool) override;
> > 
> > Should be be a "set".
> 
> I don't understand.

Sorry, should NOT be a "set".
Comment 8 Jeremy Jones 2017-05-12 15:35:18 PDT
Created attachment 309956 [details]
Patch
Comment 9 Jeremy Jones 2017-05-12 16:14:54 PDT
Created attachment 309964 [details]
Patch for landing.
Comment 10 WebKit Commit Bot 2017-05-12 17:32:27 PDT
Comment on attachment 309964 [details]
Patch for landing.

Clearing flags on attachment: 309964

Committed r216806: <http://trac.webkit.org/changeset/216806>
Comment 11 Simon Fraser (smfr) 2017-05-13 08:20:32 PDT
These API tests are timing out:

https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20%28Tests%29/builds/1503/steps/run-api-tests/logs/stdio

Tests that timed out:
  PictureInPicture.WKPageUIClient
  PictureInPicture.WKUIDelegate