Summary: | MediaStream API: add getUserMedia mock result manipulation to the LayoutTestController | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Leandro GraciĆ” Gil <leandrogracia> | ||||||||||||||||||
Component: | Tools / Tests | Assignee: | 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
Leandro GraciĆ” Gil
2011-05-18 09:31:58 PDT
Created attachment 127372 [details]
Patch
Comment on attachment 127372 [details] Patch Attachment 127372 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11542004 Comment on attachment 127372 [details] Patch Attachment 127372 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/11536198 Created attachment 127381 [details]
Patch
Comment on attachment 127381 [details] Patch Attachment 127381 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11536215 Created attachment 127548 [details]
Patch
Comment on attachment 127548 [details] Patch Attachment 127548 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11538453 Hmm, for some reason the chromium build bot is stuck on an quite old chromium revision... Created attachment 127575 [details]
Patch
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 > 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 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. +fishd for WebKit API review. Created attachment 127785 [details]
Patch
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. (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. I think the ball is in fishd's court here. 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 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 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. Created attachment 128437 [details]
Patch
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 Created attachment 129012 [details]
Patch
Please wait for approval from fishd@chromium.org before submitting because this patch contains changes to the Chromium public API. Just rebased, no actual code changes. 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 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? 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. (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. The code will be linked away for everything except DRT though but if we don't have to compile it we save time. Can we move the mock to WebCore/testing and enable it by window.internals? 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. (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 on attachment 129012 [details]
Patch
marking R- based on decision to move mocks out.
Created attachment 129440 [details]
Patch
Hi all reviewers, I gave the move to WebCore/testing a try. What do you think? 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? +morrita,dglazkov as this relates to WebCore::Internals. Comment on attachment 129440 [details] Patch Attachment 129440 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11776066 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? |