Adding support for overriding TestWebKitPlatformSupport in DumpRenderTree

Adding support for overriding TestWebKitPlatformSupport in DumpRenderTree
To be able to add mocks of PlatformSupport functions DRT needs to be able to override
the creation of TestWebKitPlatformSupport.

BUG=
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146351

[tommi (sloooow) - chröme, 2012-07-09 12:19:29]

only nits, looks good otherwise

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support.cc
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support....
webkit/support/webkit_support.cc:164:
webkit_platform_support_.reset(callback.Run(unit_test_mode));
after calling callback.Run(), you can call callback.Reset() to make sure it
never gets run again and free up any potential resources.

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support.h
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support....
webkit/support/webkit_support.h:67: void SetUpTestEnvironment(const
PlatformSupportCreatorCallback&);
add variable names

[Tommy Widenflycht, 2012-07-09 12:25:31]

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support.cc
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support....
webkit/support/webkit_support.cc:164:
webkit_platform_support_.reset(callback.Run(unit_test_mode));
On 2012/07/09 12:19:29, tommi wrote:
> after calling callback.Run(), you can call callback.Reset() to make sure it
> never gets run again and free up any potential resources.

Not possible to call Reset() on it since it is const, but it goes out of scope
anyway directly after the call chain ends.

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support.h
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/10703114/diff/1/webkit/support/webkit_support....
webkit/support/webkit_support.h:67: void SetUpTestEnvironment(const
PlatformSupportCreatorCallback&);
On 2012/07/09 12:19:29, tommi wrote:
> add variable names

Done.

[tommi (sloooow) - chröme, 2012-07-09 12:28:01]

lgtm

[tony, 2012-07-09 18:23:29]

http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:12: #include "base/callback.h"
We don't want to include callback.h here.  This makes it very hard for the types
in callback.h to change since code in the WebKit repository will depend on it.

You can just use a plain function pointer typedef instead.

[Tommy Widenflycht, 2012-07-10 09:27:19]

http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:12: #include "base/callback.h"
On 2012/07/09 18:23:29, tony wrote:
> We don't want to include callback.h here.  This makes it very hard for the
types
> in callback.h to change since code in the WebKit repository will depend on it.
> 
> You can just use a plain function pointer typedef instead.

Done.

[tommi (sloooow) - chröme, 2012-07-10 09:48:37]

lgtm with the below fixed

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:334:
SetUpTestEnvironment(CreateDefaultTestWebKitPlatformSupport);
SetUpTestEnvironment(&CreateDefaultTestWebKitPlatformSupport);

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:338:
SetUpTestEnvironmentForUnitTests(CreateDefaultTestWebKitPlatformSupport);
missing &

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:45: typedef
TestWebKitPlatformSupport*(*PlatformSupportCreatorCallback)(
nit: add a space after the return type:
typedef TestWebKitPlatformSupport* (*PlatformSupportCreatorCallback)(

[Tommy Widenflycht, 2012-07-10 09:58:10]

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.cc (right):

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:334:
SetUpTestEnvironment(CreateDefaultTestWebKitPlatformSupport);
On 2012/07/10 09:48:37, tommi wrote:
> SetUpTestEnvironment(&CreateDefaultTestWebKitPlatformSupport);

Done.

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
webkit/support/webkit_support.cc:338:
SetUpTestEnvironmentForUnitTests(CreateDefaultTestWebKitPlatformSupport);
On 2012/07/10 09:48:37, tommi wrote:
> missing &

Done.

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:45: typedef
TestWebKitPlatformSupport*(*PlatformSupportCreatorCallback)(
On 2012/07/10 09:48:37, tommi wrote:
> nit: add a space after the return type:
> typedef TestWebKitPlatformSupport* (*PlatformSupportCreatorCallback)(

Done.

[tony, 2012-07-10 17:39:08]

http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_suppo...
File webkit/support/webkit_support.h (right):

http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_suppo...
webkit/support/webkit_support.h:45: typedef TestWebKitPlatformSupport*
(*PlatformSupportCreatorCallback)(
Do we have to use TestWebKitPlatformSupport or can we use the interface
WebKit::WebKitPlatformSupport?  That would have less dependencies between
repositories.

[tony, 2012-07-10 17:48:25]

On 2012/07/10 17:39:08, tony wrote:
>
http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_suppo...
> File webkit/support/webkit_support.h (right):
> 
>
http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_suppo...
> webkit/support/webkit_support.h:45: typedef TestWebKitPlatformSupport*
> (*PlatformSupportCreatorCallback)(
> Do we have to use TestWebKitPlatformSupport or can we use the interface
> WebKit::WebKitPlatformSupport?  That would have less dependencies between
> repositories.

Oh, I guess it doesn't matter if you're going to subclass
TestWebKitPlatformSupport in DRT.  Hmm, that's going to pull in lots of
additional dependencies since test_webkit_platform_support.h depends on lots of
things in src/webkit.  I'll follow up with abarth on the webkit bug.

[Tommy Widenflycht, 2012-07-10 17:56:34]

On 2012/07/10 17:48:25, tony wrote:
> Oh, I guess it doesn't matter if you're going to subclass
> TestWebKitPlatformSupport in DRT.  Hmm, that's going to pull in lots of
> additional dependencies since test_webkit_platform_support.h depends on lots
of
> things in src/webkit.  I'll follow up with abarth on the webkit bug.

Yes, this is not a pretty solution but right now I have no other choice.

[Tommy Widenflycht, 2012-07-11 10:59:12]

Refactored according to the discussion in
https://bugs.webkit.org/show_bug.cgi?id=86215

[tony, 2012-07-11 17:56:42]

LGTM, just some style and naming nits.

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test...
File webkit/support/test_webkit_platform_support.h (right):

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test...
webkit/support/test_webkit_platform_support.h:145: WebKit::Platform*
shadow_platform_support_;
Nit: I would call this either shadow_platform_delegate_ (chromium style name) or
shadow_platform_client_ (WebKit style name).

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/webk...
File webkit/support/webkit_support.h (right):

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/webk...
webkit/support/webkit_support.h:64: WebKit::Platform* shadow_platform_support =
NULL);
The style guide says no default arguments.  Having 2 methods for each is fine. 
Once we make the change in WebKit, we can remove the no-argument function.

[Tommy Widenflycht, 2012-07-12 13:40:58]

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test...
File webkit/support/test_webkit_platform_support.h (right):

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test...
webkit/support/test_webkit_platform_support.h:145: WebKit::Platform*
shadow_platform_support_;
Renamed to shadow_platform_delegate_.

On 2012/07/11 17:56:42, tony wrote:
> Nit: I would call this either shadow_platform_delegate_ (chromium style name)
or
> shadow_platform_client_ (WebKit style name).

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/webk...
File webkit/support/webkit_support.h (right):

https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/webk...
webkit/support/webkit_support.h:64: WebKit::Platform* shadow_platform_support =
NULL);
On 2012/07/11 17:56:42, tony wrote:
> The style guide says no default arguments.  Having 2 methods for each is fine.

> Once we make the change in WebKit, we can remove the no-argument function.

Done.

[commit-bot: I haz the power, 2012-07-12 13:42:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tommyw@chromium.org/10703114/9004

[commit-bot: I haz the power, 2012-07-12 15:17:43]

Change committed as 146351