Summary: | Allow WebUserMediaClientMock to be used by client | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | wjia | ||||||||||||||||||
Component: | Tools / Tests | Assignee: | wjia | ||||||||||||||||||
Status: | RESOLVED INVALID | ||||||||||||||||||||
Severity: | Normal | CC: | abarth, dglazkov, dpranke, eric, fishd, jamesr, jochen, levin, leviw, peter+ews, tkent+wkapi, webkit.review.bot | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Bug Depends on: | 113678 | ||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||
Attachments: |
|
Description
wjia
2013-03-29 19:29:38 PDT
Created attachment 195861 [details]
Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI. This is pre-requisite for https://codereview.chromium.org/13247006/ which tries to enable media stream layout test with conent_shell. Comment on attachment 195861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195861&action=review > Tools/DumpRenderTree/chromium/TestRunner/public/WebUserMediaClientMock.h:34 > +#include "TestCommon.h" Is TestCommon a public header? Do you mean WebTestCommon? (In reply to comment #4) > (From update of attachment 195861 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=195861&action=review > > > Tools/DumpRenderTree/chromium/TestRunner/public/WebUserMediaClientMock.h:34 > > +#include "TestCommon.h" > > Is TestCommon a public header? Do you mean WebTestCommon? TestCommon.h is in DumpRenderTree/chromium/TestRunner/src. I just moved WebUserMediaClientMock.h from src/ to public/ in order to add WEBKIT_EXPORT. It seems there could be some file re-shuffling needed if file in public/ can't depend on files in src/. But I am not sure if that's a requirement or not. Comment on attachment 195861 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195861&action=review > Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:105 > + '../chromium/TestRunner/src', Public cannot depend on SRC. Created attachment 195870 [details]
Patch
The 2nd patch moves TestCommon.h from src/ into public/ and public/ doesn't depend on src/ now. Comment on attachment 195870 [details]
Patch
There's no reason to move TestCommon to the public API.
(In reply to comment #9) > (From update of attachment 195870 [details]) > There's no reason to move TestCommon to the public API. Since WebUserMediaClientMock.h uses TestCommon.h, is it ok to just expand content of TestCommon.h in WebUserMediaClientMock.h if TestCommon.h is not allowed to move to public? Or do you have other suggestions? Created attachment 195875 [details]
Patch
The functions don't appear to be used. If they are used, you'll need to refactor the code. Comment on attachment 195875 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195875&action=review > Tools/DumpRenderTree/chromium/TestRunner/public/WebUserMediaClientMock.h:36 > +#include <base/compiler_specific.h> This is a bad dependency. It should not be needed. Looks like you need to remove the OVERRIDE qualifier. (In reply to comment #14) > Looks like you need to remove the OVERRIDE qualifier. base/compiler_specific.h is needed because of "OVERRIDE". "OVERRIDE" can help us to detect any function change in base class, and it's encouraged to use it whenever possible. Does WebKit have different tradition than Chromium? Created attachment 195876 [details]
Patch
> base/compiler_specific.h is needed because of "OVERRIDE". "OVERRIDE" can help us to detect any function change in base class, and it's encouraged to use it whenever possible. Does WebKit have different tradition than Chromium?
We use OVERRIDE in WebKit as well, but we don't use it in the API to avoid these sorts of dependency problems.
Comment on attachment 195876 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195876&action=review > Tools/DumpRenderTree/DumpRenderTree.gyp/DumpRenderTree.gyp:103 > + '<(source_dir)/WebKit/chromium/public', This doesn't look correct. Lots of things in TestRunner talk about concepts from this directory, but we haven't needed this build rule previously. Take a look at http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h to see the correct pattern for including these sorts of headers. Created attachment 195880 [details]
Patch
Comment on attachment 195880 [details]
Patch
This looks good. Thanks for iterating on this patch.
Comment on attachment 195880 [details] Patch Clearing flags on attachment: 195880 Committed r147289: <http://trac.webkit.org/changeset/147289> All reviewed patches have been landed. Closing bug. Re-opened since this is blocked by bug 113678 Why are you using WEBKIT_EXPORT instead of WEBTESTRUNNER_EXPORT? And why are you exposing the mock in the api instead of overriding the methods that will return the mock in WebTestProxy? (In reply to comment #24) > Why are you using WEBKIT_EXPORT instead of WEBTESTRUNNER_EXPORT? Will use WEBTESTRUNNER_EXPORT. > > And why are you exposing the mock in the api instead of overriding the methods that will return the mock in WebTestProxy? That's anther way to access WebUserMediaClientMock. But userMediaClient() is protected in WebTestProxyBase which is used in WebKitTestRunner. Is it ok to move userMediaClient() into public in WebTestProxyBase? I don't think downcasting from WebTestProxyBase to WebTestProxy would be a good choice. (In reply to comment #25) > (In reply to comment #24) > > Why are you using WEBKIT_EXPORT instead of WEBTESTRUNNER_EXPORT? > > Will use WEBTESTRUNNER_EXPORT. > > > > > And why are you exposing the mock in the api instead of overriding the methods that will return the mock in WebTestProxy? > > That's anther way to access WebUserMediaClientMock. But userMediaClient() is protected in WebTestProxyBase which is used in WebKitTestRunner. Is it ok to move userMediaClient() into public in WebTestProxyBase? I don't think downcasting from WebTestProxyBase to WebTestProxy would be a good choice. WebTestProxy inherits from WebTestProxyBase, so it can access protected members. (In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > Why are you using WEBKIT_EXPORT instead of WEBTESTRUNNER_EXPORT? > > > > Will use WEBTESTRUNNER_EXPORT. > > > > > > > > And why are you exposing the mock in the api instead of overriding the methods that will return the mock in WebTestProxy? > > > > That's anther way to access WebUserMediaClientMock. But userMediaClient() is protected in WebTestProxyBase which is used in WebKitTestRunner. Is it ok to move userMediaClient() into public in WebTestProxyBase? I don't think downcasting from WebTestProxyBase to WebTestProxy would be a good choice. > > WebTestProxy inherits from WebTestProxyBase, so it can access protected members. I knew that. The question is how to access WebTestProxy in ShellContentRendererClient where overriding WebUserMediaClient happens? ShellContentRendererClient can access WebKitTestRunner which has only WebTestProxyBase, not WebTestProxy. Created attachment 196043 [details]
Patch
The new patch depends on https://codereview.chromium.org/13159005/. Comment on attachment 196043 [details] Patch Attachment 196043 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17327797 New failing tests: http/tests/media/reload-after-dialog.html compositing/self-painting-layers2.html compositing/video/video-reflection.html compositing/video/video-with-invalid-source.html http/tests/media/video-referer.html http/tests/media/video-cancel-load.html http/tests/appcache/video.html http/tests/media/video-play-stall-before-meta-data.html compositing/overflow/overflow-compositing-descendant.html compositing/layers-inside-overflow-scroll.html compositing/self-painting-layers.html compositing/overflow/scroll-ancestor-update.html compositing/visibility/visibility-simple-video-layer.html http/tests/canvas/webgl/origin-clean-conformance.html compositing/geometry/video-opacity-overlay.html compositing/reflections/load-video-in-reflection.html compositing/geometry/video-fixed-scrolling.html http/tests/media/text-served-as-text.html http/tests/media/video-served-as-text.html http/tests/media/video-load-suspend.html http/tests/media/video-query-url.html http/tests/media/video-error-abort.html http/tests/media/remove-while-loading.html http/tests/media/video-buffered-range-contains-currentTime.html compositing/video/video-poster.html http/tests/media/media-document.html http/tests/media/video-cookie.html compositing/geometry/clipped-video-controller.html http/tests/media/video-throttled-load-metadata.html http/tests/media/video-load-twice.html Created attachment 196114 [details]
Archive of layout-test-results from gce-cr-linux-05 for chromium-linux-x86_64
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05 Port: chromium-linux-x86_64 Platform: Linux-3.3.8-gcg-201212281604-x86_64-with-GCEL-10.04-gcel_10.04
Comment on attachment 196043 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=196043&action=review > Tools/DumpRenderTree/chromium/WebViewHost.cpp:734 > + return createMediaPlayer(frame, url, client); since createMediaPlayer is now un-used, you should copy it's body here and delete it > Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:1000 > + if (m_delegate) m_delegate should always be non-0 Created attachment 196266 [details]
Patch
Patch (on 4/2) depends on https://codereview.chromium.org/13159005/. Before that Chromium patch is landed, it's likely this patch will break webkit bots. Comment on attachment 196266 [details] Patch Attachment 196266 [details] did not pass cr-linux-debug-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17351493 Comment on attachment 196266 [details] Patch Attachment 196266 [details] did not pass cr-android-ews (chromium-android): Output: http://webkit-commit-queue.appspot.com/results/17386177 Comment on attachment 196266 [details] Patch Attachment 196266 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/17360159 This should be moved to blink. |