Bug 40558 – [DRT/Chromium] Upstream TestShellDevTools for Chromium DRT

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Roland Steiner]

Upstream TestShellDevTools for Chromium DRT

[Roland Steiner]

Created attachment 59084 [details]
patch - Upstreamed DevTools for Chromium DRT

First version, tested under Windows (running LayoutTests/inspector).

I'm unsure about the modification to WebKit.gyp (adding include dir for header generated from resources), i.e., whether to really put that there, and for which platforms.

Difference to test shell version: on showDevTools() the test shell version instantiated a separate TestShell instance using weak pointers.
This doesn't seem to be necessary for the DRT implementation (using a single TestShell instance and a simple WebViewHost* instead).

[WebKit Review Bot]

Attachment 59084 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/chromium/TestShell.cpp:65:  webkit_glue is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Attachment 59084 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3308316

[Kent Tamura]

Comment on attachment 59084 [details]
patch - Upstreamed DevTools for Chromium DRT

http://wkrietveld.appspot.com/40558/diff/1/4
File WebKitTools/ChangeLog (right):

http://wkrietveld.appspot.com/40558/diff/1/4#newcode8
WebKitTools/ChangeLog:8: Upstream DevTools for Chromium DRT.
Please write the base Chromium revision of new files.

http://wkrietveld.appspot.com/40558/diff/1/4#newcode26
WebKitTools/ChangeLog:26: * DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp: Added.
The name "WebDevToolsDRTAgent" looks curious to me.  I feel "WebDevToolsAgentDRT" or "DRT(Web)DevToolsAgent" is better.
Same for other new files.

http://wkrietveld.appspot.com/40558/diff/1/6
File WebKitTools/DumpRenderTree/chromium/EventSender.cpp (right):

http://wkrietveld.appspot.com/40558/diff/1/6#newcode63
WebKitTools/DumpRenderTree/chromium/EventSender.cpp:63: #pragma warning(disable : 4355)
It's better to have this in config.h.

http://wkrietveld.appspot.com/40558/diff/1/8
File WebKitTools/DumpRenderTree/chromium/TestShell.cpp (right):

http://wkrietveld.appspot.com/40558/diff/1/8#newcode57
WebKitTools/DumpRenderTree/chromium/TestShell.cpp:57: #include "webkit/glue/webkit_glue.h"
Do not use webkti_glue.h
If you can't remove it easily, add a FIME comment.

http://wkrietveld.appspot.com/40558/diff/1/8#newcode135
WebKitTools/DumpRenderTree/chromium/TestShell.cpp:135: FilePath dirExe;
Do not use FilePath.

http://wkrietveld.appspot.com/40558/diff/1/9
File WebKitTools/DumpRenderTree/chromium/TestShell.h (right):

http://wkrietveld.appspot.com/40558/diff/1/9#newcode146
WebKitTools/DumpRenderTree/chromium/TestShell.h:146: void initializeWebDevToolsDRTAgent(WebKit::WebView* webView);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/9#newcode147
WebKitTools/DumpRenderTree/chromium/TestShell.h:147: void createWebDevToolsDRTClient(WebDevToolsDRTAgent *agent);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/10
File WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp (right):

http://wkrietveld.appspot.com/40558/diff/1/10#newcode37
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:37: #include "base/message_loop.h"
Do not use message_loop.h.

http://wkrietveld.appspot.com/40558/diff/1/10#newcode38
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:38: #include "base/string_piece.h"
Do not use string_pieace.h.

http://wkrietveld.appspot.com/40558/diff/1/10#newcode45
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:45: #include "webkit/glue/webkit_glue.h"
Do not use webkit_glue.h.

http://wkrietveld.appspot.com/40558/diff/1/10#newcode52
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:52: using WebKit::WebCString;
"using namespace WebKit" is enough.

http://wkrietveld.appspot.com/40558/diff/1/10#newcode85
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:85: // FIXME: implement this. - original TODO(loislo)
Remove TODO(name) comment

http://wkrietveld.appspot.com/40558/diff/1/10#newcode90
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:90: base::StringPiece injectJSWebkit = webkit_glue::GetDataResource(IDR_DEVTOOLS_INJECT_WEBKIT_JS);
We should not use StringPieace and webkit_glue in WebKit code.
So how about adding following function to webkit_support?
  WebCString webkit_support::GetDevToolsInjectedScriptSource();
  WebCString webkit_support::GetDevToolsInjectedScriptDispatcherSource();
  WebCString webkit_support::GetDevToolsDebuggerScriptSource();

http://wkrietveld.appspot.com/40558/diff/1/10#newcode108
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:108: MessageLoop::current()->PostDelayedTask(
Do not use MessageLoop.  Use webkit_support::PostDelayedTaskFromhere() instead.

http://wkrietveld.appspot.com/40558/diff/1/10#newcode167
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.cpp:167: void WebDevToolsDRTAgent::dispatchMessageLoop()
Use webkit_support::DispatchMessageLoop() instead.

http://wkrietveld.appspot.com/40558/diff/1/11
File WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h (right):

http://wkrietveld.appspot.com/40558/diff/1/11#newcode34
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h:34: #include "base/logging.h"
Do not use base/logging.h.

http://wkrietveld.appspot.com/40558/diff/1/11#newcode57
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h:57: WebDevToolsDRTAgent(WebKit::WebView* web_view);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/11#newcode61
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h:61: virtual void sendMessageToFrontend(const WebKit::WebDevToolsMessageData& data);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/11#newcode69
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h:69: void asyncCall(const WebDevToolsDRTCallArgs& args);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/11#newcode71
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h:71: void attach(WebDevToolsDRTClient* client);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/11#newcode72
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h:72: void detach(WebDevToolsDRTClient* client);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/11#newcode78
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTAgent.h:78: void call(const WebDevToolsDRTCallArgs& args);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/12
File WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTCallArgs.cpp (right):

http://wkrietveld.appspot.com/40558/diff/1/12#newcode35
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTCallArgs.cpp:35: int WebDevToolsDRTCallArgs::m_callsCount = 0;
Can we merge this to another file?

http://wkrietveld.appspot.com/40558/diff/1/14
File WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.cpp (right):

http://wkrietveld.appspot.com/40558/diff/1/14#newcode37
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.cpp:37: #include "base/message_loop.h"
Do not use message_loop.h.

http://wkrietveld.appspot.com/40558/diff/1/14#newcode113
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.cpp:113: MessageLoop::current()->PostDelayedTask(
Use webkit_support::PostDelayedTaskFromHere().

http://wkrietveld.appspot.com/40558/diff/1/15
File WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.h (right):

http://wkrietveld.appspot.com/40558/diff/1/15#newcode36
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.h:36: #undef LOG
is this needed?

http://wkrietveld.appspot.com/40558/diff/1/15#newcode57
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.h:57: WebDevToolsDRTClient(WebDevToolsDRTAgent* agent, WebKit::WebView* web_view);
Argument names are not needed.

http://wkrietveld.appspot.com/40558/diff/1/15#newcode61
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.h:61: virtual void sendMessageToAgent(const WebKit::WebDevToolsMessageData& data);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/15#newcode69
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.h:69: void asyncCall(const WebDevToolsDRTCallArgs& args);
Argument name is not needed.

http://wkrietveld.appspot.com/40558/diff/1/15#newcode74
WebKitTools/DumpRenderTree/chromium/WebDevToolsDRTClient.h:74: void call(const WebDevToolsDRTCallArgs& args);
Argument name is not needed.

[Dimitri Glazkov (Google)]

Comment on attachment 59084 [details]
patch - Upstreamed DevTools for Chromium DRT

r- based on comments from tkent.

[Roland Steiner]

Created attachment 60360 [details]
patch - snapshot of current state

Temporary snap-shot of current state. It's not meant as a final patch since there are some remaining issues that I'm currently stuck on:

For some reason, the event handlers are not properly called, esp. WebDevToolsFrontendImpl::frontendLoaded() is never called from dispatchWindowLoadEvent() in Document::implicitClose() (via loadedCallback in the auto-generated V8InspectorFrontendHost.cpp).

Hence no request for review (the patch also contains 2 debugging MessageBox calls, as well as those annoying 755 file modes).

[Kent Tamura]

(In reply to comment #6)
> Created an attachment (id=60360) [details]
> patch - snapshot of current state

> Hence no request for review (the patch also contains 2 debugging MessageBox calls, as well as those annoying 755 file modes).

I think we had better commit the code as soon as possible even if the code is incomplete so that other Chromium developers can investigate problems.

[Dimitri Glazkov (Google)]

I wonder if it is time to involve the inspector folks who wrotenthis code in the first place.

[Roland Steiner]

Created attachment 60363 [details]
patch - snapshot of current state (removed debugging code, fixed modes)

Based on Kent-san's comment, uploaded a cleaned-up version of the current state, without debugging message boxes, and with the filemodes corrected.

Apart from the code issues, as indicated with my first patch, please also check whether my modifications to webkit.gyp are OK. I feel that having the include dirs modification just for Windows is probably not correct.

[WebKit Review Bot]

Attachment 60363 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKitTools/DumpRenderTree/chromium/TestShell.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/DumpRenderTree/chromium/TestShell.cpp:57:  Should have a space between // and comment  [whitespace/comments] [4]
WebKitTools/DumpRenderTree/chromium/DRTDevToolsClient.h:36:  Should have a space between // and comment  [whitespace/comments] [4]
WebKitTools/DumpRenderTree/chromium/LayoutTestController.cpp:36:  Alphabetical sorting problem.  [build/include_order] [4]
WebKitTools/DumpRenderTree/chromium/DRTDevToolsAgent.h:34:  Should have a space between // and comment  [whitespace/comments] [4]
WebKitTools/DumpRenderTree/chromium/DRTDevToolsAgent.h:37:  Should have a space between // and comment  [whitespace/comments] [4]
WebKitTools/DumpRenderTree/chromium/DRTDevToolsAgent.cpp:58:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 7 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Pavel Feldman]

What is the status of this code? Should it be reviewed? Is it a pure port or does it do anything?

[Roland Steiner]

@Pavel: It's the upstreamed version of test_shell_devtools. 

The code basically seems to get called and instantiated fine, AFAICT - it just doesn't currently do anything, because it hangs due to the problems I described.

[Kent Tamura]

Comment on attachment 60363 [details]
patch - snapshot of current state (removed debugging code, fixed modes)

Anyway, please fix style errors.


WebKitTools/DumpRenderTree/chromium/DRTDevToolsClient.cpp:45
 +  using WebKit::WebDevToolsAgent;
We prefer "using namespace WebKit;" in .cpp files.



WebKitTools/DumpRenderTree/chromium/DRTDevToolsClient.cpp:112
 +  void DRTDevToolsClient::call(const DRTDevToolsCallArgs &args)
"DRTDevToolsCallArgs &arg" -> "DRTDevToolsCallArgs& arg"


WebKitTools/DumpRenderTree/chromium/TestShell.cpp:124
 +  void TestShell::createDRTDevToolsClient(DRTDevToolsAgent *agent)
"DRTDevToolsAgent *agent" -> "DRTDevToolsAgent* agent"


WebKitTools/DumpRenderTree/chromium/config.h:44
 +  #define NOTREACHED() DCHECK(false)
Do we still need this?


WebKitTools/DumpRenderTree/chromium/EventSender.cpp:61
 +  #endif // OS(WINDOWS)
This patch doesn't need this change.

[Roland Steiner]

Created attachment 60495 [details]
patch - snapshot of current state (fixed style bugs and addressed review remarks)

New patch: addressed style issues and last review remarks. Code itself is unchanged.

[Kent Tamura]

Comment on attachment 60495 [details]
patch - snapshot of current state (fixed style bugs and addressed review remarks)

WebKitTools/DumpRenderTree/chromium/TestShell.h:88
 +      TestShell* m_testShell;
We don't prepend "m_" to public data members.

[Roland Steiner]

Created attachment 60497 [details]
patch - snapshot of current state (removed unnecessary struct)

Patch: removed unnecessary struct 'TestShellWeakReference'

(also, resubmitted to use commit-queue)

[WebKit Commit Bot]

Comment on attachment 60497 [details]
patch - snapshot of current state (removed unnecessary struct)

Rejecting patch 60497 from commit-queue.

Unexpected failure when processing patch!  Please file a bug against webkit-patch.
Failed to run "['./WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--build', '--non-interactive', '--ignore-builders', '--build-style=both', '--quiet', 60497, '--test', '--parent-command=commit-queue', '--no-update']" exit_code: 1
Last 500 characters of output:
action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=40558&ctype=xml
Processing 1 patch from 1 bug.
Cleaning working directory
Processing patch 60497 from bug 40558.
Tamura Kent found in /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog does not appear to be a valid reviewer according to committers.py.
ERROR: /Users/eseidel/Projects/CommitQueue/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Kent Tamura]

Comment on attachment 60497 [details]
patch - snapshot of current state (removed unnecessary struct)

I don't know why CQ rejects the patch :-<

WebKitTools/DumpRenderTree/chromium/TestShell.cpp:125
 +      m_drtDevToolsClient.set(new DRTDevToolsClient(agent, webView()));
webView() should be m_devTools->webView().
This is the root cause of no frontendLoaded() called.

[Kent Tamura]

Comment on attachment 60497 [details]
patch - snapshot of current state (removed unnecessary struct)

WebKit/chromium/ChangeLog:3
 +          Reviewed by Tamura Kent.
You shouldn't put a reviewer name to a uploading patch.

[Roland Steiner]

Created attachment 60602 [details]
patch - snapshot of current state (added tkent's fix, NOBODY-fied the reviewer in the ChangeLogs)

Made the suggested change to webView, but test still time out (further debugging pending). Also reset reviewer back to "NOBODY (OOPS!)" to mollify the CQ.

[Kent Tamura]

Comment on attachment 60602 [details]
patch - snapshot of current state (added tkent's fix, NOBODY-fied the reviewer in the ChangeLogs)



> diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
> index 62ad190..fa7d242 100644
> --- a/WebKitTools/ChangeLog
> +++ b/WebKitTools/ChangeLog
> @@ -1,3 +1,72 @@
> [phone]  Roland Steiner  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Bug 40558 - [DRT/Chromium] Upstream TestShellDevTools for Chromium DRT
> +        https://bugs.webkit.org/show_bug.cgi?id=40558
> +
> +        Upstream DevTools for Chromium DRT.
> +        (original Chromium files rev. 51287)
> +
> +        * DumpRenderTree/DumpRenderTree.gypi:
> +        * DumpRenderTree/chromium/DRTDevToolsAgent.cpp: Added.
> +        (DRTDevToolsAgent::DRTDevToolsAgent):
> +        (DRTDevToolsAgent::setWebView):
> +        (DRTDevToolsAgent::sendMessageToFrontend):
> +        (DRTDevToolsAgent::forceRepaint):
> +        (DRTDevToolsAgent::runtimeFeatureStateChanged):
> +        (DRTDevToolsAgent::injectedScriptSource):
> +        (DRTDevToolsAgent::injectedScriptDispatcherSource):
> +        (DRTDevToolsAgent::debuggerScriptSource):
> +        (DRTDevToolsAgent::asyncCall):
> +        (DRTDevToolsAgent::call):
> +        (DRTDevToolsAgent::webDevToolsAgent):
> +        (DRTDevToolsAgent::attach):
> +        (DRTDevToolsAgent::detach):
> +        (DRTDevToolsAgent::setTimelineProfilingEnabled):
> +        (DRTDevToolsAgent::evaluateInWebInspector):
> +        (DRTDevToolsAgent::dispatchMessageLoop):
> +        * DumpRenderTree/chromium/DRTDevToolsAgent.h: Added.
> +        (DRTDevToolsAgent::~DRTDevToolsAgent):
> +        (DRTDevToolsAgent::hostIdentifier):
> +        * DumpRenderTree/chromium/DRTDevToolsCallArgs.cpp:
> +        * DumpRenderTree/chromium/DRTDevToolsCallArgs.h:
> +        (DRTDevToolsCallArgs::DRTDevToolsCallArgs):
> +        (DRTDevToolsCallArgs::~DRTDevToolsCallArgs):
> +        (DRTDevToolsCallArgs::callsCount):
> +        * DumpRenderTree/chromium/DRTDevToolsClient.cpp: Added.
> +        (DRTDevToolsClient::DRTDevToolsClient):
> +        (DRTDevToolsClient::~DRTDevToolsClient):
> +        (DRTDevToolsClient::sendMessageToAgent):
> +        (DRTDevToolsClient::sendDebuggerCommandToAgent):
> +        (DRTDevToolsClient::activateWindow):
> +        (DRTDevToolsClient::closeWindow):
> +        (DRTDevToolsClient::dockWindow):
> +        (DRTDevToolsClient::undockWindow):
> +        (DRTDevToolsClient::asyncCall):
> +        (DRTDevToolsClient::call):
> +        (DRTDevToolsClient::allMessagesProcessed):
> +        * DumpRenderTree/chromium/DRTDevToolsClient.h:
> +        * DumpRenderTree/chromium/EventSender.cpp:
> +        * DumpRenderTree/chromium/LayoutTestController.cpp:
> +        (LayoutTestController::LayoutTestController):
> +        (LayoutTestController::closeWebInspector):
> +        (LayoutTestController::setTimelineProfilingEnabled):
> +        (LayoutTestController::evaluateInWebInspector):
> +        * DumpRenderTree/chromium/LayoutTestController.h:
> +        * DumpRenderTree/chromium/TestShell.cpp:
> +        (TestShell::TestShell):
> +        (TestShell::~TestShell):
> +        (TestShell::createDRTDevToolsClient):
> +        (TestShell::showDevTools):
> +        (TestShell::closeDevTools):
> +        (TestShell::runFileTest):
> +        (TestShell::createNewWindow):
> +        * DumpRenderTree/chromium/TestShell.h:
> +        (TestShell::drtDevToolsAgent):
> +        (TestShell::drtDevToolsClient):
> +        * DumpRenderTree/chromium/config.h:
> +
>  2010-07-05  Csaba Osztrogonác  <[e-mail]>
>  
>          Reviewed by Eric Seidel.
> @@ -40,7 +109,6 @@
>  
>          * Scripts/webkitpy/common/checkout/scm.py:
>          * Scripts/webkitpy/common/checkout/scm_unittest.py:
> -
>  2010-07-03  Patrick Gansterer  <[e-mail]>

Do not change unrelated part of ChangeLog.


WebKit/chromium/WebKit.gyp:633
 +                          '<(SHARED_INTERMEDIATE_DIR)/webkit', # for a header generated by grit
Is this needed?

[Eric Seidel (no email)]

Comment on attachment 60495 [details]
patch - snapshot of current state (fixed style bugs and addressed review remarks)

Cleared Kent Tamura's review+ from obsolete attachment 60495 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Roland Steiner]

Created attachment 60619 [details]
patch - snapshot of current state (cleaned up, removed webkit.gyp modification)

You're right, the WebKit.gyp modification is really no longer necessary.

new, cleaned up patch.

[WebKit Commit Bot]

Comment on attachment 60619 [details]
patch - snapshot of current state (cleaned up, removed webkit.gyp modification)

Clearing flags on attachment: 60619

Committed r62545: <http://trac.webkit.org/changeset/62545>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62545 might have broken GTK Linux 64-bit Debug