Bug 172048 - Add objc version of WK2 UIPageClient hasVideoInPictureInPictureDidChange
Summary: Add objc version of WK2 UIPageClient hasVideoInPictureInPictureDidChange
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on: 172023 172121
Blocks:
  Show dependency tree / graph
 
Reported: 2017-05-12 14:10 PDT by Jeremy Jones
Modified: 2017-05-15 11:15 PDT (History)
5 users (show)

See Also:

Attachments
Patch (12.30 KB, patch)
2017-05-12 14:20 PDT, Jeremy Jones 		no flags Details | Formatted Diff | Diff
Patch (12.30 KB, patch)
2017-05-12 14:58 PDT, Jeremy Jones 		no flags Details | Formatted Diff | Diff
Patch (18.97 KB, patch)
2017-05-12 15:35 PDT, Jeremy Jones 		simon.fraser: review+
jeremyj-wk: commit-queue-
Details | Formatted Diff | Diff
Patch for landing. (19.18 KB, patch)
2017-05-12 16:14 PDT, Jeremy Jones 		no flags Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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