Bug 203723 - Update WebKitTestRunner to support running multiple video fullscreen and Picture-in-Picture tests simultaneously
Summary: Update WebKitTestRunner to support running multiple video fullscreen and Pict...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Safari 12
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Peng Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-10-31 21:30 PDT by Peng Liu
Modified: 2020-05-05 15:37 PDT (History)
14 users (show)

See Also:


Attachments
WIP patch (58.68 KB, patch)
2020-04-17 22:08 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix GTK and WPE build failures (58.68 KB, patch)
2020-04-17 22:37 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix GTK and WPE build failures again (58.68 KB, patch)
2020-04-17 22:52 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix build failures (58.92 KB, patch)
2020-04-17 23:15 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix build failures again (58.93 KB, patch)
2020-04-17 23:49 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix wincairo build failure (50.08 KB, patch)
2020-04-18 00:07 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Add back the changes on WebKitTestRunner (59.71 KB, patch)
2020-04-20 10:12 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch (60.25 KB, patch)
2020-04-27 20:18 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch - fix WPE build failures (60.30 KB, patch)
2020-04-27 20:31 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch - fix iOS build failures (60.31 KB, patch)
2020-04-27 21:41 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch - fix iOS build failures (60.32 KB, patch)
2020-04-27 23:25 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch - fix WinCairo build failures (61.00 KB, patch)
2020-04-28 10:16 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
WIP patch - a simpler implementation (29.98 KB, patch)
2020-04-28 23:02 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (35.97 KB, patch)
2020-05-01 17:32 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Fix iOS build failures (36.00 KB, patch)
2020-05-01 19:49 PDT, Peng Liu
no flags Details | Formatted Diff | Diff
Patch (31.56 KB, patch)
2020-05-04 14:04 PDT, Peng Liu
jer.noble: review+
Details | Formatted Diff | Diff
Prepare for landing (update based on Youenn's comments) (31.52 KB, patch)
2020-05-05 14:50 PDT, Peng Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Peng Liu 2019-10-31 21:30:48 PDT
In order to support running multiple test cases simultaneously, we need to isolate the UI process from testing Picture-in-Picture feature with WebKitTestRunner.
Comment 1 Peng Liu 2019-10-31 21:35:21 PDT
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.
Comment 2 Radar WebKit Bug Importer 2019-10-31 21:36:17 PDT
<rdar://problem/56806580>
Comment 3 Peng Liu 2020-04-17 22:08:52 PDT
Created attachment 396832 [details]
WIP patch
Comment 4 Peng Liu 2020-04-17 22:37:06 PDT
Created attachment 396833 [details]
Fix GTK and WPE build failures
Comment 5 Peng Liu 2020-04-17 22:52:23 PDT
Created attachment 396834 [details]
Fix GTK and WPE build failures again
Comment 6 Peng Liu 2020-04-17 23:15:28 PDT
Created attachment 396835 [details]
Fix build failures
Comment 7 Peng Liu 2020-04-17 23:49:32 PDT
Created attachment 396836 [details]
Fix build failures again
Comment 8 Peng Liu 2020-04-18 00:07:46 PDT
Created attachment 396839 [details]
Fix wincairo build failure
Comment 9 Peng Liu 2020-04-20 10:12:50 PDT
Created attachment 396985 [details]
Add back the changes on WebKitTestRunner
Comment 10 Alex Christensen 2020-04-21 09:30:47 PDT
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?
Comment 11 Jer Noble 2020-04-21 09:52:12 PDT
(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).
Comment 12 Jer Noble 2020-04-21 09:55:40 PDT
(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.
Comment 13 Peng Liu 2020-04-27 20:18:02 PDT
Created attachment 397791 [details]
WIP patch
Comment 14 Peng Liu 2020-04-27 20:31:10 PDT
Created attachment 397792 [details]
WIP patch - fix WPE build failures
Comment 15 Peng Liu 2020-04-27 21:41:15 PDT
Created attachment 397801 [details]
WIP patch - fix iOS build failures
Comment 16 Peng Liu 2020-04-27 23:25:41 PDT
Created attachment 397812 [details]
WIP patch - fix iOS build failures
Comment 17 Peng Liu 2020-04-28 10:16:36 PDT
Created attachment 397852 [details]
WIP patch - fix WinCairo build failures
Comment 18 Peng Liu 2020-04-28 23:02:23 PDT
Created attachment 397927 [details]
WIP patch - a simpler implementation
Comment 19 Peng Liu 2020-05-01 17:32:00 PDT
Created attachment 398265 [details]
Patch
Comment 20 Peng Liu 2020-05-01 19:49:01 PDT
Created attachment 398273 [details]
Fix iOS build failures
Comment 21 youenn fablet 2020-05-04 07:18:24 PDT
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
Comment 22 Peng Liu 2020-05-04 13:53:00 PDT
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.
Comment 23 Peng Liu 2020-05-04 14:04:12 PDT
Created attachment 398410 [details]
Patch
Comment 24 youenn fablet 2020-05-05 10:44:36 PDT
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.
Comment 25 Peng Liu 2020-05-05 11:00:00 PDT
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.
Comment 26 Jer Noble 2020-05-05 13:13:01 PDT
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?
Comment 27 Peng Liu 2020-05-05 14:50:23 PDT
Created attachment 398553 [details]
Prepare for landing (update based on Youenn's comments)
Comment 28 EWS 2020-05-05 15:37:40 PDT
Committed r261203: <https://trac.webkit.org/changeset/261203>

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