Bug 61068 – MediaStream API: add getUserMedia mock result manipulation to the LayoutTestController

• Status: RESOLVED
• Resolution: WONTFIX
• Component: Tools / Tests

[Leandro Graciá Gil]

Add a new method to the different ports of the LayoutTestController to set the available mock devices in the next navigator.getUserMedia call. This will be used through many layout tests.

[Tommy Widenflycht]

Created attachment 127372 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127372 [details]
Patch

Attachment 127372 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11542004

[Gyuyoung Kim]

Comment on attachment 127372 [details]
Patch

Attachment 127372 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11536198

[Tommy Widenflycht]

Created attachment 127381 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127381 [details]
Patch

Attachment 127381 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11536215

[Tommy Widenflycht]

Created attachment 127548 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127548 [details]
Patch

Attachment 127548 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11538453

[Tommy Widenflycht]

Hmm, for some reason the chromium build bot is stuck on an quite old chromium revision...

[Tommy Widenflycht]

Created attachment 127575 [details]
Patch

[WebKit Review Bot]

Comment on attachment 127575 [details]
Patch

Attachment 127575 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11540498

New failing tests:
platform/chromium/media/video-capture-preview.html

[Adam Barth]

> Hmm, for some reason the chromium build bot is stuck on an quite old chromium revision...

The revision is controlled by this file:
http://trac.webkit.org/browser/trunk/Source/WebKit/chromium/DEPS

If you find that it's older than you'd like, please update it to a newer revision.

[Adam Barth]

Comment on attachment 127575 [details]
Patch

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

This generally looks good.  We'll need fishd to review the API changes.  Also, it looks like you're failing one test.

> Source/WebKit/chromium/public/WebUserMediaClientMock.h:57
> +    WEBKIT_EXPORT virtual void requestUserMedia(const WebUserMediaRequest&, const WebVector<WebMediaStreamSource>& audioSources, const WebVector<WebMediaStreamSource>& videoSources);
> +    WEBKIT_EXPORT virtual void cancelUserMediaRequest(const WebUserMediaRequest&);

You don't need to export virtual functions.  They're called indirectly anyway.

[Adam Barth]

+fishd for WebKit API review.

[Tommy Widenflycht]

Created attachment 127785 [details]
Patch

[Tommy Widenflycht]

Comment on attachment 127575 [details]
Patch

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

>> Source/WebKit/chromium/public/WebUserMediaClientMock.h:57
>> +    WEBKIT_EXPORT virtual void cancelUserMediaRequest(const WebUserMediaRequest&);
> 
> You don't need to export virtual functions.  They're called indirectly anyway.

Fixed.

[Tommy Widenflycht]

(In reply to comment #12)
> (From update of attachment 127575 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127575&action=review
> 
> Also, it looks like you're failing one test.
> 

Sorry about that, I forgot to add that changed test to the patch.

[Adam Barth]

I think the ball is in fishd's court here.

[Adam Barth]

Comment on attachment 127785 [details]
Patch

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

One naming nit below, but I don't think its worth re-spinning this patch.  Please feel free to address it in a follow-up patch if you think it's worth changing.

> Source/WebKit/chromium/src/WebUserMediaClientMock.cpp:32
> +#if ENABLE(MEDIA_STREAM)

Missing a blank line above here.

> Source/WebKit/chromium/src/WebUserMediaClientMock.cpp:64
> +    m_private->setGetUserMediaResult(allowAudio, allowVideo);

I wonder if this would read better as "setUserMediaResult".

> Tools/DumpRenderTree/chromium/LayoutTestController.cpp:187
> +    bindMethod("setMockGetUserMediaResult", &LayoutTestController::setMockGetUserMediaResult);

This would then be setMockUserMediaResult, which would make sense.

[WebKit Review Bot]

Comment on attachment 127785 [details]
Patch

Rejecting attachment 127785 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
edia-arguments.html
patching file LayoutTests/fast/mediastream/getusermedia-not-allowed-expected.txt
patching file LayoutTests/fast/mediastream/getusermedia-not-allowed.html
patching file LayoutTests/fast/mediastream/script-tests/getusermedia-not-allowed.js
patching file LayoutTests/platform/chromium/media/video-capture-preview.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11599057

[Tommy Widenflycht]

Comment on attachment 127785 [details]
Patch

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

>> Source/WebKit/chromium/src/WebUserMediaClientMock.cpp:32
>> +#if ENABLE(MEDIA_STREAM)
> 
> Missing a blank line above here.

Fixed.

>> Source/WebKit/chromium/src/WebUserMediaClientMock.cpp:64
>> +    m_private->setGetUserMediaResult(allowAudio, allowVideo);
> 
> I wonder if this would read better as "setUserMediaResult".

Hmm, maybe. The function this sets the expectations for is "getUserMedia" and thus setGetUserMediaResult feels more logical. However I don't feel very strongly about it.

[Tommy Widenflycht]

Created attachment 128437 [details]
Patch

[WebKit Review Bot]

Comment on attachment 128437 [details]
Patch

Rejecting attachment 128437 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
media-not-allowed-expected.txt
patching file LayoutTests/fast/mediastream/getusermedia-not-allowed.html
patching file LayoutTests/fast/mediastream/script-tests/Attributes.js
patching file LayoutTests/fast/mediastream/script-tests/getusermedia-not-allowed.js
patching file LayoutTests/platform/chromium/media/video-capture-preview.html

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Adam Barth']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11603437

[Tommy Widenflycht]

Created attachment 129012 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Tommy Widenflycht]

Just rebased, no actual code changes.

[WebKit Review Bot]

Comment on attachment 129012 [details]
Patch

Attachment 129012 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11633895

New failing tests:
inspector/protocol/console-agent.html

[Darin Fisher (:fishd, Google)]

Comment on attachment 129012 [details]
Patch

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

> Source/WebKit/chromium/public/WebUserMediaClientMock.h:50
> +class WebUserMediaClientMock : public WebUserMediaClient {

Why do we have mocks as part of the API?  Why isn't this class just part of DumpRenderTree?

[Tommy Widenflycht]

I wanted to place the mock next to the code it is coming from, and also there is other code doing the same. The idea to place XxxMock.* in DRT/ main directory and WebXxxMock.* in DRT/chromium just never struck me.

I'll give it a shot and report back.

[Darin Fisher (:fishd, Google)]

(In reply to comment #28)
> I wanted to place the mock next to the code it is coming from, and also there is other code doing the same. The idea to place XxxMock.* in DRT/ main directory and WebXxxMock.* in DRT/chromium just never struck me.
> 
> I'll give it a shot and report back.

We have some bad code in the codebase.  There doesn't seem to be any point to shipping code for mocks in Chrome.  We only need to builds mocks into DRT.

What we need is the correct interfaces to support dependency injection so that DRT can supply mock implementations of various layers.

[Tommy Widenflycht]

The code will be linked away for everything except DRT though but if we don't have to compile it we save time.

[Kent Tamura]

Can we move the mock to WebCore/testing and enable it by window.internals?

[Tommy Widenflycht]

After a chat session with Darin yesterday we agreed to make the mock code DRT/chromium only for now and take the refactoring when another port have implemented WebRTC.

[Darin Fisher (:fishd, Google)]

(In reply to comment #32)
> After a chat session with Darin yesterday we agreed to make the mock code DRT/chromium only for now and take the refactoring when another port have implemented WebRTC.

To be clear, I'm also OK with tkent's suggestion!

[Darin Fisher (:fishd, Google)]

Comment on attachment 129012 [details]
Patch

marking R- based on decision to move mocks out.

[Tommy Widenflycht]

Created attachment 129440 [details]
Patch

[Tommy Widenflycht]

Hi all reviewers,

I gave the move to WebCore/testing a try. What do you think?

[Adam Barth]

Comment on attachment 129440 [details]
Patch

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

> Source/WebCore/testing/InternalSettings.idl:44
> +        void setMockGetUserMediaResult(in boolean audio, in boolean video) raises(DOMException);

I think previously this object has been used to manipulate WebCore::Settings, no mocks.  I wonder if we should create a new IDL for manipulating mocks?

[Adam Barth]

+morrita,dglazkov as this relates to WebCore::Internals.

[Gustavo Noronha (kov)]

Comment on attachment 129440 [details]
Patch

Attachment 129440 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11776066

[Kent Tamura]

Comment on attachment 129440 [details]
Patch

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

> Source/WebKit/chromium/src/WebUserMediaClientMock.cpp:1
> +/*

This isn't used anymore?

> Tools/DumpRenderTree/chromium/WebViewHost.h:254
> +    WebKit::WebUserMediaClientMock* userMediaClientMock();

This isn't used anymore?