Bug 172023 – Add UIClient callback for when picture-in-picture is activated.

• Component: WebKit2
• Status: REOPENED

[Jeremy Jones]

Add UIClient callback for when picture-in-picture is activated.

[Jeremy Jones]

Created attachment 309882 [details]
Patch

[Jeremy Jones]

rdar://problem/30882102

[Build Bot]

Comment on attachment 309882 [details]
Patch

Attachment 309882 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3724924

New failing tests:
fast/dom/Window/querySelectorAll-with-pseudo-elements.html

[Build Bot]

Created attachment 309886 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Jeremy Jones]

Created attachment 309913 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 309913 [details]
Patch

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

> Source/WebKit2/UIProcess/API/APIUIClient.h:82
> +    virtual void setPictureInPictureActive(WebKit::WebPageProxy*, bool) { }

I think "active" is too vague here. It could be taken to mean that the PIP window was brought to the front. Maybe setIsShowingPictureInPicture or something?

[Jeremy Jones]

Created attachment 309920 [details]
Patch

[Jeremy Jones]

(In reply to Simon Fraser (smfr) from comment #6)
> Comment on attachment 309913 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=309913&action=review
> 
> > Source/WebKit2/UIProcess/API/APIUIClient.h:82
> > +    virtual void setPictureInPictureActive(WebKit::WebPageProxy*, bool) { }
> 
> I think "active" is too vague here. It could be taken to mean that the PIP
> window was brought to the front. Maybe setIsShowingPictureInPicture or
> something?

OK. setHasVideoInPictureInPicture

[Simon Fraser (smfr)]

Comment on attachment 309920 [details]
Patch

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

> Source/WebKit2/UIProcess/Cocoa/WebVideoFullscreenManagerProxy.mm:373
> +    m_page->uiClient().setHasVideoInPictureInPicture(m_page, videoFullscreenMode & MediaPlayerEnums::VideoFullscreenModePictureInPicture);

Do we always want to call the UI client, even when the PiP mode doesn't change?

> Source/WebKit2/UIProcess/Cocoa/WebVideoFullscreenManagerProxy.mm:502
> +    m_page->uiClient().setHasVideoInPictureInPicture(m_page, mode & MediaPlayerEnums::VideoFullscreenModePictureInPicture);

Ditto.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PictureInPictureDelegate.mm:131
> +    sleep(1); // Wait for PIPAgent to launch, or it won't call -pipDidClose: callback.

Is 1 second always enough, even on loaded bots?

[Jeremy Jones]

Comment on attachment 309920 [details]
Patch

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

>> Source/WebKit2/UIProcess/Cocoa/WebVideoFullscreenManagerProxy.mm:373
>> +    m_page->uiClient().setHasVideoInPictureInPicture(m_page, videoFullscreenMode & MediaPlayerEnums::VideoFullscreenModePictureInPicture);
> 
> Do we always want to call the UI client, even when the PiP mode doesn't change?

With semantics of a setter, it s fine. And that prevents us from having to cache the value here.

>> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/PictureInPictureDelegate.mm:131
>> +    sleep(1); // Wait for PIPAgent to launch, or it won't call -pipDidClose: callback.
> 
> Is 1 second always enough, even on loaded bots?

Unknown. It has worked consistently in my testing.

[WebKit Commit Bot]

Comment on attachment 309920 [details]
Patch

Clearing flags on attachment: 309920

Committed r216791: <http://trac.webkit.org/changeset/216791>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #11)
> Comment on attachment 309920 [details]
> Patch
> 
> Clearing flags on attachment: 309920
> 
> Committed r216791: <http://trac.webkit.org/changeset/216791>

The API test added with this change has been timing out on El Capitan since it was added:

PictureInPicture.WKPageUIClient

https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK1%20%28Tests%29/builds/1610

[WebKit Commit Bot]

Re-opened since this is blocked by bug 172121