Bug 172048

Summary:

Add objc version of WK2 UIPageClient hasVideoInPictureInPictureDidChange

Product:

WebKit

Reporter:

Jeremy Jones <jeremyj-wk>

Component:

WebKit2

Assignee:

Jeremy Jones <jeremyj-wk>

Status:

NEW

Severity:

Normal

CC:

commit-queue, jlewis3, simon.fraser, thorton, webkit-bug-importer

Priority:

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

Jeremy Jones

Reported 2017-05-12 14:10:19 PDT

Add objc version of WK2 UIPageClient setHasVideoInPictureInPicture

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 attachment proposed patch, testcase, etc.

Jeremy Jones

Comment 1 2017-05-12 14:11:48 PDT

rdar://problem/32163054

Jeremy Jones

Comment 2 2017-05-12 14:20:11 PDT

Created attachment 309944 [details] Patch

Simon Fraser (smfr)

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

Jeremy Jones

Comment 4 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.

Jeremy Jones

Comment 5 2017-05-12 14:58:19 PDT

Created attachment 309949 [details] Patch

Simon Fraser (smfr)

Comment 6 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"

Simon Fraser (smfr)

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

Jeremy Jones

Comment 8 2017-05-12 15:35:18 PDT

Created attachment 309956 [details] Patch

Jeremy Jones

Comment 9 2017-05-12 16:14:54 PDT

Created attachment 309964 [details] Patch for landing.

WebKit Commit Bot

Comment 10 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>

Simon Fraser (smfr)

Comment 11 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

Note You need to log in before you can comment on or make changes to this bug.