Bug 86215 – [chromium] MediaStream API: Moving the mock create* WebRTC calls into a shadow Platform class

• Component: WebKit API
• Status: RESOLVED FIXED

[Tommy Widenflycht]

This is patch #1 of 3 to make most of the WebRTC code testable in DumpRenderTree.

This patch moves the WebRTC Platform createXXX calls into a factory.
The second patch (in chromium) will add a setMockWebRTCFactory call to TestWebKitPlatformSupport.
And the third patch will then create a Mock factory that will return Mock objects.

All Mock code will be chromium specific, but I will be very happy working with the next port that enables WebRTC to make as much as possible generic.

[Tommy Widenflycht]

Created attachment 141412 [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.

[Tommy Widenflycht]

Created attachment 141428 [details]
Patch

[Adam Barth]

Comment on attachment 141428 [details]
Patch

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

> Source/Platform/chromium/public/Platform.h:300
> +    virtual WebWebRTCFactory* webRTCFactory() { return 0; }

WebWeb -> so many Webs!

I don't understand why we need this lever of indirection.  Can't DumpRenderTree just override the virtual functions below without creating a separate factory object in the API?

[Tommy Widenflycht]

Comment on attachment 141428 [details]
Patch

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

>> Source/Platform/chromium/public/Platform.h:300
>> +    virtual WebWebRTCFactory* webRTCFactory() { return 0; }
> 
> WebWeb -> so many Webs!
> 
> I don't understand why we need this lever of indirection.  Can't DumpRenderTree just override the virtual functions below without creating a separate factory object in the API?

Re name: What do you suggest? The name is a mouthful I agree and I thought a lot about it but eventually following the standard naming scheme won.

Unfortunately DRT can't override TestWebKitPlatformSupport since it is created in the Chromium code base: http://src.chromium.org/viewvc/chrome/trunk/src/webkit/support/webkit_support.cc , line 149
Instead of adding three setMock* functions I refactored everything to use a factory.

[Adam Barth]

I recommend solving the problem on the embedder's side of the interface.  The Chromium code should let DRT override TestWebKitPlatformSupport.

[Tommy Widenflycht]

Agree that letting DRT overriding the test platform support class is the best approach but that opens up a gigantic can of worms. It seems that DRT is using a hacked/minimal version of the logging functionality tailored to get the current include files to compile and including e.g. test_webkit_platform_support.h from DumpRenderTree.cpp causes havoc. There's a fixme in DRT/chromium/config.h that says "fix this"...
I don't think I am the correct person to do that.

[Adam Barth]

> I don't think I am the correct person to do that.

Sounds like we need to unwind that hack.  Tony might have some ideas about the best approach.  Maybe we should add a mechanism to Assertions.h to disable LOG?  It might be the case that we can substitute the LOG from logging.h whenever folks are expecting LOG from Assertions.h...

[Tony Chang]

The hack mentioned in config.h may not be valid anymore.  Specifically, we shouldn't be including base/logging.h in webkit and I don't see it with a quick grep.  I think we just need to convert some Chromium style DCHECKs to WebKit style ASSERTs.

Rather than including test_webkit_platform_support.h directly, can you add functions to webkit_support.h that override the TestWebKitPlatformSupport features you need?  You could pass in a pointer to a TestWebKitPlatformSupport instance.  webkit_support.h (and webkit_support_gfx.h) is the API into chromium for DRT.

[Tommy Widenflycht]

Yep, that was what this patch was in preparation for :)

Instead of adding three different factory objects to webkit_support.h, I have grouped together our three create* functions into one factory in preparation of adding a single function to  webkit_support.h. Since the three function actually need to create objects simple set* functions can't be used.

(In reply to comment #9)
> Rather than including test_webkit_platform_support.h directly, can you add functions to webkit_support.h that override the TestWebKitPlatformSupport features you need?  You could pass in a pointer to a TestWebKitPlatformSupport instance.  webkit_support.h (and webkit_support_gfx.h) is the API into chromium for DRT.

[Adam Barth]

I'm sorry, I still don't think we should add this complexity to the API.  We should build a mechanism that lets DumpRenderTree override whichever of these virtual functions it wants.

The natural approach is for DumpRenderTree to subclass the implementation of this object in test_webkit_platform_support.h.  If we need to remove the fake implementation of logging.h in config.h, then that's what we need to do.

[Tony Chang]

Isn't test_webkit_platform_support.h only used by DRT and test_shell?  Tommy, can TestWebKitPlatformSupport always use the mock objects?

[Tommy Widenflycht]

It sure can, but I would like to place the mock objects themselves in DRT/chromium. That way any needed behaviour changes for tests can be done solely within WebKit.

(In reply to comment #12)
> Isn't test_webkit_platform_support.h only used by DRT and test_shell?  Tommy, can TestWebKitPlatformSupport always use the mock objects?

[Tommy Widenflycht]

Any ideas how to progress with this?

[Adam Barth]

IMHO, we should add the ability for DRT to subclass TestWebKitPlatformSupport in order to replace these virtual functions with mock implementations.

[Tommy Widenflycht]

To make this work I needed to "enhance" the macros since they were actually unused and not working.

The chromium patch to make this possible is here: http://codereview.chromium.org/10703114/

[Tony Chang]

(In reply to comment #15)
> IMHO, we should add the ability for DRT to subclass TestWebKitPlatformSupport in order to replace these virtual functions with mock implementations.

I don't think this is going to work.  If we #include test_webkit_platform_support.h in DRT, we're going to end up depending on everything that TestWebKitPlatformSupport #includes.  This is a lot of implementation files:
http://git.chromium.org/gitweb/?p=chromium/chromium.git;a=blob;f=webkit/support/test_webkit_platform_support.h;h=b5c78ad5d192e20dcf72b47cdd3f9d10cc5f97cf;hb=HEAD

Instead, we could have a DRT implementation of WebKitPlatformSupport that TestWebKitPlatformSupport includes an instance of.  TestWebKitPlatformSupport will first try to see if createXXX returns an object on the DRT implementation and if it's not, fall back to it's own code (or WebKitPlatformSupportImpl version).

Alternately, we could maybe add test interfaces to a DumpRenderTree/chromium/public directory and have TestWebKitPlatformSupport just call these directly.

[Adam Barth]

> Instead, we could have a DRT implementation of WebKitPlatformSupport that TestWebKitPlatformSupport includes an instance of.  TestWebKitPlatformSupport will first try to see if createXXX returns an object on the DRT implementation and if it's not, fall back to it's own code (or WebKitPlatformSupportImpl version).

That sounds like a good approach, at least for functions that have a well-defined "fail" output.

> Alternately, we could maybe add test interfaces to a DumpRenderTree/chromium/public directory and have TestWebKitPlatformSupport just call these directly.

Hum...  Can we try an approach that doesn't introduce more API first?  We should be able to do this using the WebKitPlatformSupport API itself.

[Tommy Widenflycht]

(In reply to comment #18)
> > Instead, we could have a DRT implementation of WebKitPlatformSupport that TestWebKitPlatformSupport includes an instance of.  TestWebKitPlatformSupport will first try to see if createXXX returns an object on the DRT implementation and if it's not, fall back to it's own code (or WebKitPlatformSupportImpl version).
> 
> That sounds like a good approach, at least for functions that have a well-defined "fail" output.
> 

I'll give this a try tomorrow. It is certainly sounds much cleaner than the current approach.

[Tommy Widenflycht]

Created attachment 151672 [details]
Patch

[Tommy Widenflycht]

Here's roughly how the functionality in Chromium bug 10703114 would be implemented in DRT.

[Tommy Widenflycht]

Created attachment 152714 [details]
Patch

[Tony Chang]

Comment on attachment 152714 [details]
Patch

LGTM, but I'll let Adam make the final call.  Adam has been moving many of the DRT mocks into TestRunner.  It's not clear to me how we want to share this with content_shell, but we can deal with that in the future.

[Adam Barth]

Comment on attachment 152714 [details]
Patch

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

> Tools/DumpRenderTree/chromium/MockWebKitPlatformSupport.h:40
> +    virtual void cryptographicallyRandomValues(unsigned char* buffer, size_t length) OVERRIDE;

Are we sure we want to override this function?

[Tommy Widenflycht]

Comment on attachment 152714 [details]
Patch

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

>> Tools/DumpRenderTree/chromium/MockWebKitPlatformSupport.h:40
>> +    virtual void cryptographicallyRandomValues(unsigned char* buffer, size_t length) OVERRIDE;
> 
> Are we sure we want to override this function?

Yes, it is pure virtual in WebKit::Platform.

[Tommy Widenflycht]

Right now I don't need any other intergration than just to be able to create mock versions of WebMediaStreamCenter and WebPeerConnection00Handler (or the next version currently cooking).

(In reply to comment #23)
> (From update of attachment 152714 [details])
> LGTM, but I'll let Adam make the final call.  Adam has been moving many of the DRT mocks into TestRunner.  It's not clear to me how we want to share this with content_shell, but we can deal with that in the future.

[Adam Barth]

Ok.  Let's add a CRASH() to cryptographicallyRandomValues to make sure we don't ever call it.  The reason it's pure virtual is to make sure folks provide an implementation.

[Adam Barth]

Yeah, jochen and I need to figure out what our story is with mocks in ContentShell.  We certainly don't need to block this patch on that issue, however.  I'll update the patch with the CRASH to save Tommy the timezone round-trip.

[Adam Barth]

Created attachment 153036 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 153036 [details]
Patch for landing

Rejecting attachment 153036 [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:
ools/DumpRenderTree/chromium/MockWebKitPlatformSupport.o
Tools/DumpRenderTree/chromium/MockWebKitPlatformSupport.cpp: In member function 'virtual void MockWebKitPlatformSupport::cryptographicallyRandomValues(unsigned char*, size_t)':
Tools/DumpRenderTree/chromium/MockWebKitPlatformSupport.cpp:47: error: 'CRASH' was not declared in this scope
make: *** [out/Debug/obj.target/DumpRenderTree/Tools/DumpRenderTree/chromium/MockWebKitPlatformSupport.o] Error 1
make: *** Waiting for unfinished jobs....

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

[Adam Barth]

Created attachment 153042 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 153042 [details]
Patch for landing

Clearing flags on attachment: 153042

Committed r122995: <http://trac.webkit.org/changeset/122995>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Tommy Widenflycht]

Thanks!