Bug 57957 - Web Inspector: migrate Inspector protocol messages format to JSON-RPC
Summary: Web Inspector: migrate Inspector protocol messages format to JSON-RPC
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Ilya Tikhonovsky
URL: http://groups.google.com/group/json-r...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-06 09:43 PDT by Ilya Tikhonovsky
Modified: 2011-04-07 07:17 PDT (History)
11 users (show)

See Also:


Attachments
errors sample (2.04 KB, text/plain)
2011-04-06 09:43 PDT, Ilya Tikhonovsky
no flags Details
protocol messages sample (4.12 KB, text/plain)
2011-04-06 09:44 PDT, Ilya Tikhonovsky
no flags Details
InspectorBackendDispatcher.cpp sample (159.44 KB, text/x-c++src)
2011-04-06 09:47 PDT, Ilya Tikhonovsky
no flags Details
InspectorBackendStub.js (19.99 KB, application/javascript)
2011-04-06 09:48 PDT, Ilya Tikhonovsky
no flags Details
[patch] initial version (44.26 KB, patch)
2011-04-06 09:49 PDT, Ilya Tikhonovsky
no flags Details | Formatted Diff | Diff
[patch] initial version with fixed ChangeLogs (44.37 KB, patch)
2011-04-06 09:53 PDT, Ilya Tikhonovsky
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ilya Tikhonovsky 2011-04-06 09:43:31 PDT
Created attachment 88449 [details]
errors sample

There is not a significant difference between  inspector messages spec and and JSON-RPC 2.0 messages spec.
Also JSON-RPC has a pretty clear specification for error descriptions which we haven't.
It was decided that we will use it.

the list of renames:

1) type-> /dev/null
2) domain + '.' + event => method // for events
3) domain + '.' + command => method // for requests
4) requestId => id // for responses

5) arguments => params // for requests
6) data => params // for events 
7) body => result // for responses

protocolErrors and error properties will be converted to JSON-RPC error format.
The order of properties in messages also will be adjusted.
The only thing that looks unnecessary is jsonrpc property.
Comment 1 Ilya Tikhonovsky 2011-04-06 09:44:12 PDT
Created attachment 88450 [details]
protocol messages sample
Comment 2 Ilya Tikhonovsky 2011-04-06 09:47:46 PDT
Created attachment 88452 [details]
InspectorBackendDispatcher.cpp sample
Comment 3 Ilya Tikhonovsky 2011-04-06 09:48:13 PDT
Created attachment 88453 [details]
InspectorBackendStub.js
Comment 4 Ilya Tikhonovsky 2011-04-06 09:49:22 PDT
Created attachment 88455 [details]
[patch] initial version
Comment 5 WebKit Review Bot 2011-04-06 09:50:55 PDT
Attachment 88455 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style']" exit_code: 1

LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 2 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Ilya Tikhonovsky 2011-04-06 09:53:31 PDT
Created attachment 88457 [details]
[patch] initial version with fixed ChangeLogs
Comment 7 Yury Semikhatsky 2011-04-06 11:15:54 PDT
Comment on attachment 88457 [details]
[patch] initial version with fixed ChangeLogs

View in context: https://bugs.webkit.org/attachment.cgi?id=88457&action=review

> LayoutTests/inspector/report-protocol-errors-expected.txt:37
> +        data : {

Shouldn't the data be an array instead of object with properties 0, 1, 2,... ?

> Source/WebCore/inspector/CodeGeneratorInspector.pm:508
> +        commonErrors.insert(ParseError, "{\\\"code\\\":-32700,\\\"message\\\":\\\"Parse error.\\\"}");

Would be nice to have a link to the spec defining all these constants.
Comment 8 Pavel Feldman 2011-04-06 11:24:46 PDT
Comment on attachment 88457 [details]
[patch] initial version with fixed ChangeLogs

Note that this needs coordinated landing.
Comment 9 Ilya Tikhonovsky 2011-04-06 11:46:08 PDT
(In reply to comment #7)
> (From update of attachment 88457 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88457&action=review
> 
> > LayoutTests/inspector/report-protocol-errors-expected.txt:37
> > +        data : {
> 
> Shouldn't the data be an array instead of object with properties 0, 1, 2,... ?

Really it is the array but there is a bug in InspectorTest.addObject. 
As far as such fix will affect a number of expectations it'd be better do do that in another patch.

> 
> > Source/WebCore/inspector/CodeGeneratorInspector.pm:508
> > +        commonErrors.insert(ParseError, "{\\\"code\\\":-32700,\\\"message\\\":\\\"Parse error.\\\"}");
> 
> Would be nice to have a link to the spec defining all these constants.

all these codes are described in the the spec. 
You can see the url to it in URL property of the bug. 

http://groups.google.com/group/json-rpc/web/json-rpc-2-0
Comment 10 WebKit Review Bot 2011-04-06 21:49:40 PDT
Attachment 88457 [details] did not build on mac:
Build output: http://queues.webkit.org/results/8341528
Comment 11 Yury Semikhatsky 2011-04-07 06:27:12 PDT
Comment on attachment 88457 [details]
[patch] initial version with fixed ChangeLogs

View in context: https://bugs.webkit.org/attachment.cgi?id=88457&action=review

> Source/WebCore/inspector/CodeGeneratorInspector.pm:966
> +    push(@backendHead, "        ParseError = 0,");

Why not use same constant values as defined in the spec?
Comment 12 Yury Semikhatsky 2011-04-07 06:28:35 PDT
(In reply to comment #9)
> (In reply to comment #7)
> > (From update of attachment 88457 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=88457&action=review
> > 
> > > LayoutTests/inspector/report-protocol-errors-expected.txt:37
> > > +        data : {
> > 
> > Shouldn't the data be an array instead of object with properties 0, 1, 2,... ?
> 
> Really it is the array but there is a bug in InspectorTest.addObject. 
> As far as such fix will affect a number of expectations it'd be better do do that in another patch.
> 
Please file a bug on this.

> > 
> > > Source/WebCore/inspector/CodeGeneratorInspector.pm:508
> > > +        commonErrors.insert(ParseError, "{\\\"code\\\":-32700,\\\"message\\\":\\\"Parse error.\\\"}");
> > 
> > Would be nice to have a link to the spec defining all these constants.
> 
> all these codes are described in the the spec. 
> You can see the url to it in URL property of the bug. 
> 
> http://groups.google.com/group/json-rpc/web/json-rpc-2-0
I meant putting the URL into code so that it's easier to find the spec when you are reading the code.
Comment 13 Ilya Tikhonovsky 2011-04-07 07:17:15 PDT
Committed r83171: <http://trac.webkit.org/changeset/83171>