Bug 44338 – Web Inspector: inspector protocol should be switched from array to object based representation.

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

[Ilya Tikhonovsky]

%subj%

[Ilya Tikhonovsky]

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

[Joseph Pecoraro]

Ilya, could you attach a generated version of InspectorBackendStub after you patch?
Some minor comments after a glance.

WebCore/inspector/CodeGeneratorInspector.pm:527
> +          this[command] = this.sendMessageToBackend.bind(this, commandInfo);
I think _registerDelegate should just take a single argument, commandInfo, and "command" is just "commandInfo.command".

WebCore/inspector/CodeGeneratorInspector.pm:540
> +          if (args.length == 1 && typeof args[0] === "function")
Nit: ===

[Ilya Tikhonovsky]

Created attachment 64978 [details]
sample: InspectorBackendDispatcher.cpp

[Ilya Tikhonovsky]

Created attachment 64979 [details]
sample: InspectorBackendStub.js

[Ilya Tikhonovsky]

Created attachment 64980 [details]
sample: RemoteInspectorFrontend.cpp

[Ilya Tikhonovsky]

(In reply to comment #2)
> Ilya, could you attach a generated version of InspectorBackendStub after you patch?
> Some minor comments after a glance.

done


> WebCore/inspector/CodeGeneratorInspector.pm:527
> > +          this[command] = this.sendMessageToBackend.bind(this, commandInfo);
> I think _registerDelegate should just take a single argument, commandInfo, and "command" is just "commandInfo.command".


> WebCore/inspector/CodeGeneratorInspector.pm:540
> > +          if (args.length == 1 && typeof args[0] === "function")
> Nit: ===

[WebKit Review Bot]

Attachment 64973 [details] did not build on win:
Build output: http://queues.webkit.org/results/3742425

[Yury Semikhatsky]

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

WebCore/inspector/CodeGeneratorInspector.pm:295
 +      push(@function, "    RefPtr<InspectorObject> requestMessageObject(requestMessageObjectRef);");
This is not needed.

WebCore/inspector/CodeGeneratorInspector.pm:511
 +              "\\\"arguments\\\": {$argumentNames}" .
Unrelated to this issue, but it would be nice if we didn't escape quotes in the JSON strings in InspectorBackendStub.js but use single quotes instead( "\"property\"" -> '"property"')

WebCore/inspector/front-end/inspector.js:637
 +      WebInspector.log("Error: InspectorBackend." + message.command + " failed.");
These messages should go into console.error not the Inspector's console.

WebCore/inspector/CodeGeneratorInspector.pm:354
 +      push(@function, "        responseMessage->setString(\"type\", \"response\");");
It might make sense to extract these string constants and reuse them.

[Ilya Tikhonovsky]

Created attachment 65096 [details]
[patch] second iteration


> WebCore/inspector/CodeGeneratorInspector.pm:527
> > +          this[command] = this.sendMessageToBackend.bind(this, commandInfo);
> I think _registerDelegate should just take a single argument, commandInfo, and "command" is just "commandInfo.command".

done.

> WebCore/inspector/CodeGeneratorInspector.pm:540
> > +          if (args.length == 1 && typeof args[0] === "function")
> Nit: ===

done.

> WebCore/inspector/CodeGeneratorInspector.pm:295
>  +      push(@function, "    RefPtr<InspectorObject> requestMessageObject(requestMessageObjectRef);");
> This is not needed.

done

> WebCore/inspector/CodeGeneratorInspector.pm:511
> +              "\\\"arguments\\\": {$argumentNames}" .
> Unrelated to this issue, but it would be nice if we didn't escape quotes in the JSON strings in InspectorBackendStub.js but use single quotes instead( "\"property\"" -> '"property"')

done

> WebCore/inspector/front-end/inspector.js:637
> +      WebInspector.log("Error: InspectorBackend." + message.command + " failed.");
> These messages should go into console.error not the Inspector's console.

done

> WebCore/inspector/CodeGeneratorInspector.pm:354
> +      push(@function, "        responseMessage->setString(\"type\", \"response\");");
> It might make sense to extract these string constants and reuse them.

done


InspectorFrontendClientLocal::setAttachedWindow was fixed.
WebInspector_syncDispatch was fixed.

[Ilya Tikhonovsky]

Created attachment 65097 [details]
sample: InspectorBackendStub.js

[Eric Seidel (no email)]

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

[Early Warning System Bot]

Attachment 65096 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3789453

[WebKit Review Bot]

Attachment 65096 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3775586

[Ilya Tikhonovsky]

Created attachment 65103 [details]
[patch] second iteration. Rebaselined.

[Yury Semikhatsky]

Comment on attachment 65103 [details]
[patch] second iteration. Rebaselined.

WebCore/inspector/front-end/inspector.js:638
 +      console.log("Error: InspectorBackend." + messageObject.command + " failed.");
console.log -> console.error

[WebKit Review Bot]

Attachment 65096 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3787410

[Ilya Tikhonovsky]

Committed r65803
	M	WebCore/ChangeLog
	M	WebCore/inspector/CodeGeneratorInspector.pm
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorFrontendClientLocal.cpp
	M	WebCore/inspector/InspectorValues.h
	M	WebCore/inspector/Inspector.idl
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/Callback.js
	M	WebCore/inspector/InspectorController.h
r65803 = f2ffcc11e3506172e312f7f20d457460ec35a38c (refs/remotes/trunk)

[Ilya Tikhonovsky]

With fixed windows build

Committed r65809
	M	WebCore/ChangeLog
	M	WebCore/inspector/CodeGeneratorInspector.pm
	M	WebCore/inspector/InspectorController.cpp
	M	WebCore/inspector/InspectorFrontendClientLocal.cpp
	M	WebCore/inspector/InspectorValues.h
	M	WebCore/inspector/Inspector.idl
	M	WebCore/inspector/front-end/inspector.js
	M	WebCore/inspector/front-end/Callback.js
	M	WebCore/inspector/InspectorController.h
r65809 = 12044a8f1a0b8a82f217ae39411f717045555612 (refs/remotes/trunk)

[Ilya Tikhonovsky]

Created attachment 65117 [details]
actually landed