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).
Created attachment 186991 [details] Patch
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 on attachment 186991 [details] Patch r- per Eugene's comment.
Created attachment 188750 [details] Patch
(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 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).
Created attachment 189447 [details] Patch
(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 on attachment 189447 [details] Patch LGTM
Comment on attachment 189447 [details] Patch Clearing flags on attachment: 189447 Committed r143564: <http://trac.webkit.org/changeset/143564>
All reviewed patches have been landed. Closing bug.