Bug 110461

Summary: Web Inspector: [Protocol] Generate JS enum definitions.
Product: WebKit Reporter: Eugene Klyuchnikov <eustas>
Component: Web Inspector (Deprecated)Assignee: Eugene Klyuchnikov <eustas>
Status: RESOLVED FIXED    
Severity: Normal CC: aandrey, apavlov, graouts, joepeck, keishi, loislo, pfeldman, pmuellr, timothy, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Diff of generated files.
none
Patch
none
Difference in generated files. none

Description Eugene Klyuchnikov 2013-02-21 05:51:05 PST 
In protocol enums are defined in 3 ways:

1) As a type, like Page.ResourceType, currently 3 occurrences;
2) As type member, like Runtime.RemoteObject.type, 18 occurrences;
3) Directly in command parameters, like Page.getScriptExecutionStatus.type, 5 occurrences.

Generating corresponding type annotations would help to compiler to point errors like this:

this.responseReceived(requestId, frameId, loaderId, time, "Other" /* actually in should be "other" */, redirectResponse);
Comment 1 Eugene Klyuchnikov 2013-02-21 07:43:53 PST 
Created attachment 189527 [details]
Patch
Comment 2 Andrey Adaikin 2013-02-21 08:23:21 PST 
This is exiting!
Could you plz also attach diffs of the generated files as a patch?
Comment 3 Eugene Klyuchnikov 2013-02-21 22:21:22 PST 
(In reply to comment #2)
> This is exiting!
> Could you plz also attach diffs of the generated files as a patch?

Diff for protocol-externs.js: http://diffchecker.com/1l1aEUqu
Diff for InspectorBackendCommands.js: http://diffchecker.com/67s9Jkv5
Comment 4 Pavel Feldman 2013-02-22 00:25:03 PST 
Comment on attachment 189527 [details]
Patch

You should produce CamelCase enum literals per WebKit code guidelines.
Comment 5 Eugene Klyuchnikov 2013-02-22 01:02:44 PST 
Created attachment 189710 [details]
Patch
Comment 6 Pavel Feldman 2013-02-22 01:06:04 PST 
Could you attach the diff in the generated files as a patch to this bug?
Comment 7 Eugene Klyuchnikov 2013-02-22 01:06:30 PST 
Updated literal names. New diffs:

  protocol-externs.js: http://diffchecker.com/p0799b5q

  InspectorBackendCommands.js: http://www.diffchecker.com/69982Uz9
Comment 8 Pavel Feldman 2013-02-22 01:07:54 PST 
Could you attach the diff in the generated files as a patch to this bug?
Comment 9 Eugene Klyuchnikov 2013-02-22 01:29:34 PST 
Created attachment 189713 [details]
Diff of generated files.
Comment 10 Timothy Hatcher 2013-02-22 03:57:59 PST 
Comment on attachment 189713 [details]
Diff of generated files.

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

> new/InspectorBackendCommands.js:91
> +InspectorBackend.registerEnum("Console.ConsoleMessageSource", {Html: "html", Wml: "wml", Xml: "xml", Javascript: "javascript", Network: "network", ConsoleApi: "console-api", Other: "other"});

You should special case acronyms to make them all CAPS. Html => HTML, ConsoleApi => ConsoleAPI, etc.
Comment 11 Pavel Feldman 2013-02-22 07:27:03 PST 
Comment on attachment 189710 [details]
Patch

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

> Source/WebCore/inspector/front-end/InspectorBackend.js:85
> +        var domainAndMethod = type.split(".");

You need to call this from loadFromJSONIfNeeded as well.
Comment 12 Andrey Adaikin 2013-02-22 07:52:32 PST 
Comment on attachment 189710 [details]
Patch

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

> Source/WebCore/inspector/front-end/InspectorBackend.js:90
> +        this._initialized = true;

Is this correct? seems suspicious to have this line both in registerCommand and registerEnum
Comment 13 Eugene Klyuchnikov 2013-02-26 07:24:42 PST 
Created attachment 190278 [details]
Patch
Comment 14 Eugene Klyuchnikov 2013-02-26 07:27:10 PST 
Comment on attachment 189710 [details]
Patch

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

>> Source/WebCore/inspector/front-end/InspectorBackend.js:85
>> +        var domainAndMethod = type.split(".");
> 
> You need to call this from loadFromJSONIfNeeded as well.

Done.

>> Source/WebCore/inspector/front-end/InspectorBackend.js:90
>> +        this._initialized = true;
> 
> Is this correct? seems suspicious to have this line both in registerCommand and registerEnum

As I see, this marker is used do avoid recursion/repetitive loading. So, all 3 methods should mark instance as initialized.
Comment 15 Eugene Klyuchnikov 2013-02-26 07:27:53 PST 
Comment on attachment 189713 [details]
Diff of generated files.

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

>> new/InspectorBackendCommands.js:91
>> +InspectorBackend.registerEnum("Console.ConsoleMessageSource", {Html: "html", Wml: "wml", Xml: "xml", Javascript: "javascript", Network: "network", ConsoleApi: "console-api", Other: "other"});
> 
> You should special case acronyms to make them all CAPS. Html => HTML, ConsoleApi => ConsoleAPI, etc.

Fixed
Comment 16 Eugene Klyuchnikov 2013-02-26 07:31:55 PST 
Created attachment 190280 [details]
Difference in generated files.
Comment 17 WebKit Review Bot 2013-02-27 23:52:17 PST 
Comment on attachment 190278 [details]
Patch

Clearing flags on attachment: 190278

Committed r144265: <http://trac.webkit.org/changeset/144265>
Comment 18 WebKit Review Bot 2013-02-27 23:52:22 PST 
All reviewed patches have been landed.  Closing bug.