Bug 42983 – Web Inspector: migrate WebCore from InspectorBackend binding to InspectorBackendDispatcher

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

Created attachment 62589 [details]
[patch] initial version.

[Joseph Pecoraro]

Comment on attachment 62589 [details]
[patch] initial version.

> +++ b/WebCore/inspector/Inspector.idl
> +        [notify] void didDispatch(out long callId, out String e);

WebKit typically doesn't abbreviate variable names.
"e" should be "exception".

The rest of this looked good to me =)

[Ilya Tikhonovsky]

(In reply to comment #2)
> (From update of attachment 62589 [details])
> > +++ b/WebCore/inspector/Inspector.idl
> > +        [notify] void didDispatch(out long callId, out String e);
> 
> WebKit typically doesn't abbreviate variable names.
> "e" should be "exception".

Unfortunately exception is a reserved word of IDLParser.

> 
> The rest of this looked good to me =)

[Ilya Tikhonovsky]

Created attachment 62595 [details]
[patch] second iteration. fixed windows build.

[Joseph Pecoraro]

> > WebKit typically doesn't abbreviate variable names.
> > "e" should be "exception".
> 
> Unfortunately exception is a reserved word of IDLParser.

How about "exceptionMessage" or "exceptionString" than? =)

[Pavel Feldman]

Comment on attachment 62595 [details]
[patch] second iteration. fixed windows build.

WebCore/inspector/Inspector.idl:42
 +          [notify] void didDispatch(out long callId, out String e);
Also, we should not have a generic ack / failure reporting. Each of the backend methods should ack (either with the results or with failure reporting). The protocol should say that request with type 'foo' and sequence number 'bar' has failed.

WebCore/inspector/InspectorController.cpp:2210
 +      m_inspectorBackendDispatcher->dispatch(message, &exception);
Nit: should this guy be static and receive backend instance as an argument to emphasize its stateless nature?

WebCore/inspector/InspectorController.h:125
 +      void dispatchOnBackend(long callId, const String& message);
This should be dispatchOnBackend(const String& message). We should serialize all the call data into a single message.

WebCore/inspector/front-end/InspectorBackendStub.js:35
 +      this._registerDelegate("addInspectedNode");
Can you generate these?

WebCore/inspector/front-end/InspectorBackendStub.js:124
 +          var callbackId = WebInspector.Callback.wrap(function(){});
You don't need this artificial call id / empty response callback anymore. Just send the message.

WebKit/chromium/src/InspectorFrontendClientImpl.cpp:132
 +      WebVector<WebString> args((size_t)2);
We only allow static_cast<> in such places.

WebKit/chromium/src/InspectorFrontendClientImpl.cpp:136
 +      m_client->sendMessageToAgent(messageData);
I'd introduce new sendWebKitMessageToBackend(string) and use it here. It will eventually become the main transport for the interaction.

[Pavel Feldman]

I understand that this patch is growing and you'd like to land it. Please address everything in the follow up patches, but following I'd like to see fixed prior to landing this one:

> WebCore/inspector/Inspector.idl:42
>  +          [notify] void didDispatch(out long callId, out String e);
> Also, we should not have a generic ack / failure reporting. Each of the backend methods should ack (either with the results or with failure reporting). The protocol should say that request with type 'foo' and sequence number 'bar' has failed.

Please fix this ^^.

> WebCore/inspector/InspectorController.h:125
>  +      void dispatchOnBackend(long callId, const String& message);
> This should be dispatchOnBackend(const String& message). We should serialize all the call data into a single message.

Please fix this ^^.

> WebCore/inspector/front-end/InspectorBackendStub.js:124
>  +          var callbackId = WebInspector.Callback.wrap(function(){});
> You don't need this artificial call id / empty response callback anymore. Just send the message.

Please fix this ^^.

> WebKit/chromium/src/InspectorFrontendClientImpl.cpp:132
>  +      WebVector<WebString> args((size_t)2);
> We only allow static_cast<> in such places.

Please fix this ^^.

[Ilya Tikhonovsky]

Created attachment 62675 [details]
[patch] third iteration.

generic callId and related things were removed.

[Ilya Tikhonovsky]

Committed r64124
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/WebDevToolsAgentImpl.cpp
	M	WebKit/chromium/src/InspectorFrontendClientImpl.cpp
	D	WebKit/chromium/src/js/InspectorControllerImpl.js
	M	WebKit/chromium/src/WebDevToolsAgentImpl.h
	M	WebKit/chromium/src/ToolsAgent.h
	M	WebKit/chromium/src/InspectorFrontendClientImpl.h
	M	WebKit/chromium/WebKit.gypi
	M	WebCore/WebCore.exp.in
	M	WebCore/DerivedSources.cpp
	M	WebCore/WebCore.pri
	M	WebCore/Android.derived.v8bindings.mk
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gypi
	M	WebCore/bindings/js/ScriptObject.h
	M	WebCore/bindings/js/ScriptObject.cpp
	M	WebCore/bindings/v8/ScriptObject.h
	M	WebCore/bindings/v8/ScriptObject.cpp
	M	WebCore/DerivedSources.make
	D	WebCore/inspector/InspectorBackend.idl
	M	WebCore/inspector/CodeGeneratorInspector.pm
	M	WebCore/inspector/InspectorFrontendHost.cpp
	M	WebCore/inspector/InspectorFrontendClient.h
	M	WebCore/inspector/InspectorFrontendClientLocal.cpp
	M	WebCore/inspector/InspectorFrontendHost.h
	M	WebCore/inspector/front-end/InspectorBackendStub.js
	M	WebCore/inspector/InspectorFrontendClientLocal.h
	M	WebCore/inspector/InspectorFrontendHost.idl
	M	WebCore/CMakeLists.txt
	M	WebCore/WebCore.xcodeproj/project.pbxproj
W: -empty_dir: trunk/WebCore/inspector/InspectorBackend.idl
W: -empty_dir: trunk/WebKit/chromium/src/js/InspectorControllerImpl.js
r64124 = a471a4cc0749dddd35a779ad3abff89150aa6cce (refs/remotes/trunk)