Bug 109135

Summary: Web Inspector: Sort by Initiator functionality of Network Panel doesn't work well
Product: WebKit Reporter: pdeng6 <pan.deng>
Component: Web Inspector (Deprecated)Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: apavlov, eustas, keishi, loislo, pfeldman, pmuellr, vsevik, web-inspector-bugs, webkit.review.bot, yurys
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
sort by Initiator problem
none
Patch
none
Patch
none
Patch none

Description pdeng6 2013-02-06 22:25:10 PST
Created attachment 186990 [details]
sort by Initiator problem

How to reproduce?
1: open "foo.com" and launch inspector network panel
2: refresh "foo.com"
3: sort by initiator column
You can find it behaves incorrectly.

Root Cause:
1, "Other" is never matched, since it should be "other"
2, the compared url is not the string actually displayed in initiatorCell, even some url are "undefined" when access it.
3, in the last lineNumber compare, access initiator.lineNumber directly won't work well sometimes (initiator is script).
Comment 1 pdeng6 2013-02-06 22:33:11 PST
Created attachment 186991 [details]
Patch
Comment 2 Eugene Klyuchnikov 2013-02-11 01:39:57 PST
Comment on attachment 186991 [details]
Patch

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

> Source/WebCore/inspector/front-end/NetworkPanel.js:2220
> +    get displayedInitiatorUrl()

We're not using getters/setters in new code.

> Source/WebCore/inspector/front-end/NetworkPanel.js:2226
> +        return displayedUrl.replace(/:\d*$/, "");

Accessing text content and using regexp should be avoided at all and specifically in loop (sorting).
This data should be stored in node field when refresh comes.

> Source/WebCore/inspector/front-end/NetworkPanel.js:2235
> +        return displayedUrl.replace(/[^:]*:/g, "");

Ditto.

> Source/WebCore/inspector/front-end/NetworkPanel.js:2407
> +    if (!a._request.initiator || a._request.initiator.type === "other")

Enum containing ["parser", "script", "other"] should be declared and used to avoid further typos.
IMHO it could be placed in NetworkRequest.js
Comment 3 Vsevolod Vlasov 2013-02-11 01:58:51 PST
Comment on attachment 186991 [details]
Patch

r- per Eugene's comment.
Comment 4 pdeng6 2013-02-16 18:56:56 PST
Created attachment 188750 [details]
Patch
Comment 5 pdeng6 2013-02-16 18:59:25 PST
(In reply to comment #2)
> (From update of attachment 186991 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=186991&action=review
> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:2220
> > +    get displayedInitiatorUrl()
> 
> We're not using getters/setters in new code.
Got it, done, thanks!
> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:2226
> > +        return displayedUrl.replace(/:\d*$/, "");
> 
> Accessing text content and using regexp should be avoided at all and specifically in loop (sorting).
> This data should be stored in node field when refresh comes.
> 
Done, thanks.
> > Source/WebCore/inspector/front-end/NetworkPanel.js:2235
> > +        return displayedUrl.replace(/[^:]*:/g, "");
> 
> Ditto.
> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:2407
> > +    if (!a._request.initiator || a._request.initiator.type === "other")
> 
> Enum containing ["parser", "script", "other"] should be declared and used to avoid further typos.
> IMHO it could be placed in NetworkRequest.js
Great, done, thanks :)

Pan
Comment 6 Eugene Klyuchnikov 2013-02-20 07:17:07 PST
Comment on attachment 188750 [details]
Patch

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

> Source/WebCore/inspector/front-end/NetworkPanel.js:2228
> +    displayedInitiatorUrl: function()

1) This method should be private (with underscore before name), because it is not used outside of NetworkPanel.js
2) This method should be inlined, because it private (de facto) and plain (do not contain logic).
Comment 7 pdeng6 2013-02-20 19:11:13 PST
Created attachment 189447 [details]
Patch
Comment 8 pdeng6 2013-02-20 19:12:46 PST
(In reply to comment #6)
> (From update of attachment 188750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188750&action=review
> 
> > Source/WebCore/inspector/front-end/NetworkPanel.js:2228
> > +    displayedInitiatorUrl: function()
> 
> 1) This method should be private (with underscore before name), because it is not used outside of NetworkPanel.js
> 2) This method should be inlined, because it private (de facto) and plain (do not contain logic).

done, thanks :)

Pan
Comment 9 Eugene Klyuchnikov 2013-02-20 20:00:24 PST
Comment on attachment 189447 [details]
Patch

LGTM
Comment 10 WebKit Review Bot 2013-02-20 22:27:47 PST
Comment on attachment 189447 [details]
Patch

Clearing flags on attachment: 189447

Committed r143564: <http://trac.webkit.org/changeset/143564>
Comment 11 WebKit Review Bot 2013-02-20 22:27:51 PST
All reviewed patches have been landed.  Closing bug.