Bug 95334

Summary: Web Inspector: the URL of worker inspector window sometimes is invalid
Product: WebKit Reporter: Peter Wang <PeterHWang>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aandrey, apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 93519    
Attachments:
Description Flags
Patch
none
Patch
none
The description of the problem
none
Patch
none
Patch none

Description Peter Wang 2012-08-29 05:27:06 PDT 
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".
Comment 1 Peter Wang 2012-08-29 05:44:39 PDT 
Created attachment 161203 [details]
Patch
Comment 2 Yury Semikhatsky 2012-08-29 06:04:32 PDT 
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;
Comment 3 Peter Wang 2012-08-29 06:23:53 PDT 
Created attachment 161210 [details]
Patch
Comment 4 Peter Wang 2012-08-29 06:24:08 PDT 
(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.
Comment 5 Peter Wang 2012-08-29 06:27:07 PDT 
(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.
Comment 6 Pavel Feldman 2012-08-31 01:57:41 PDT 
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?
Comment 7 Peter Wang 2012-08-31 02:06:03 PDT 
(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.
Comment 8 Peter Wang 2012-08-31 02:40:56 PDT 
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.
Comment 9 Pavel Feldman 2012-08-31 02:45:58 PDT 
I think I understand the problem clearly. But you code produces

foo.com?&dedicatedWorkerId=...

for "foo.com" href. It should produce

foo.com?dedicatedWorkerId=...
Comment 10 Peter Wang 2012-08-31 03:22:15 PDT 
Created attachment 161650 [details]
Patch
Comment 11 Peter Wang 2012-08-31 03:25:18 PDT 
(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.
Comment 12 Pavel Feldman 2012-08-31 05:45:07 PDT 
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.
Comment 13 Andrey Adaikin 2012-08-31 12:29:18 PDT 
(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
Comment 14 Pavel Feldman 2012-09-03 02:08:43 PDT 
Comment on attachment 161650 [details]
Patch

Lets keep the code dense!
Comment 15 Peter Wang 2012-09-03 02:16:34 PDT 
Created attachment 161882 [details]
Patch
Comment 16 Peter Wang 2012-09-03 02:17:46 PDT 
(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.
Comment 17 Peter Wang 2012-09-03 02:27:50 PDT 
(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.
Comment 18 Pavel Feldman 2012-09-03 06:23:48 PDT 
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.
Comment 19 Charles Wei 2012-09-03 19:04:53 PDT 
Comment on attachment 161882 [details]
Patch

commit.
Comment 20 WebKit Review Bot 2012-09-03 19:09:20 PDT 
Comment on attachment 161882 [details]
Patch

Clearing flags on attachment: 161882

Committed r127437: <http://trac.webkit.org/changeset/127437>
Comment 21 WebKit Review Bot 2012-09-03 19:09:24 PDT 
All reviewed patches have been landed.  Closing bug.