Bug 86215

Summary: [chromium] MediaStream API: Moving the mock create* WebRTC calls into a shadow Platform class
Product: WebKit Reporter: Tommy Widenflycht <tommyw>
Component: WebKit APIAssignee: Tommy Widenflycht <tommyw>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, eric.carlson, feature-media-reviews, fishd, jamesr, jochen, tkent, tkent+wkapi, tony, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 61069    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing none

Description Tommy Widenflycht 2012-05-11 07:10:26 PDT
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.
Comment 1 Tommy Widenflycht 2012-05-11 07:17:14 PDT
Created attachment 141412 [details]
Patch
Comment 2 WebKit Review Bot 2012-05-11 07:20:13 PDT
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.
Comment 3 Tommy Widenflycht 2012-05-11 09:21:00 PDT
Created attachment 141428 [details]
Patch
Comment 4 Adam Barth 2012-05-11 10:00:12 PDT
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?
Comment 5 Tommy Widenflycht 2012-05-14 02:19:13 PDT
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.
Comment 6 Adam Barth 2012-05-14 10:58:27 PDT
I recommend solving the problem on the embedder's side of the interface.  The Chromium code should let DRT override TestWebKitPlatformSupport.
Comment 7 Tommy Widenflycht 2012-05-15 04:40:13 PDT
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.
Comment 8 Adam Barth 2012-05-15 11:15:08 PDT
> 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...
Comment 9 Tony Chang 2012-05-15 13:38:02 PDT
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.
Comment 10 Tommy Widenflycht 2012-05-16 01:32:18 PDT
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.
Comment 11 Adam Barth 2012-05-16 19:46:15 PDT
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.
Comment 12 Tony Chang 2012-05-17 09:59:10 PDT
Isn't test_webkit_platform_support.h only used by DRT and test_shell?  Tommy, can TestWebKitPlatformSupport always use the mock objects?
Comment 13 Tommy Widenflycht 2012-05-21 01:10:32 PDT
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?
Comment 14 Tommy Widenflycht 2012-06-26 05:13:39 PDT
Any ideas how to progress with this?
Comment 15 Adam Barth 2012-06-26 11:10:41 PDT
IMHO, we should add the ability for DRT to subclass TestWebKitPlatformSupport in order to replace these virtual functions with mock implementations.
Comment 16 Tommy Widenflycht 2012-07-09 05:28:47 PDT
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/
Comment 17 Tony Chang 2012-07-10 11:01:21 PDT
(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.
Comment 18 Adam Barth 2012-07-10 12:20:02 PDT
> 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.
Comment 19 Tommy Widenflycht 2012-07-10 12:50:26 PDT
(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.
Comment 20 Tommy Widenflycht 2012-07-11 04:08:57 PDT
Created attachment 151672 [details]
Patch
Comment 21 Tommy Widenflycht 2012-07-11 04:11:15 PDT
Here's roughly how the functionality in Chromium bug 10703114 would be implemented in DRT.
Comment 22 Tommy Widenflycht 2012-07-17 01:25:30 PDT
Created attachment 152714 [details]
Patch
Comment 23 Tony Chang 2012-07-17 09:36:28 PDT
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.
Comment 24 Adam Barth 2012-07-17 11:00:44 PDT
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?
Comment 25 Tommy Widenflycht 2012-07-18 00:33:17 PDT
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.
Comment 26 Tommy Widenflycht 2012-07-18 00:36:06 PDT
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.
Comment 27 Adam Barth 2012-07-18 09:56:23 PDT
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.
Comment 28 Adam Barth 2012-07-18 09:58:25 PDT
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.
Comment 29 Adam Barth 2012-07-18 10:19:32 PDT
Created attachment 153036 [details]
Patch for landing
Comment 30 WebKit Review Bot 2012-07-18 10:38:33 PDT
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
Comment 31 Adam Barth 2012-07-18 10:44:50 PDT
Created attachment 153042 [details]
Patch for landing
Comment 32 WebKit Review Bot 2012-07-18 11:34:29 PDT
Comment on attachment 153042 [details]
Patch for landing

Clearing flags on attachment: 153042

Committed r122995: <http://trac.webkit.org/changeset/122995>
Comment 33 WebKit Review Bot 2012-07-18 11:34:37 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 Tommy Widenflycht 2012-07-19 00:36:09 PDT
Thanks!