Bug 86215

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

Created attachment 141428 [details] Patch

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

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

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.

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

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.

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.

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.

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

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?

Any ideas how to progress with this?

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

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/

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

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

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

Created attachment 151672 [details] Patch

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

Created attachment 152714 [details] Patch

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

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.

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.

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.

Created attachment 153036 [details] Patch for landing

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

Created attachment 153042 [details] Patch for landing

Comment on attachment 153042 [details] Patch for landing Clearing flags on attachment: 153042 Committed r122995: <http://trac.webkit.org/changeset/122995>

All reviewed patches have been landed. Closing bug.

Thanks!