Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(8)

Issue 10703114: Adding support for overriding TestWebKitPlatformSupport in DumpRenderTree (Closed)

Created:
7 years, 1 month ago by Tommy Widenflycht
Modified:
7 years, 1 month ago
CC:
chromium-reviews, darin-cc_chromium.org
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fixing comments #

Total comments: 2

Patch Set 3 : Review comments #

Total comments: 6

Patch Set 4 : Review comments fixed #

Total comments: 1

Patch Set 5 : Refactored to use a shadow PlatformSupport object #

Total comments: 4

Patch Set 6 : "Fixed review comments" #

Unified diffs Side-by-side diffs Delta from patch set Stats (+44 lines, -9 lines) Patch
M webkit/support/test_webkit_platform_support.h View 1 2 3 4 5 3 chunks +6 lines, -1 line 0 comments Download
M webkit/support/test_webkit_platform_support.cc View 1 2 3 4 5 2 chunks +14 lines, -2 lines 0 comments Download
M webkit/support/webkit_support.h View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M webkit/support/webkit_support.cc View 1 2 3 4 5 5 chunks +20 lines, -6 lines 0 comments Download

Messages

Total messages: 17 (0 generated)
Tommy Widenflycht
7 years, 1 month ago (2012-07-09 12:15:26 UTC) #1
tommi (sloooow) - chröme
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.cc#newcode164 webkit/support/webkit_support.cc:164: webkit_platform_support_.reset(callback.Run(unit_test_mode)); after calling callback.Run(), ...
7 years, 1 month ago (2012-07-09 12:19:29 UTC) #2
Tommy Widenflycht
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.cc#newcode164 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 ...
7 years, 1 month ago (2012-07-09 12:25:31 UTC) #3
Tommy Widenflycht
7 years, 1 month ago (2012-07-09 12:26:45 UTC) #4
tommi (sloooow) - chröme
lgtm
7 years, 1 month ago (2012-07-09 12:28:01 UTC) #5
tony
http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_support.h File webkit/support/webkit_support.h (right): http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_support.h#newcode12 webkit/support/webkit_support.h:12: #include "base/callback.h" We don't want to include callback.h here. ...
7 years, 1 month ago (2012-07-09 18:23:29 UTC) #6
Tommy Widenflycht
http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_support.h File webkit/support/webkit_support.h (right): http://codereview.chromium.org/10703114/diff/7001/webkit/support/webkit_support.h#newcode12 webkit/support/webkit_support.h:12: #include "base/callback.h" On 2012/07/09 18:23:29, tony wrote: > We ...
7 years, 1 month ago (2012-07-10 09:27:19 UTC) #7
tommi (sloooow) - chröme
lgtm with the below fixed http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_support.cc File webkit/support/webkit_support.cc (right): http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_support.cc#newcode334 webkit/support/webkit_support.cc:334: SetUpTestEnvironment(CreateDefaultTestWebKitPlatformSupport); SetUpTestEnvironment(&CreateDefaultTestWebKitPlatformSupport); http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_support.cc#newcode338 webkit/support/webkit_support.cc:338: ...
7 years, 1 month ago (2012-07-10 09:48:37 UTC) #8
Tommy Widenflycht
http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_support.cc File webkit/support/webkit_support.cc (right): http://codereview.chromium.org/10703114/diff/9001/webkit/support/webkit_support.cc#newcode334 webkit/support/webkit_support.cc:334: SetUpTestEnvironment(CreateDefaultTestWebKitPlatformSupport); On 2012/07/10 09:48:37, tommi wrote: > SetUpTestEnvironment(&CreateDefaultTestWebKitPlatformSupport); Done. ...
7 years, 1 month ago (2012-07-10 09:58:10 UTC) #9
tony
http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_support.h File webkit/support/webkit_support.h (right): http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_support.h#newcode45 webkit/support/webkit_support.h:45: typedef TestWebKitPlatformSupport* (*PlatformSupportCreatorCallback)( Do we have to use TestWebKitPlatformSupport ...
7 years, 1 month ago (2012-07-10 17:39:08 UTC) #10
tony
On 2012/07/10 17:39:08, tony wrote: > http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_support.h > File webkit/support/webkit_support.h (right): > > http://codereview.chromium.org/10703114/diff/5002/webkit/support/webkit_support.h#newcode45 > ...
7 years, 1 month ago (2012-07-10 17:48:25 UTC) #11
Tommy Widenflycht
On 2012/07/10 17:48:25, tony wrote: > Oh, I guess it doesn't matter if you're going ...
7 years, 1 month ago (2012-07-10 17:56:34 UTC) #12
Tommy Widenflycht
Refactored according to the discussion in https://bugs.webkit.org/show_bug.cgi?id=86215
7 years, 1 month ago (2012-07-11 10:59:12 UTC) #13
tony
LGTM, just some style and naming nits. https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test_webkit_platform_support.h File webkit/support/test_webkit_platform_support.h (right): https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test_webkit_platform_support.h#newcode145 webkit/support/test_webkit_platform_support.h:145: WebKit::Platform* shadow_platform_support_; ...
7 years, 1 month ago (2012-07-11 17:56:42 UTC) #14
Tommy Widenflycht
https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test_webkit_platform_support.h File webkit/support/test_webkit_platform_support.h (right): https://chromiumcodereview.appspot.com/10703114/diff/4005/webkit/support/test_webkit_platform_support.h#newcode145 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 ...
7 years, 1 month ago (2012-07-12 13:40:58 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tommyw@chromium.org/10703114/9004
7 years, 1 month ago (2012-07-12 13:42:05 UTC) #16
commit-bot: I haz the power
7 years, 1 month ago (2012-07-12 15:17:43 UTC) #17
Change committed as 146351

Powered by Google App Engine
This is Rietveld 408576698