Web Inspector: the URL of worker inspector window sometimes is invalid

[Peter Wang]

It's caused by a defect of function "_openInspectorWindow" in "WorkerManager.js". Taking QT port as example, the "window.location.href" is "qrc://../inspector.html", so the url of popup inspector window is "qrc://../inspector.html&dedicatedWorkerId", it causes a problem. To insert a '?' can make it works, the right url is "qrc://../inspector.html?&dedicatedWorkerId".

[Peter Wang]

Created attachment 161203 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 161203 [details]
Patch

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

> Source/WebCore/inspector/front-end/WorkerManager.js:166
> +        if (!search)

This can be simplified:
var url = window.location.href
if (!search)
    url += "?";
url += "&dedicatedWorkerId=" + workerId;

[Peter Wang]

Created attachment 161210 [details]
Patch

[Peter Wang]

(In reply to comment #2)
> (From update of attachment 161203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161203&action=review
> 
> > Source/WebCore/inspector/front-end/WorkerManager.js:166
> > +        if (!search)
> 
> This can be simplified:
> var url = window.location.href
> if (!search)
>     url += "?";
> url += "&dedicatedWorkerId=" + workerId;

But, seems for Chromium the "window.location.href" have a '?'(In reply to comment #2)
> (From update of attachment 161203 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161203&action=review
> 
> > Source/WebCore/inspector/front-end/WorkerManager.js:166
> > +        if (!search)
> 
> This can be simplified:
> var url = window.location.href
> if (!search)
>     url += "?";
> url += "&dedicatedWorkerId=" + workerId;

sorry.

[Peter Wang]

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 161203 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161203&action=review
> > 
> > > Source/WebCore/inspector/front-end/WorkerManager.js:166
> > > +        if (!search)
> > 
> > This can be simplified:
> > var url = window.location.href
> > if (!search)
> >     url += "?";
> > url += "&dedicatedWorkerId=" + workerId;
> 
> But, seems for Chromium the "window.location.href" have a '?'(In reply to comment #2)
> > (From update of attachment 161203 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161203&action=review
> > 
> > > Source/WebCore/inspector/front-end/WorkerManager.js:166
> > > +        if (!search)
> > 
> > This can be simplified:
> > var url = window.location.href
> > if (!search)
> >     url += "?";
> > url += "&dedicatedWorkerId=" + workerId;
> 
> sorry.
sorry, this comment is submitted by a mistake. It should be ignored.

[Pavel Feldman]

Comment on attachment 161210 [details]
Patch

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

> Source/WebCore/inspector/front-end/WorkerManager.js:168
> +        url += "&dedicatedWorkerId=" + workerId;

You don't need '&' to follow '?', right?

[Peter Wang]

(In reply to comment #6)
> (From update of attachment 161210 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161210&action=review
> 
> > Source/WebCore/inspector/front-end/WorkerManager.js:168
> > +        url += "&dedicatedWorkerId=" + workerId;
> 
> You don't need '&' to follow '?', right?

No, I need "&dedicatedWorkerId" to indicate the parameter, but the "url" is not always ended with '?', and it will caused a invalid URL.

[Peter Wang]

Created attachment 161646 [details]
The description of the problem

I've upload a picture as a description of this bug. Maybe it shows the problem more clearly.

[Pavel Feldman]

I think I understand the problem clearly. But you code produces

foo.com?&dedicatedWorkerId=...

for "foo.com" href. It should produce

foo.com?dedicatedWorkerId=...

[Peter Wang]

Created attachment 161650 [details]
Patch

[Peter Wang]

(In reply to comment #9)
> I think I understand the problem clearly. But you code produces
> 
> foo.com?&dedicatedWorkerId=...
> 
> for "foo.com" href. It should produce
> 
> foo.com?dedicatedWorkerId=...

Yes, it seems not good. Shame.

[Pavel Feldman]

Comment on attachment 161650 [details]
Patch

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

> Source/WebCore/inspector/front-end/WorkerManager.js:-164
> -        var url = window.location.href + "&dedicatedWorkerId=" + workerId;

var url = window.location.href + (window.location.search ? "&" : "?") + "dedicatedWorkerId=" + workerId;

would keep it one liner.

[Andrey Adaikin]

(In reply to comment #12)
> (From update of attachment 161650 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161650&action=review
> 
> > Source/WebCore/inspector/front-end/WorkerManager.js:-164
> > -        var url = window.location.href + "&dedicatedWorkerId=" + workerId;
> 
> var url = window.location.href + (window.location.search ? "&" : "?") + "dedicatedWorkerId=" + workerId;
> 
> would keep it one liner.

FYI
provided that window.location.hash is always empty

[Pavel Feldman]

Comment on attachment 161650 [details]
Patch

Lets keep the code dense!

[Peter Wang]

Created attachment 161882 [details]
Patch

[Peter Wang]

(In reply to comment #13)
> (In reply to comment #12)
> > (From update of attachment 161650 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=161650&action=review
> > 
> > > Source/WebCore/inspector/front-end/WorkerManager.js:-164
> > > -        var url = window.location.href + "&dedicatedWorkerId=" + workerId;
> > 
> > var url = window.location.href + (window.location.search ? "&" : "?") + "dedicatedWorkerId=" + workerId;
> > 
> > would keep it one liner.
> 
> FYI
> provided that window.location.hash is always empty

Thx. Yes, window.location.hash is a problem.

[Peter Wang]

(In reply to comment #15)
> Created an attachment (id=161882) [details]
> Patch
So far 
(In reply to comment #14)
> (From update of attachment 161650 [details])
> Lets keep the code dense!

Ok.The newest patch can handle the situation if there is a "window.location.hash" in url.

[Pavel Feldman]

Comment on attachment 161882 [details]
Patch

At some point we should make ParsedURL read-write so that one could patch query parameters and serialize it back.

[Charles Wei]

Comment on attachment 161882 [details]
Patch

commit.

[WebKit Review Bot]

Comment on attachment 161882 [details]
Patch

Clearing flags on attachment: 161882

Committed r127437: <http://trac.webkit.org/changeset/127437>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.