<rdar://problem/20509200>

Created attachment 288522 [details] [Patch] WIP So this does work, but it isn't perfect: - I am not entirely sure how to clear the saved displayName values in WI.SourceCode._displayNameBiMap when the user changes locations (via a link or modifying the URL). - I don't think that the same URL will ever be loaded twice (like "http://test.com/a/b/c.js?d=e" being loaded more than once), but in the case that it does happen this will just kinda fail (the BiMap.prototype.put assertions will fire). - I don't know how to address the situation where two URLs are identical except the domain (like "http://foo.com/a.js?b=c" and "http://bar.com/a.js?b=c"). The only idea I have for fixing this is to make BiMap into a BiMultiMap, but that kinda defeats the entire purpose of this change. - I don't think that this is a big issue, but it is possible that BiMap's _map will keep references to objects that could otherwise be GCd. My thinking is that this is not an issue for WI.SourceCode._displayNameBiMap (but may be an issue elsewhere) since the SourceCode objects all have sidebar entries that aren't removed unless the location changes. Any feedback is very welcome :)

These seem like real LayoutTest failures.

Comment on attachment 288522 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=288522&action=review I haven't yet reviewed this all in detail, I just sort of scanned it. Could you include a screenshot of what this looks like? What if two URLs are entirely the same except for their query string? <img src="image.php?id=1234"> <img src="image.php?id=4567"> > Source/WebInspectorUI/.eslintrc:58 > + "BiMap": true, Nice! > Source/WebInspectorUI/UserInterface/Base/BiMap.js:31 > + this._map = new Map; > + this._mirror = new WeakMap; These should probably be identical since they have different constraints, unless you wish to enforce them. A WeakMap can only have Object keys. However a Map can have any type of key: js> (new WeakMap).set(1, 1) TypeError: Attempted to set a non-object key in a WeakMap js> (new Map).set(1, 1) Map {1 => 1} (1) By using a WeakMap here that means you expect any value put into the main Map must have an Object key, which seems unnecessarily restrictive. > Source/WebInspectorUI/UserInterface/Base/BiMap.js:33 > + Nit: // Public > Source/WebInspectorUI/UserInterface/Base/URLUtilities.js:236 > + if (commonSubstring.includes("/")) > + commonSubstring = commonSubstring.substring(0, commonSubstring.lastIndexOf("/") + 1); You can reduce the potential for passes through the string (includes and lastIndexOf): let lastSlashIndex = commonSubstring.lastIndexOf("/"); if (lastSlashIndex !== -1 && lastSlashIndex !== i) commonSubstring = commonSubstring.substring(0, lastSlashIndex + 1); Also this should have some very basic tests: LayoutTests/inspector/unit-tests/url-utilities.html longestCommonPath("foo", "bar"); longestCommonPath("zFoo", "zBar"); longestCommonPath("common/foo", "common/"); longestCommonPath("common/foo", "common/bar"); longestCommonPath("common/dir/foo", "common/dir/bar"); longestCommonPath("common/dir1/foo", "common/dir2/bar"); > Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:589 > + WebInspector.SourceCode.clearDisplayNames(); This seems like the wrong place for this. Perhaps DebuggerManager would be best. Alongside: WebInspector.Script.resetUniqueDisplayNameNumbers(); > Source/WebInspectorUI/UserInterface/Models/Resource.js:227 > - get size() > - { > - return this._size; > - } > - > get encodedSize() > { > if (!isNaN(this._transferSize)) I didn't move the getters on some of these classes because it was messy and would move things that would naturally go well side by side to suddenly be far apart. For instance size/encodedSize/transferSize or requestData/requestDataContentType. There are plenty of getters that should move, but I'm not sure just moving all adds value here.

Comment on attachment 288522 [details] [Patch] WIP r- for lack of tests and the open questions for now

Comment on attachment 288522 [details] [Patch] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=288522&action=review >> Source/WebInspectorUI/UserInterface/Base/BiMap.js:31 >> + this._mirror = new WeakMap; > > These should probably be identical since they have different constraints, unless you wish to enforce them. > > A WeakMap can only have Object keys. However a Map can have any type of key: > > js> (new WeakMap).set(1, 1) > TypeError: Attempted to set a non-object key in a WeakMap > > js> (new Map).set(1, 1) > Map {1 => 1} (1) > > By using a WeakMap here that means you expect any value put into the main Map must have an Object key, which seems unnecessarily restrictive. For some reason, I thought that keys of Map had to be strings :/ >> Source/WebInspectorUI/UserInterface/Controllers/FrameResourceManager.js:589 >> + WebInspector.SourceCode.clearDisplayNames(); > > This seems like the wrong place for this. Perhaps DebuggerManager would be best. Alongside: > > WebInspector.Script.resetUniqueDisplayNameNumbers(); I was wondering where to put this :) >> Source/WebInspectorUI/UserInterface/Models/Resource.js:227 >> if (!isNaN(this._transferSize)) > > I didn't move the getters on some of these classes because it was messy and would move things that would naturally go well side by side to suddenly be far apart. For instance size/encodedSize/transferSize or requestData/requestDataContentType. There are plenty of getters that should move, but I'm not sure just moving all adds value here. Oh good point. I think that since this patch is already quite large, I will just undo these and leave it to a future change.

Created attachment 288781 [details] Patch

Comment on attachment 288781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288781&action=review > Source/WebInspectorUI/UserInterface/Base/BiMap.js:26 > +class BiMap We should probably have some brief unit tests for this too. > LayoutTests/inspector/unit-tests/url-utilities.html:8 > - let suite = InspectorTest.createSyncSuite("URLUtilities"); > + let suite = InspectorTest.createAsyncSuite("URLUtilities"); Did this need to change to async? I would expect nearly all Utilities to not be async.

Comment on attachment 288781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288781&action=review r- because BiMap needs tests. Please revert unrelated getter changes and code moves, as it really hinders reviewing. >> LayoutTests/inspector/unit-tests/url-utilities.html:8 >> + let suite = InspectorTest.createAsyncSuite("URLUtilities"); > > Did this need to change to async? I would expect nearly all Utilities to not be async. Please revert.

Comment on attachment 288781 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=288781&action=review >>> LayoutTests/inspector/unit-tests/url-utilities.html:8 >>> + let suite = InspectorTest.createAsyncSuite("URLUtilities"); >> >> Did this need to change to async? I would expect nearly all Utilities to not be async. > > Please revert. I don't know why, but I thought I had heard somewhere that we wanted to move away from sync tests. I think I just completely misunderstood what I heard =/

Created attachment 289042 [details] Patch

Created attachment 289043 [details] [Image] After Patch is applied So I tested this on my website (http://devinrousso.com) since I use query strings for my PHP magnification scripts (judge me as much as you want). The thing to note from this image is just the difference between the "filename" value in the details sidebar and the actual displayed name in the navigation sidebar.

(In reply to comment #18) > The thing to note from this image is just the difference between the > "filename" value in the details sidebar and the actual displayed name in the > navigation sidebar. I don't showing "?file=common" instead of "js" is what we want. Maybe your solution could be extended so that: 1. Filename is unique -> use filename 2. Path is unique -> use path + filename 3. Query string is unique -> use filename + query string 4. Otherwise just filename

(In reply to comment #19) > I don't showing "?file=common" instead of "js" is what we want. Maybe your > solution could be extended so that: > > 1. Filename is unique -> use filename > 2. Path is unique -> use path + filename > 3. Query string is unique -> use filename + query string > 4. Otherwise just filename So I may have chosen a bad example, but the URL "?file=common" resolves to "http://devinrousso.com/ui/js/?file=common", meaning that "?file=common" doesn't actually have a filename (implicitly it's index.php). The logic I wrote (IIRC) always includes the filename, as well as any unique parent folders (compared to other resources) and query strings if necessary.

(In reply to comment #20) > (In reply to comment #19) > So I may have chosen a bad example, but the URL "?file=common" resolves to > "http://devinrousso.com/ui/js/?file=common", meaning that "?file=common" > doesn't actually have a filename (implicitly it's index.php). The logic I > wrote (IIRC) always includes the filename, as well as any unique parent > folders (compared to other resources) and query strings if necessary. I didn't notice that "js" was a folder! This is actually a really good example, because it highlights a bug with the handling of query string only filenames. The logic you described sounds good, will give a thorough review later.

I don't like the hanging question marks on these examples. I think it should show "js/?foo=bar" since we can't know it is "index.php".

*** Bug 176765 has been marked as a duplicate of this bug. ***

Created attachment 335980 [details] Patch I have to run out now, but I'll add a ChangeLog comment for the logic later

Created attachment 336000 [details] Patch Should fix EWS

Also, just for "posterity" this is what I've been using as a reduction: ``` function createScript(url) { let script = document.body.appendChild(document.createElement("script")); script.src = url; } createScript("a/script.js"); createScript("b/script.js"); createScript("c/a/script.js"); createScript("c/b/script.js"); createScript("d/?1"); createScript("d/?2"); createScript("e/?1"); createScript("e/?2"); ``` This will generate a console error for each script, but that's fine because we only care about the `displayName`.

Created attachment 336020 [details] Patch

Comment on attachment 336020 [details] Patch r- this is not in a state ready for review.