Bug 106434 - [chromium] move dumpcreateview methods to testrunner library
Summary: [chromium] move dumpcreateview methods to testrunner library
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-01-09 03:01 PST by Dan Carney
Modified: 2013-01-09 13:08 PST (History)
7 users (show)

See Also:


Attachments
Patch (8.79 KB, patch)
2013-01-09 03:02 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (15.97 KB, patch)
2013-01-09 07:45 PST, Dan Carney
no flags Details | Formatted Diff | Diff
Patch (16.12 KB, patch)
2013-01-09 08:08 PST, Dan Carney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dan Carney 2013-01-09 03:01:53 PST
[chromium] move dumpcreateview methods to testrunner library
Comment 1 Dan Carney 2013-01-09 03:02:27 PST
Created attachment 181886 [details]
Patch
Comment 2 WebKit Review Bot 2013-01-09 03:04:19 PST
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 jochen 2013-01-09 04:06:44 PST
Comment on attachment 181886 [details]
Patch

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

I would recommend to move this method together with canOpenWindows, so you can move the first two if statements of WebViewHost::createView to WebTestProxyBase::createView (and add WebTestProxy::createView to invoke it)

> Tools/DumpRenderTree/chromium/TestRunner/src/TestRunner.h:169
> +    void dumpCreateView(const CppArgumentList&, CppVariant*);

this should go in the section of methods that modify TestRunner's state (after dumpTitleChanges iirc)
Comment 4 Dan Carney 2013-01-09 07:45:27 PST
Created attachment 181925 [details]
Patch
Comment 5 jochen 2013-01-09 07:57:35 PST
Comment on attachment 181925 [details]
Patch

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

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestProxy.h:58
> +class WebURLRequest;

these should be alphabetically sorted. First all classes, then all structs

> Tools/DumpRenderTree/chromium/TestRunner/public/WebTestRunner.h:60
> +    virtual bool canOpenWindows() { return false; }

const missing

> Tools/DumpRenderTree/chromium/TestRunner/src/WebTestProxy.cpp:146
> +static string URLDescription(const GURL& url)

you can remove the static and move it in the anonymous namespace above
Comment 6 Dan Carney 2013-01-09 08:08:08 PST
Created attachment 181929 [details]
Patch
Comment 7 jochen 2013-01-09 08:09:24 PST
Comment on attachment 181929 [details]
Patch

LGTM
Comment 8 WebKit Review Bot 2013-01-09 13:08:37 PST
Comment on attachment 181929 [details]
Patch

Clearing flags on attachment: 181929

Committed r139231: <http://trac.webkit.org/changeset/139231>
Comment 9 WebKit Review Bot 2013-01-09 13:08:41 PST
All reviewed patches have been landed.  Closing bug.