Bug 118678 – Web Inspector: WebSocket initialization to use remote inspector

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Roland Takacs]

Some port use remote inspector only that requires WebSocket to the connection.

[Radar WebKit Bug Importer]

<rdar://problem/14444476>

[Roland Takacs]

Created attachment 206676 [details]
initial patch

Added a WebSocket initialization from the old inspector code.
Also defined a 'connect-src' directive into the Content-Security-Policy HTTP header to enable the connection.

Added WorkerManager.js file from the old inspector because it defines the isDedicatedWorkerFrontend() to skip the websocket initialization.

[Timothy Hatcher]

Comment on attachment 206676 [details]
initial patch

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

> Source/WebInspectorUI/UserInterface/Main.js:41
> +{(function parseQueryParameters()
> +{
> +    WebInspector.queryParamsObject = {};
> +    var queryParams = window.location.search;
> +    if (!queryParams)
> +        return;
> +    var params = queryParams.substring(1).split("&");
> +    for (var i = 0; i < params.length; ++i) {
> +        var pair = params[i].split("=");
> +        WebInspector.queryParamsObject[pair[0]] = pair[1];
> +    }
> +})();}

We have parseLocationQueryParameters in Utilities.js. And we use parseLocationQueryParameters once in Main.js already. We should only parse this once. We don't want WebInspector.queryParamsObject like we had before either.

> Source/WebInspectorUI/UserInterface/Main.js:57
> +    if (ws) {

This can be a early return.

> Source/WebInspectorUI/UserInterface/Main.js:66
> +            if (!WebInspector.socket._detachReason)
> +                (new WebInspector.RemoteDebuggingTerminatedScreen("websocket_closed")).showModal();

WebInspector.RemoteDebuggingTerminatedScreen does not exist in our UI. Remove this for now.

> Source/WebInspectorUI/UserInterface/Main.js:76
> +    WebInspector.initializeWebSocketIfNeeded();

this.initializeWebSocketIfNeeded();

> Source/WebInspectorUI/UserInterface/WorkerManager.js:50
> +WebInspector.WorkerManager.isDedicatedWorkerFrontend = function()
> +{
> +    return !!WebInspector.queryParamsObject["dedicatedWorkerId"];
> +}

If this is all you use WebInspector.WorkerManager for, then this can go in Main.js and WorkerManager.js can be removed. I don't see WorkerManager being called otherwise.

[Timothy Hatcher]

Comment on attachment 206676 [details]
initial patch

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

> Source/WebInspectorUI/UserInterface/Main.js:46
> +    if (WebInspector.WorkerManager.isDedicatedWorkerFrontend())
> +        return;

We don't have dedicated worker frontends. So you can remove this check and WorkerManager.

> Source/WebInspectorUI/UserInterface/Main.js:61
> +        WebInspector.socket.onmessage = function(message) { InspectorBackend.dispatch(message.data); }
> +        WebInspector.socket.onerror = function(error) { console.error(error); }
> +        WebInspector.socket.onopen = function() {

Can these use addEventListener instead?

> Source/WebInspectorUI/UserInterface/Main.js:62
> +            InspectorFrontendHost.sendMessageToBackend = WebInspector.socket.send.bind(WebInspector.socket);

This code should move to InspectorFrontendHostStub.js and if WebInspector.socket exists then use WebInspector.socket.send.

> Source/WebInspectorUI/UserInterface/Main.js:64
> +        WebInspector.socket.onclose = function() {

You can remove onclose.

>> Source/WebInspectorUI/UserInterface/WorkerManager.js:50
>> +}
> 
> If this is all you use WebInspector.WorkerManager for, then this can go in Main.js and WorkerManager.js can be removed. I don't see WorkerManager being called otherwise.

It turns out we don't have dedicated worker frontends like the deprecated Inspector — and likely never will. You can remove this and all of WorkerManager.js.

[Seokju Kwon]

Any update on this?

[Roland Takacs]

I will attach an updated patch soon.

(In reply to comment #5)
> Any update on this?

[Roland Takacs]

Created attachment 207307 [details]
patch

[WebKit Commit Bot]

Comment on attachment 207307 [details]
patch

Clearing flags on attachment: 207307

Committed r153044: <http://trac.webkit.org/changeset/153044>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 207307 [details]
patch

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

Just some unimportant post thoughts. Nothing that needs to be changed.

> Source/WebInspectorUI/UserInterface/Main.js:35
> +    // Initialize WebSocket to communication

Nits for the future, comments are sentences and should end in a period.

> Source/WebInspectorUI/UserInterface/Main.js:1100
> +        var host = "host" in queryParams ? queryParams.host : window.location.host;

I think this could have been written:

    var host = queryParams.host || window.location.host;

Unless the possibility of an empty string host ("") would be considered valid.