Bug 44338

Summary: Web Inspector: inspector protocol should be switched from array to object based representation.
Product: WebKit Reporter: Ilya Tikhonovsky <loislo>
Component: Web Inspector (Deprecated)Assignee: Ilya Tikhonovsky <loislo>
Status: RESOLVED FIXED    
Severity: Normal CC: bweinstein, dglazkov, eric, gns, joepeck, keishi, pfeldman, pmuellr, rik, timothy, webkit-ews, webkit.review.bot, xan.lopez, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 44416    
Bug Blocks:    
Attachments:
Description Flags
[patch] initial version.
none
sample: InspectorBackendDispatcher.cpp
none
sample: InspectorBackendStub.js
none
sample: RemoteInspectorFrontend.cpp
none
[patch] second iteration
none
sample: InspectorBackendStub.js
none
[patch] second iteration. Rebaselined.
yurys: review+
actually landed none

Description Ilya Tikhonovsky 2010-08-20 09:02:30 PDT
%subj%
Comment 1 Ilya Tikhonovsky 2010-08-20 12:31:06 PDT
Created attachment 64973 [details]
[patch] initial version.
Comment 2 Joseph Pecoraro 2010-08-20 12:48:35 PDT
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: ===
Comment 3 Ilya Tikhonovsky 2010-08-20 13:20:16 PDT
Created attachment 64978 [details]
sample: InspectorBackendDispatcher.cpp
Comment 4 Ilya Tikhonovsky 2010-08-20 13:20:56 PDT
Created attachment 64979 [details]
sample: InspectorBackendStub.js
Comment 5 Ilya Tikhonovsky 2010-08-20 13:21:31 PDT
Created attachment 64980 [details]
sample: RemoteInspectorFrontend.cpp
Comment 6 Ilya Tikhonovsky 2010-08-20 13:25:39 PDT
(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: ===
Comment 7 WebKit Review Bot 2010-08-20 14:36:37 PDT
Attachment 64973 [details] did not build on win:
Build output: http://queues.webkit.org/results/3742425
Comment 8 Yury Semikhatsky 2010-08-22 23:25:47 PDT
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.
Comment 9 Ilya Tikhonovsky 2010-08-23 03:18:15 PDT
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.
Comment 10 Ilya Tikhonovsky 2010-08-23 03:19:11 PDT
Created attachment 65097 [details]
sample: InspectorBackendStub.js
Comment 11 Eric Seidel (no email) 2010-08-23 03:26:13 PDT
Attachment 65096 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3728546
Comment 12 Early Warning System Bot 2010-08-23 03:27:27 PDT
Attachment 65096 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3789453
Comment 13 WebKit Review Bot 2010-08-23 04:44:29 PDT
Attachment 65096 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3775586
Comment 14 Ilya Tikhonovsky 2010-08-23 04:58:00 PDT
Created attachment 65103 [details]
[patch] second iteration. Rebaselined.
Comment 15 Yury Semikhatsky 2010-08-23 05:06:41 PDT
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
Comment 16 WebKit Review Bot 2010-08-23 05:13:01 PDT
Attachment 65096 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/3787410
Comment 17 Ilya Tikhonovsky 2010-08-23 05:15:04 PDT
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)
Comment 18 Ilya Tikhonovsky 2010-08-23 07:21:14 PDT
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)
Comment 19 Ilya Tikhonovsky 2010-08-23 07:38:21 PDT
Created attachment 65117 [details]
actually landed