Bug 113633 – Allow WebUserMediaClientMock to be used by client

• Component: Tools / Tests
• Status: RESOLVED INVALID

To enable media stream layout test in chromium content_shell, WebUserMediaClientMock is needed.

Created attachment 195861 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] 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.

[Adam Barth]

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.

[Adam Barth]

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.

[Adam Barth]

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

[Adam Barth]

The functions don't appear to be used.  If they are used, you'll need to refactor the code.

[Adam Barth]

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.

[Adam Barth]

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

[Adam Barth]

> 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.

[Adam Barth]

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

[Adam Barth]

Comment on attachment 195880 [details]
Patch

This looks good.  Thanks for iterating on this patch.

[WebKit Review Bot]

Comment on attachment 195880 [details]
Patch

Clearing flags on attachment: 195880

Committed r147289: <http://trac.webkit.org/changeset/147289>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

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/.

[WebKit Review Bot]

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

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Peter Beverloo (cr-android ews)]

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

[WebKit Review Bot]

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

[Levi Weintraub]

This should be moved to blink.