[webkit-reviews] review requested: [Bug 42588] Web Inspector: CodeGeneratorInspector should be able to generate Backend part of Inspector interface. : [Attachment 62298] [patch] second iteration

[bugzilla-daemon at webkit.org]

Ilya Tikhonovsky <loislo at chromium.org> has asked  for review:
Bug 42588: Web Inspector: CodeGeneratorInspector should be able to generate
Backend part of Inspector interface.
https://bugs.webkit.org/show_bug.cgi?id=42588

Attachment 62298: [patch] second iteration
https://bugs.webkit.org/attachment.cgi?id=62298&action=review

------- Additional Comments from Ilya Tikhonovsky <loislo at chromium.org>
(From update of attachment 62278 [details])
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