Bug 43970 – Web Inspector: upstream frontend-side WebSocket transport.

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

[Pavel Feldman]

Chromium already has an alternate WebSocket-based communication channel with the backend. Upstreaming it in this change. We will agree on the URI of the remote service as the protocol matures.

[Pavel Feldman]

Created attachment 64338 [details]
[PATCH] Proposed change.

[Yury Semikhatsky]

Comment on attachment 64338 [details]
[PATCH] Proposed change.

WebCore/inspector/front-end/inspector.js:434
 +  var queryParams = window.location.search;
Please wrap it into an anonymous function to avoid cluttering global context.

WebCore/inspector/front-end/inspector.js:445
 +      WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); }
Why not do this unconditionally after WebInspector has been loaded?

[Joseph Pecoraro]

Comment on attachment 64338 [details]
[PATCH] Proposed change.

> +++ b/WebCore/inspector/front-end/inspector.js
> +    WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); }
> +WebInspector_syncDispatch = function(message)

What is the reason this uses an "_" and not "."? Is it so the backend can access
it directly off of the global scope without requiring messy property lookups?

>  v8::Handle<v8::Value> dispatchFunction = frameContext->Global()->Get(v8::String::New("WebInspector_syncDispatch"));

If so, it could use a comment in the code, along the lines of:

  "// This function is purposely put into the global scope for easy access."


> +    WebInspector.socket.onopen = function() {
> +        ...
> +        WebInspector.socketOpened = true;
> +        if (WebInspector.loadedDone)
> +            WebInspector.doLoadedDone();
> +    };
> +}
> +
>  WebInspector.loaded = function()
>  {
> +    if ("page" in WebInspector.queryParamsObject) {
> +        WebInspector.loadedDone = true;
> +        if (WebInspector.socketOpened)
> +            WebInspector.doLoadedDone();
> +        return;
> +    }
> +    WebInspector.doLoadedDone();
> +}

If I understand this correctly, this could potentially call doLoadedDone()
twice if WebInspector.socket.onopen gets called before WebInspector.loaded
gets called. Is that realistic? I would have thought both of the "if"
statements above would be exclusive, so if (!...).

[Pavel Feldman]

(In reply to comment #2)
> (From update of attachment 64338 [details])
> WebCore/inspector/front-end/inspector.js:434
>  +  var queryParams = window.location.search;
> Please wrap it into an anonymous function to avoid cluttering global context.
> 

Done.

> WebCore/inspector/front-end/inspector.js:445
>  +      WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); }
> Why not do this unconditionally after WebInspector has been loaded?

Done. Was not feasible before, but now communication starts post-load, so should be fine.

(In reply to comment #3)

> What is the reason this uses an "_" and not "."? Is it so the backend can access
> it directly off of the global scope without requiring messy property lookups?
> If so, it could use a comment in the code, along the lines of:
> 
>   "// This function is purposely put into the global scope for easy access."

Done.
> 
> If I understand this correctly, this could potentially call doLoadedDone()
> twice if WebInspector.socket.onopen gets called before WebInspector.loaded
> gets called. Is that realistic? I would have thought both of the "if"
> statements above would be exclusive, so if (!...).

The two flags (loadedDone and socketOpened) were supposed to guarantee single invocation. But given the above comments, this complexity goes away.

[Pavel Feldman]

Created attachment 64447 [details]
[PATCH] Review comments addressed.

[Joseph Pecoraro]

Comment on attachment 64447 [details]
[PATCH] Review comments addressed.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> +        Web Inspector: upstream frontend-side WebSocket transport.
> +        Chromium already has an alternate WebSocket-based communication channel with
> +        the backend. Upstreaming it in this change. We will agree on the URI
> +        of the remote service as the protocol matures.
> +        https://bugs.webkit.org/show_bug.cgi?id=43970

NIT: I should have commented on this last iteration. This is not the
usual formatting? "Title \n Bug \n \n Description" is much easier to read.


> +function parseQueryParameters()
> +{
> + ...
> +}
> +WebInspector.queryParamsObject = parseQueryParameters();

Now this just puts the function name "parseQueryParameters"
in the global namespace. An anonymous function might work
best because right now the function is only used once. There
are a number of choices, but I don't think this addressed
Yury's concern.


> +function installWebSocketTransport()
> +{
> +    WebInspector.socket = new WebSocket("ws://" + window.location.host + "/devtools/page/" + WebInspector.queryParamsObject.page);
> +    WebInspector.socket.onmessage = function(message) { WebInspector_syncDispatch(message.data); }
> +    WebInspector.socket.onerror = function(error) { console.err(error); }

I agree with the console.err(). It might also be worth a WebInspector.log
to warn the user an attempt was made at a connection and that attempt
failed. Currently it looks like there would be no user indication.


> +    WebInspector.socket.onopen = function() {
> +        ...
> +    };

NIT: Semicolon not needed here. I think we should add this as a style rule,
to not add these. I'm guilty of it myself all the time too.


> +//This function is purposely put into the global scope for easy access.

NIT: Missing space. "// This..."


> +WebInspector_syncDispatch = function(message)
> +{
> +    ...
> +};

NIT: Semicolon not needed.


Looks good to me, just I don't think you completely
addressed Yury's comment.

[Pavel Feldman]

Joe, thanks for the thorough review (as usually).

> NIT: I should have commented on this last iteration. This is not the
> usual formatting? "Title \n Bug \n \n Description" is much easier to read.
> 

Done. Ok, I now glue Title and Bug with no \n\n and even that is not enough ?!?!

> Now this just puts the function name "parseQueryParameters"
> in the global namespace. An anonymous function might work
> best because right now the function is only used once. There
> are a number of choices, but I don't think this addressed
> Yury's concern.
> 

I know. It is just that we never practiced anonymous closures for hiding functions in WebKit (see preloadImages). But if everyone agrees on the practice, I am happy to start using it. Migrated preloadImages as well.

> 
> I agree with the console.err(). It might also be worth a WebInspector.log
> to warn the user an attempt was made at a connection and that attempt
> failed. Currently it looks like there would be no user indication.
> 

We will need to wire complete error handling to the new protocol. We will need to consider ui indication for it (such as connection indicator or something).

> NIT: Semicolon not needed here. I think we should add this as a style rule,
> to not add these. I'm guilty of it myself all the time too.
> 

Done.

> NIT: Missing space. "// This..."
> 

Done.

> NIT: Semicolon not needed.
> 

Done.

> 
> Looks good to me, just I don't think you completely
> addressed Yury's comment.

Will upload new one shortly.

[Pavel Feldman]

Created attachment 64449 [details]
[PATCH] Proposed change.

[Pavel Feldman]

Comment on attachment 64449 [details]
[PATCH] Proposed change.

Self-review notes: 

inspector.js:430
> (function parseQueryParameters()
>  431 {

This thing does not work unless I put semicolon after

WebInspector.PlatformFlavor = {
    ...
    MacSnowLeopard: "mac-snowleopard"
};

[Pavel Feldman]

Created attachment 64450 [details]
[PATCH] Proposed change.

[Joseph Pecoraro]

Comment on attachment 64450 [details]
[PATCH] Proposed change.

> > NIT: I should have commented on this last iteration. This is not the
> > usual formatting? "Title \n Bug \n \n Description" is much easier to read.
> > 
> 
> Done. Ok, I now glue Title and Bug with no \n\n and even that is not enough ?!?!

This is perfect. This is how I'd say 90% of ChangeLogs are formatted.
And its what `prepare-ChangeLog`encourages.

In the past, you have been putting an extra line between the title and
bug link. The `prepare-ChangeLog` script does not, so I try and keep
consistent with that. I commented on how you did it in the past =),
but I remember Tim saying he liked your way better. Either way is
fine, but I think the description should really be separate from the
title/description.


> I know. It is just that we never practiced anonymous closures for
> hiding functions in WebKit (see preloadImages). But if everyone
> agrees on the practice, I am happy to start using it. Migrated
> preloadImages as well.

True. I think there is only a few usages. I prefer them. I like keeping
the global scope clean, and not storing a reference to 1-off functions.

I had one final thought! parseQueryParameters should probably
use window.decodeURIComponent for pair[1]!


> We will need to wire complete error handling to the new protocol.
> We will need to consider ui indication for it (such as connection indicator
> or something).

Gotcha. If you think its appropriate, please file a follow-up bug. Or, just
keep it in mind as I'm sure it will be addressed later.

Thanks for addressing the comments. r=me

-------------

> This thing does not work unless I put semicolon after
> 
> WebInspector.PlatformFlavor = {
>     ...
>     MacSnowLeopard: "mac-snowleopard"
> };

Interesting. How about this for the semicolon rule:

  Semicolons SHOULD be used for object literals:

    var obj = { ... };
    some.obj = {
      ...
    };

  But SHOULD NOT be used for closing scopes like
  functions or anonymous scopes (which I don't think
  we use at all currently).

Let me know if that misses anything.

[Pavel Feldman]

>   Semicolons SHOULD be used for object literals:
> 
>     var obj = { ... };
>     some.obj = {
>       ...
>     };
> 
>   But SHOULD NOT be used for closing scopes like
>   functions or anonymous scopes (which I don't think
>   we use at all currently).
> 
> Let me know if that misses anything.

That would be a nice rule to follow (and is in fact standard to Google), but WebInspector traditionally did not set semicolon after assigning to the prototype. Time to change the tradition?

[Pavel Feldman]

Comment on attachment 64450 [details]
[PATCH] Proposed change.

Clearing flags on attachment: 64450

Committed r65415: <http://trac.webkit.org/changeset/65415>

[Pavel Feldman]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/65415 might have broken Qt Linux Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/65414
http://trac.webkit.org/changeset/65415

[Joseph Pecoraro]

> That would be a nice rule to follow (and is in fact standard to Google), but
> WebInspector traditionally did not set semicolon after assigning to the
> prototype. Time to change the tradition?

You're right. I hate special cases but I like tradition on this one.