Bug 34326 – [Chromium] Upstream the rest of DevTools WebKit API implementation

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit API

[Yury Semikhatsky]

There are some parts of DevTools WebKit API implementation that are not specific to Chromium architecture but still live in the Chromium repository. We should migrate them to WebKit.

[Yury Semikhatsky]

Created attachment 47707 [details]
patch

Add API methods for loading injected script and dispatch script sources from resource bundle.

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebDevToolsAgentClient.h:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1


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

[Pavel Feldman]

Comment on attachment 47707 [details]
patch

Please fix style.

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/public/WebDevToolsAgentClient.h
Committed r54057

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 47707 [details])
> Please fix style.
Done.

[Yury Semikhatsky]

Created attachment 47842 [details]
patch

[WebKit Review Bot]

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

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
Last 3072 characters of output:
h:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ToolsAgent.h:61:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ToolsAgent.h:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ToolsAgent.h:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ToolsAgent.h:65:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/WebDevToolsAgentImpl.h:43:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/src/WebDevToolsFrontendImpl.h:46:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/src/ProfilerAgentImpl.cpp:38:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/ProfilerAgentImpl.cpp:39:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ProfilerAgentImpl.cpp:42:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/ProfilerAgentImpl.cpp:43:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ProfilerAgentImpl.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ProfilerAgentImpl.cpp:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ProfilerAgentImpl.cpp:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/ProfilerAgentImpl.cpp:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/DebuggerAgentManager.h:40:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/src/DebuggerAgent.h:39:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/DebuggerAgent.h:40:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/DebuggerAgent.h:45:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/DebuggerAgent.h:46:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/DebuggerAgent.h:47:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/DebuggerAgent.h:48:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
WebKit/chromium/src/DevToolsRPC.h:86:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 76


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

[WebKit Review Bot]

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

[Pavel Feldman]

Comment on attachment 47842 [details]
patch

Bunch of style nits, other than that r+.

> +                'src/DebuggerAgent.h',
> +		'src/DebuggerAgentImpl.cpp',
> +		'src/DebuggerAgentImpl.h',
> +		'src/DebuggerAgentManager.cpp',
> +		'src/DebuggerAgentManager.h',
> +		'src/DevToolsRPC.h',
> +		'src/DevToolsRPCJS.h',

Indent.


> +
> +#define APU_AGENT_DELEGATE_STRUCT(METHOD0, METHOD1, METHOD2, METHOD3, MEHTOD4, METHOD5) \
> +  /* Sends a json object to apu. */ \
> +  METHOD1(dispatchToApu, String /* data */)
> +

4 space indent?

> +
> +    v8::Handle<v8::Value> args[] = {
> +      functionNameWrapper,
> +      jsonArgsWrapper,
> +      callIdWrapper
> +    };

4 space indent?

> +    v8::Handle<v8::Value> args[] = {
> +      v8::Local<v8::Value>()

Indent.

> +WebCore::Page* DebuggerAgentImpl::page()
> +{
> +  return m_webViewImpl->page();
> +}

Indent.


> +
> +#include "webkit/glue/glue_util.h"
> +#include "webkit/glue/webkit_glue.h"
> +


Does this compile?

> +    virtual void dispatchOnInjectedScript(
> +        int callId,
> +        int injectedScriptId,
> +        const WebCore::String& functionName,
> +        const WebCore::String& jsonArgs,
> +        bool async);

Use single line here in and cpp.


> +WebDevToolsFrontend* WebDevToolsFrontend::create(
> +    WebView* view,
> +    WebDevToolsFrontendClient* client,
> +    const WebString& applicationLocale)
> +{
> +    return new WebDevToolsFrontendImpl(
> +      static_cast<WebViewImpl*>(view),
> +      client,
> +      applicationLocale);
> +}

Why wrapping?

[Yury Semikhatsky]

Created attachment 47911 [details]
patch

All the comments have been addressed. Style errors fixed.

[Yury Semikhatsky]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/WebKit.gyp
	A	WebKit/chromium/src/APUAgentDelegate.h
	A	WebKit/chromium/src/BoundObject.cpp
	A	WebKit/chromium/src/BoundObject.h
	A	WebKit/chromium/src/DebuggerAgent.h
	A	WebKit/chromium/src/DebuggerAgentImpl.cpp
	A	WebKit/chromium/src/DebuggerAgentImpl.h
	A	WebKit/chromium/src/DebuggerAgentManager.cpp
	A	WebKit/chromium/src/DebuggerAgentManager.h
	A	WebKit/chromium/src/DevToolsRPC.h
	A	WebKit/chromium/src/DevToolsRPCJS.h
	A	WebKit/chromium/src/ProfilerAgent.h
	A	WebKit/chromium/src/ProfilerAgentImpl.cpp
	A	WebKit/chromium/src/ProfilerAgentImpl.h
	A	WebKit/chromium/src/ToolsAgent.h
	A	WebKit/chromium/src/WebDevToolsAgentImpl.cpp
	A	WebKit/chromium/src/WebDevToolsAgentImpl.h
	A	WebKit/chromium/src/WebDevToolsFrontendImpl.cpp
	A	WebKit/chromium/src/WebDevToolsFrontendImpl.h
Committed r54232

[Eric Seidel (no email)]

Looks like this was landed.  Closing bug.

[Eric Seidel (no email)]

Consider trying "webkit-patch" for landing your bugs.  That tool will automatically update/close bugs for you.

See "webkit-patch --help"

[Yury Semikhatsky]

(In reply to comment #13)
> Consider trying "webkit-patch" for landing your bugs.  That tool will
> automatically update/close bugs for you.
> 
> See "webkit-patch --help"
 Thaks for the suggestion, will try. But this particular bug was not closed intentionally since there are some pieces of devtools implemented in JavaScript that are to be upstreamed. It might be a good idea to file a separate bug for them though.

[Eric Seidel (no email)]

One bug per change is generally preferred, yes.