Bug 109135 – Web Inspector: Sort by Initiator functionality of Network Panel doesn't work well

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[pdeng6]

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).

[pdeng6]

Created attachment 186991 [details]
Patch

[Eugene Klyuchnikov]

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

[Vsevolod Vlasov]

Comment on attachment 186991 [details]
Patch

r- per Eugene's comment.

[pdeng6]

Created attachment 188750 [details]
Patch

[pdeng6]

(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

[Eugene Klyuchnikov]

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).

[pdeng6]

Created attachment 189447 [details]
Patch

[pdeng6]

(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

[Eugene Klyuchnikov]

Comment on attachment 189447 [details]
Patch

LGTM

[WebKit Review Bot]

Comment on attachment 189447 [details]
Patch

Clearing flags on attachment: 189447

Committed r143564: <http://trac.webkit.org/changeset/143564>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.