Bug 203723 – Update WebKitTestRunner to support running multiple video fullscreen and Picture-in-Picture tests simultaneously

• Component: Media
• Status: RESOLVED FIXED

[Peng Liu]

In order to support running multiple test cases simultaneously, we need to isolate the UI process from testing Picture-in-Picture feature with WebKitTestRunner.

[Peng Liu]

For bug 203614, we use the solution in the HTMLVideoElement, which essentially only tests the API implementation, but not the picture-in-picture feature implementation in the WebCore.

[Radar WebKit Bug Importer]

<rdar://problem/56806580>

[Peng Liu]

Created attachment 396832 [details]
WIP patch

[Peng Liu]

Created attachment 396833 [details]
Fix GTK and WPE build failures

[Peng Liu]

Created attachment 396834 [details]
Fix GTK and WPE build failures again

[Peng Liu]

Created attachment 396835 [details]
Fix build failures

[Peng Liu]

Created attachment 396836 [details]
Fix build failures again

[Peng Liu]

Created attachment 396839 [details]
Fix wincairo build failure

[Peng Liu]

Created attachment 396985 [details]
Add back the changes on WebKitTestRunner

[Alex Christensen]

Comment on attachment 396985 [details]
Add back the changes on WebKitTestRunner

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

> Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h:60
> +WK_EXPORT void WKBundlePageRequestVideoContentLayer(WKBundlePageRef page, uint64_t contextId);

I don't think we should be exposing all theses uint64_t's.  If we need to, let's introduce a context object that just contains an identifier.

> Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageVideoFullscreenClient.cpp:71
> +    if (m_client.enterFullscreen)
> +        m_client.enterFullscreen(toAPI(page), contextId);
> +    else
> +        page->send(Messages::VideoFullscreenManagerProxy::EnterFullscreen(contextId));

Why can't we aadd UI process SPI that intercepts calls instead of bundle SPI that intercepts calls?

[Jer Noble]

(In reply to Alex Christensen from comment #10)
> Comment on attachment 396985 [details]
> Add back the changes on WebKitTestRunner
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=396985&action=review
> 
> > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h:60
> > +WK_EXPORT void WKBundlePageRequestVideoContentLayer(WKBundlePageRef page, uint64_t contextId);
> 
> I don't think we should be exposing all theses uint64_t's.  If we need to,
> let's introduce a context object that just contains an identifier.
> 
> > Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageVideoFullscreenClient.cpp:71
> > +    if (m_client.enterFullscreen)
> > +        m_client.enterFullscreen(toAPI(page), contextId);
> > +    else
> > +        page->send(Messages::VideoFullscreenManagerProxy::EnterFullscreen(contextId));
> 
> Why can't we aadd UI process SPI that intercepts calls instead of bundle SPI
> that intercepts calls?

Probably because of these details of the bundle implementation:

+    if (injectedBundle.testRunner()->shouldDumpFullScreenCallbacks())
+        injectedBundle.outputText("setupFullscreenWithID()\n");

I don't think that there is a mechanism to talk to the testRunner from the UIProcess (but maybe there should be).

[Jer Noble]

(In reply to Jer Noble from comment #11)
> (In reply to Alex Christensen from comment #10)
> > Comment on attachment 396985 [details]
> > Add back the changes on WebKitTestRunner
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=396985&action=review
> > 
> > > Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundlePage.h:60
> > > +WK_EXPORT void WKBundlePageRequestVideoContentLayer(WKBundlePageRef page, uint64_t contextId);
> > 
> > I don't think we should be exposing all theses uint64_t's.  If we need to,
> > let's introduce a context object that just contains an identifier.
> > 
> > > Source/WebKit/WebProcess/InjectedBundle/InjectedBundlePageVideoFullscreenClient.cpp:71
> > > +    if (m_client.enterFullscreen)
> > > +        m_client.enterFullscreen(toAPI(page), contextId);
> > > +    else
> > > +        page->send(Messages::VideoFullscreenManagerProxy::EnterFullscreen(contextId));
> > 
> > Why can't we aadd UI process SPI that intercepts calls instead of bundle SPI
> > that intercepts calls?
> 
> Probably because of these details of the bundle implementation:
> 
> +    if (injectedBundle.testRunner()->shouldDumpFullScreenCallbacks())
> +        injectedBundle.outputText("setupFullscreenWithID()\n");
> 
> I don't think that there is a mechanism to talk to the testRunner from the
> UIProcess (but maybe there should be).

Oh hey, look at TestInvocation.cpp.

[Peng Liu]

Created attachment 397791 [details]
WIP patch

[Peng Liu]

Created attachment 397792 [details]
WIP patch - fix WPE build failures

[Peng Liu]

Created attachment 397801 [details]
WIP patch - fix iOS build failures

[Peng Liu]

Created attachment 397812 [details]
WIP patch - fix iOS build failures

[Peng Liu]

Created attachment 397852 [details]
WIP patch - fix WinCairo build failures

[Peng Liu]

Created attachment 397927 [details]
WIP patch - a simpler implementation

[Peng Liu]

Created attachment 398265 [details]
Patch

[Peng Liu]

Created attachment 398273 [details]
Fix iOS build failures

[youenn fablet]

Comment on attachment 398273 [details]
Fix iOS build failures

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

> Source/WebKit/ChangeLog:10
> +        messages to the UI process.

Testing IPC messages sending and receiving is nice and will validate code running in UIProcess as well.
Can you explain why the mock is done in WebProcess instead of UIProcess?
Is it the idea that API tests would cover UIProcess code and layout test the WebProcess code?

> Source/WebKit/UIProcess/WebPageProxy.h:2803
> +

Unneeded change

> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:288
> +    if (!m_mockVideoPresentationModeEnabled)

We usually return early, something like:

if (m_mockVideoPresentationModeEnabled) {
    dispatch_async(...);
    return;
}

> LayoutTests/media/video-presentation-mode-expected.txt:20
> +** Entered picture-in-pictur

picture

[Peng Liu]

Comment on attachment 398273 [details]
Fix iOS build failures

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

>> Source/WebKit/ChangeLog:10
>> +        messages to the UI process.
> 
> Testing IPC messages sending and receiving is nice and will validate code running in UIProcess as well.
> Can you explain why the mock is done in WebProcess instead of UIProcess?
> Is it the idea that API tests would cover UIProcess code and layout test the WebProcess code?

Thanks for the review.

Yes, it was the plan to only test the web process code (for WK2) with the layout tests. However, I agree with your point that it is a good idea to test the IPC message related code as well.

Will update the patch to move the mock to the UI process.

>> Source/WebKit/UIProcess/WebPageProxy.h:2803
>> +
> 
> Unneeded change

Will remove it.

>> Source/WebKit/WebProcess/cocoa/VideoFullscreenManager.mm:288
>> +    if (!m_mockVideoPresentationModeEnabled)
> 
> We usually return early, something like:
> 
> if (m_mockVideoPresentationModeEnabled) {
>     dispatch_async(...);
>     return;
> }

Right.
The changes in the VideoFullscreenManager will be removed in the new patch.

>> LayoutTests/media/video-presentation-mode-expected.txt:20
>> +** Entered picture-in-pictur
> 
> picture

Good catch! Will fix it.

[Peng Liu]

Created attachment 398410 [details]
Patch

[youenn fablet]

Comment on attachment 398410 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h:185
> +    bool m_setMockVideoPresentationModeEnabled { false };

s/m_setMockVideoPresentationModeEnabled/m_mockVideoPresentationModeEnabled/

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:-158
> -

Changes not required.

> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:540
>      ensureInterface(contextId).hasVideoChanged(hasVideo);

In a lot of these calls, it seems we could use a mock VideoFullscreenInterfaceContext and leave VideoFullscreenManagerProxy code mostly unchanged.

> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:992
> +    m_page.send(Messages::WebPageProxy::SetMockVideoPresentationModeEnabled(enabled));

This is ok like this but it might be better to use a test runner API that will set the boolean using some C/ObjC SPI.

[Peng Liu]

Comment on attachment 398410 [details]
Patch

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

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.h:185
>> +    bool m_setMockVideoPresentationModeEnabled { false };
> 
> s/m_setMockVideoPresentationModeEnabled/m_mockVideoPresentationModeEnabled/

Oops, will fix it.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:-158
>> -
> 
> Changes not required.

Right, just removed the unnecessary blank lines.

>> Source/WebKit/UIProcess/Cocoa/VideoFullscreenManagerProxy.mm:540
>>      ensureInterface(contextId).hasVideoChanged(hasVideo);
> 
> In a lot of these calls, it seems we could use a mock VideoFullscreenInterfaceContext and leave VideoFullscreenManagerProxy code mostly unchanged.

But VideoFullscreenInterfaceContext is in the Web process. We would like to cover the IPC message sending/receiving in the tests so this patch changes the UI process side.

I guess you mean VideoFullscreenModelContext, which primarily takes care of message sending in the UI process side. So we still need to change VideoFullscreenManagerProxy to intercept the IPC messages from the Web process.

Another possibility is mocking the VideoFullscreenInterface[Mac|IOS] completely, but that will be more complicated.

>> Source/WebKit/WebProcess/WebCoreSupport/WebChromeClient.cpp:992
>> +    m_page.send(Messages::WebPageProxy::SetMockVideoPresentationModeEnabled(enabled));
> 
> This is ok like this but it might be better to use a test runner API that will set the boolean using some C/ObjC SPI.

We have tried to add a C SPI to add an injected bundle client for test purpose (the obsoleted patches) but it seems to be overkill.

[Jer Noble]

Comment on attachment 398410 [details]
Patch

It seems a shame that this will only work on an opt-in basis. Why can't the WKTR just enable this mock PiP behavior unconditionally?

[Peng Liu]

Created attachment 398553 [details]
Prepare for landing (update based on Youenn's comments)

[EWS]

Committed r261203: <https://trac.webkit.org/changeset/261203>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 398553 [details].