[webkit-reviews] review denied: [Bug 43970] Web Inspector: upstream frontend-side WebSocket transport. : [Attachment 64447] [PATCH] Review comments addressed.

[bugzilla-daemon at webkit.org]

Joseph Pecoraro <joepeck at webkit.org> has denied Pavel Feldman
<pfeldman at chromium.org>'s request for review:
Bug 43970: Web Inspector: upstream frontend-side WebSocket transport.
https://bugs.webkit.org/show_bug.cgi?id=43970

Attachment 64447: [PATCH] Review comments addressed.
https://bugs.webkit.org/attachment.cgi?id=64447&action=review

------- Additional Comments from Joseph Pecoraro <joepeck at webkit.org>
> 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.