Bug 61068

Summary: MediaStream API: add getUserMedia mock result manipulation to the LayoutTestController
Product: WebKit Reporter: Leandro GraciĆ” Gil <leandrogracia>
Component: Tools / TestsAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED WONTFIX    
Severity: Normal CC: abarth, danilo.eu, dglazkov, donggwan.kim, fishd, gustavo, harald, morrita, rakuco, s.choi, sh9.choi, tkent, webkit.review.bot, wjjeon, xan.lopez, yujie.mao
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 61069    
Bug Blocks: 56459, 56587    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch gustavo: commit-queue-

Description Leandro GraciĆ” Gil 2011-05-18 09:31:58 PDT
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.
Comment 1 Tommy Widenflycht 2012-02-16 06:56:27 PST
Created attachment 127372 [details]
Patch
Comment 2 WebKit Review Bot 2012-02-16 07:12:19 PST
Comment on attachment 127372 [details]
Patch

Attachment 127372 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11542004
Comment 3 Gyuyoung Kim 2012-02-16 07:19:56 PST
Comment on attachment 127372 [details]
Patch

Attachment 127372 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11536198
Comment 4 Tommy Widenflycht 2012-02-16 08:00:55 PST
Created attachment 127381 [details]
Patch
Comment 5 WebKit Review Bot 2012-02-16 08:43:38 PST
Comment on attachment 127381 [details]
Patch

Attachment 127381 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11536215
Comment 6 Tommy Widenflycht 2012-02-17 01:13:42 PST
Created attachment 127548 [details]
Patch
Comment 7 WebKit Review Bot 2012-02-17 03:53:50 PST
Comment on attachment 127548 [details]
Patch

Attachment 127548 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11538453
Comment 8 Tommy Widenflycht 2012-02-17 04:18:56 PST
Hmm, for some reason the chromium build bot is stuck on an quite old chromium revision...
Comment 9 Tommy Widenflycht 2012-02-17 05:27:12 PST
Created attachment 127575 [details]
Patch
Comment 10 WebKit Review Bot 2012-02-17 09:43:07 PST
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
Comment 11 Adam Barth 2012-02-17 10:50:40 PST
> 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.
Comment 12 Adam Barth 2012-02-17 10:55:16 PST
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.
Comment 13 Adam Barth 2012-02-17 10:55:38 PST
+fishd for WebKit API review.
Comment 14 Tommy Widenflycht 2012-02-20 02:03:15 PST
Created attachment 127785 [details]
Patch
Comment 15 Tommy Widenflycht 2012-02-20 02:04:23 PST
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.
Comment 16 Tommy Widenflycht 2012-02-20 02:05:38 PST
(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.
Comment 17 Adam Barth 2012-02-20 19:46:15 PST
I think the ball is in fishd's court here.
Comment 18 Adam Barth 2012-02-22 17:56:43 PST
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.
Comment 19 WebKit Review Bot 2012-02-22 18:21:06 PST
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
Comment 20 Tommy Widenflycht 2012-02-23 01:53:18 PST
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.
Comment 21 Tommy Widenflycht 2012-02-23 01:58:40 PST
Created attachment 128437 [details]
Patch
Comment 22 WebKit Review Bot 2012-02-23 14:56:41 PST
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
Comment 23 Tommy Widenflycht 2012-02-27 04:29:26 PST
Created attachment 129012 [details]
Patch
Comment 24 WebKit Review Bot 2012-02-27 04:33:28 PST
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API.
Comment 25 Tommy Widenflycht 2012-02-27 04:34:34 PST
Just rebased, no actual code changes.
Comment 26 WebKit Review Bot 2012-02-27 05:10:14 PST
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
Comment 27 Darin Fisher (:fishd, Google) 2012-02-27 08:41:52 PST
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?
Comment 28 Tommy Widenflycht 2012-02-27 10:20:11 PST
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.
Comment 29 Darin Fisher (:fishd, Google) 2012-02-27 10:23:58 PST
(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.
Comment 30 Tommy Widenflycht 2012-02-27 10:26:43 PST
The code will be linked away for everything except DRT though but if we don't have to compile it we save time.
Comment 31 Kent Tamura 2012-02-27 17:28:54 PST
Can we move the mock to WebCore/testing and enable it by window.internals?
Comment 32 Tommy Widenflycht 2012-02-28 01:33:27 PST
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.
Comment 33 Darin Fisher (:fishd, Google) 2012-02-28 11:14:25 PST
(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!
Comment 34 Darin Fisher (:fishd, Google) 2012-02-28 11:15:02 PST
Comment on attachment 129012 [details]
Patch

marking R- based on decision to move mocks out.
Comment 35 Tommy Widenflycht 2012-02-29 07:12:13 PST
Created attachment 129440 [details]
Patch
Comment 36 Tommy Widenflycht 2012-02-29 07:14:17 PST
Hi all reviewers,

I gave the move to WebCore/testing a try. What do you think?
Comment 37 Adam Barth 2012-02-29 10:27:52 PST
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?
Comment 38 Adam Barth 2012-02-29 10:28:26 PST
+morrita,dglazkov as this relates to WebCore::Internals.
Comment 39 Gustavo Noronha (kov) 2012-02-29 16:45:48 PST
Comment on attachment 129440 [details]
Patch

Attachment 129440 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/11776066
Comment 40 Kent Tamura 2012-02-29 16:58:45 PST
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?