Bug 42588 – Web Inspector: CodeGeneratorInspector should be able to generate Backend part of Inspector interface.

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

[Ilya Tikhonovsky]

After big discussion with Pavel it was decided to keep original InspectorBackend
functionality as is, and generate separate class RemoteInspectorBackend
for parsing transfered arguments and pass these arguments to the current
implementation of InspectorBackend. This implementation will be an adoptation layer
between Remote Debugging interface, which should be stable for 
customers, and flexible internal implementation of that interface.

[Pavel Feldman]

The name RemoteInspectorBackend sounds confusing to me. How can backend be remote wrt WebCore. I'd call the new class InspectorBackendDispatcher. It might be created with backend instance in the constructor and have single dispatch(String) method.

[Joseph Pecoraro]

(In reply to comment #1)
> The name RemoteInspectorBackend sounds confusing to me. How can backend be
> remote wrt WebCore. I'd call the new class InspectorBackendDispatcher. It might
> be created with backend instance in the constructor and have single
> dispatch(String) method.

+1, I like that.

[Ilya Tikhonovsky]

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

[Ilya Tikhonovsky]

Created attachment 62279 [details]
sample: InspectorBackendDispatcher.cpp

[Ilya Tikhonovsky]

Created attachment 62280 [details]
sample: InspectorBackendDispatcher.h

[Yury Semikhatsky]

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

WebCore/GNUmakefile.am:553
 +  	DerivedSources/WebCore/InspectorBackendDispatcher.cpp \
mind alphabetic order

WebCore/WebCore.gyp/WebCore.gyp:481
 +              '../inspector/CodeGeneratorInspector.pm',
Could you add a comment for this?

WebCore/WebCore.gypi: 
 +          'webcore_inspector_idl_files': [
is it referenced from other places?

WebCore/inspector/CodeGeneratorInspector.pm:168
 +      push(@backendHead, "    ${backendClassName}* create(InspectorBackend* backend) { return new ${backendClassName}(backend); }");
either remote create or make the constructor private

WebCore/inspector/CodeGeneratorInspector.pm:241
 +      push(@function, "void ${backendClassName}::${functionName}(PassRefPtr<InspectorArray>" . ( scalar(@argsFiltered) ? " args)" : ")"));
Please add comment about "unused argument" warning/

WebCore/inspector/CodeGeneratorInspector.pm:248
 +              $type = "double";
can you take it from the type description?

WebCore/inspector/CodeGeneratorInspector.pm:271
 +      my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods);
Is explicit check really needed here?

WebCore/inspector/CodeGeneratorInspector.pm:271
 +      my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods);
I think calling .set( would be more appropriate here.

WebCore/inspector/front-end/ElementsPanel.js:225
 +              InspectorBackend.pushNodeByPathToFrontend(callId, this._selectedPathOnReset.join(","));
We should eventually pass an array instead of string here.

WebKit/chromium/src/js/InspectorControllerImpl.js:157
 +      args.unshift(methodName);
just pass 2 as the second parameter above.

[Yury Semikhatsky]

All (In reply to comment #5)
> Created an attachment (id=62280) [details]
> sample: InspectorBackendDispatcher.h

All methods but constructor and dispatch should be private.

[Ilya Tikhonovsky]

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

WebCore/GNUmakefile.am:553
 +      DerivedSources/WebCore/InspectorBackendDispatcher.cpp \
mind alphabetic order

fixed.



WebCore/WebCore.gyp/WebCore.gyp:481
 +              '../inspector/CodeGeneratorInspector.pm',
Could you add a comment for this?

done.


WebCore/WebCore.gypi: 
 +          'webcore_inspector_idl_files': [
is it referenced from other places?

no  other references.



WebCore/inspector/CodeGeneratorInspector.pm:168
 +      push(@backendHead, "    ${backendClassName}* create(InspectorBackend* backend) { return new ${backendClassName}(backend); }");
either remote create or make the constructor private

removed.

WebCore/inspector/CodeGeneratorInspector.pm:241
 +      push(@function, "void ${backendClassName}::${functionName}(PassRefPtr<InspectorArray>" . ( scalar(@argsFiltered) ? " args)" : ")"));
Please add comment about "unused argument" warning/

done.


WebCore/inspector/CodeGeneratorInspector.pm:248
 +              $type = "double";
can you take it from the type description?

fixed.


WebCore/inspector/CodeGeneratorInspector.pm:271
 +      my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods);
Is explicit check really needed here?

removed.


WebCore/inspector/CodeGeneratorInspector.pm:271
 +      my @mapEntries = map("dispatchMap.add(String(\"$_\"), &${backendClassName}::$_);", @methods);
I think calling .set( would be more appropriate here.

all these entries are assigning at the first run.


WebCore/inspector/front-end/ElementsPanel.js:225
 +              InspectorBackend.pushNodeByPathToFrontend(callId, this._selectedPathOnReset.join(","));
We should eventually pass an array instead of string here.

In generally it is possible but other platforms are not migrated yet.



> All methods but constructor and dispatch should be private.

done

[Eric Seidel (no email)]

Attachment 62298 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3577349

[Ilya Tikhonovsky]

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

x64 compile error was fixed.

[Ilya Tikhonovsky]

*** Bug 40167 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

Comment on attachment 62301 [details]
[patch] third iteration.

Excellent! This is just what I thought it would look like after
talking with Pavel. You did it very well, with nice error messages.
My usual minor comments below.


> WebCore/WebCore.gyp/WebCore.gyp
> 479           # The second input item will be used as item name in vcproj.
> 480           # It is not possible to put there Inspector.idl because
> 481           # all idl files are marking as excluded by gyp generator.

This could be reworded. "It is not possible to put Inspector.idl there".


>      InspectorBackend* inspectorBackend() { return m_inspectorBackend.get(); }
> +    InspectorBackendDispatcher* inspectorBackendDispatcher() { return m_inspectorBackendDispatcher.get(); }
>      InjectedScriptHost* injectedScriptHost() { return m_injectedScriptHost.get(); }

These could all use "const". I think I mentioned this before.
No big deal.


> +++ b/WebCore/inspector/InspectorValues.cpp

> +    *output = (long)m_doubleValue;
> +    *output = (unsigned long)m_doubleValue;

Should these have used static_cast<...>(...)?


> *exception = \"Error: Invalid message format. The message should be jsonified array of arguments.\";");

I would say "The message should be a JSONified array of arguments.",
Because JSON was capitalized before, but mostly the "a" in front
of JSONified.

[Ilya Tikhonovsky]

Committed r63952
	M	WebKit/chromium/ChangeLog
	M	WebKit/chromium/src/WebDevToolsAgentImpl.cpp
	M	WebKit/chromium/src/js/InspectorControllerImpl.js
	M	WebKit/chromium/src/WebDevToolsAgentImpl.h
	M	WebKit/chromium/src/ToolsAgent.h
	M	WebCore/WebCore.pri
	M	WebCore/ChangeLog
	M	WebCore/WebCore.vcproj/WebCore.vcproj
	M	WebCore/GNUmakefile.am
	M	WebCore/WebCore.gyp/WebCore.gyp
	M	WebCore/WebCore.gypi
	M	WebCore/inspector/CodeGeneratorInspector.pm
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorValues.h
	M	WebCore/inspector/front-end/ElementsPanel.js
	M	WebCore/inspector/InspectorController.h
	M	WebCore/inspector/InspectorValues.cpp
	M	WebCore/WebCore.xcodeproj/project.pbxproj
r63952 = d923b0a72242f6f9ae060d20a3e391f6ed5da461 (refs/remotes/trunk)

[Ilya Tikhonovsky]

Created attachment 62394 [details]
really landed