Bug 172048 – Add objc version of WK2 UIPageClient hasVideoInPictureInPictureDidChange

• Status: NEW
• Component: WebKit2

[Jeremy Jones]

Add objc version of WK2 UIPageClient setHasVideoInPictureInPicture

[Jeremy Jones]

rdar://problem/32163054

[Jeremy Jones]

Created attachment 309944 [details]
Patch

[Simon Fraser (smfr)]

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".

[Jeremy Jones]

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.

[Jeremy Jones]

Created attachment 309949 [details]
Patch

[Simon Fraser (smfr)]

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"

[Simon Fraser (smfr)]

(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".

[Jeremy Jones]

Created attachment 309956 [details]
Patch

[Jeremy Jones]

Created attachment 309964 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 309964 [details]
Patch for landing.

Clearing flags on attachment: 309964

Committed r216806: <http://trac.webkit.org/changeset/216806>

[Simon Fraser (smfr)]

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