Created attachment 250569[details]
[Animated GIF] Actual/expected
HTML:
<script src="a/script.js"></script>
<script src="b/script.js"></script>
The resource list shows "script.js" twice.
We should display parent directory when file names match. It could be possible that parent directories match too, so we should traverse up the tree until we have uniq paths.
Via https://twitter.com/jasonlfunk/status/586134407716741121
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 :)
Created attachment 288523[details]
Archive of layout-test-results from ews102 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102 Port: mac-yosemite Platform: Mac OS X 10.10.5
Created attachment 288525[details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 288526[details]
Archive of layout-test-results from ews116 for mac-yosemite
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-yosemite Platform: Mac OS X 10.10.5
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
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.
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 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.
Created attachment 335984[details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 335985[details]
Archive of layout-test-results from ews100 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 335987[details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
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 336001[details]
Archive of layout-test-results from ews101 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 336002[details]
Archive of layout-test-results from ews104 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Created attachment 336005[details]
Archive of layout-test-results from ews112 for mac-sierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
2015-04-11 00:20 PDT, Nikita Vasilyev
2016-09-10 22:30 PDT, Devin Rousso
2016-09-10 23:23 PDT, Build Bot
2016-09-10 23:26 PDT, Build Bot
2016-09-10 23:31 PDT, Build Bot
2016-09-14 01:00 PDT, Devin Rousso
2016-09-16 00:30 PDT, Devin Rousso
2016-09-16 00:31 PDT, Devin Rousso
2018-03-16 15:58 PDT, Devin Rousso
2018-03-16 17:01 PDT, EWS Watchlist
2018-03-16 17:05 PDT, EWS Watchlist
2018-03-16 17:39 PDT, EWS Watchlist
2018-03-17 00:06 PDT, Devin Rousso
2018-03-17 01:09 PDT, EWS Watchlist
2018-03-17 01:14 PDT, EWS Watchlist
2018-03-17 01:37 PDT, EWS Watchlist
2018-03-17 17:01 PDT, Devin Rousso