Bug 40558

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

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.

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

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.

Comment on attachment 59084 [details] patch - Upstreamed DevTools for Chromium DRT r- based on comments from tkent.

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

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

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

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.

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.

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

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

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.

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.

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.

Created attachment 60497 [details] patch - snapshot of current state (removed unnecessary struct) Patch: removed unnecessary struct 'TestShellWeakReference' (also, resubmitted to use commit-queue)

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

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.

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.

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.

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 @@ > +2010-07-05 Roland Steiner <rolandsteiner@chromium.org> > + > + 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 <ossy@webkit.org> > > 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 <paroga@paroga.com> 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?

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.

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.

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>

All reviewed patches have been landed. Closing bug.

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