Bug 143632 - Web Inspector: Resources with the same name in different folders aren't distinguished
Summary: Web Inspector: Resources with the same name in different folders aren't disti...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
: 176765 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-04-11 00:20 PDT by Nikita Vasilyev
Modified: 2018-10-01 11:29 PDT (History)
9 users (show)

See Also:


Attachments
[Animated GIF] Actual/expected (38.00 KB, image/gif)
2015-04-11 00:20 PDT, Nikita Vasilyev
no flags Details
[Patch] WIP (40.27 KB, patch)
2016-09-10 22:30 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-yosemite (994.20 KB, application/zip)
2016-09-10 23:23 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (910.21 KB, application/zip)
2016-09-10 23:26 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-yosemite (1.59 MB, application/zip)
2016-09-10 23:31 PDT, Build Bot
no flags Details
Patch (45.99 KB, patch)
2016-09-14 01:00 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (45.38 KB, patch)
2016-09-16 00:30 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (240.17 KB, image/png)
2016-09-16 00:31 PDT, Devin Rousso
no flags Details
Patch (37.16 KB, patch)
2018-03-16 15:58 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-sierra-wk2 (3.11 MB, application/zip)
2018-03-16 17:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews100 for mac-sierra (2.37 MB, application/zip)
2018-03-16 17:05 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (3.12 MB, application/zip)
2018-03-16 17:39 PDT, EWS Watchlist
no flags Details
Patch (41.14 KB, patch)
2018-03-17 00:06 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (2.19 MB, application/zip)
2018-03-17 01:09 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (2.60 MB, application/zip)
2018-03-17 01:14 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews112 for mac-sierra (2.93 MB, application/zip)
2018-03-17 01:37 PDT, EWS Watchlist
no flags Details
Patch (43.99 KB, patch)
2018-03-17 17:01 PDT, Devin Rousso
hi: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2015-04-11 00:20:43 PDT
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
Comment 1 Radar WebKit Bug Importer 2015-04-11 00:21:25 PDT
<rdar://problem/20509200>
Comment 2 Devin Rousso 2016-09-10 22:30:18 PDT
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 :)
Comment 3 Build Bot 2016-09-10 23:23:31 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2016-09-10 23:23:35 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2016-09-10 23:26:46 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2016-09-10 23:26:49 PDT Comment hidden (obsolete)
Comment 7 Build Bot 2016-09-10 23:31:04 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2016-09-10 23:31:07 PDT Comment hidden (obsolete)
Comment 9 Joseph Pecoraro 2016-09-12 14:19:09 PDT
These seem like real LayoutTest failures.
Comment 10 Joseph Pecoraro 2016-09-12 14:44:57 PDT
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 11 Joseph Pecoraro 2016-09-13 11:18:03 PDT
Comment on attachment 288522 [details]
[Patch] WIP

r- for lack of tests and the open questions for now
Comment 12 Devin Rousso 2016-09-13 21:53:29 PDT
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 13 Devin Rousso 2016-09-14 01:00:43 PDT
Created attachment 288781 [details]
Patch
Comment 14 Joseph Pecoraro 2016-09-15 17:05:31 PDT
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 15 BJ Burg 2016-09-15 17:39:52 PDT
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 16 Devin Rousso 2016-09-15 23:45:42 PDT
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 =/
Comment 17 Devin Rousso 2016-09-16 00:30:20 PDT
Created attachment 289042 [details]
Patch
Comment 18 Devin Rousso 2016-09-16 00:31:50 PDT
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.
Comment 19 Matt Baker 2016-10-04 17:13:58 PDT
(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
Comment 20 Devin Rousso 2016-10-04 17:33:32 PDT
(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.
Comment 21 Matt Baker 2016-10-04 18:00:06 PDT
(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.
Comment 22 Timothy Hatcher 2016-10-05 10:18:20 PDT
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".
Comment 23 Joseph Pecoraro 2017-09-11 21:29:41 PDT
*** Bug 176765 has been marked as a duplicate of this bug. ***
Comment 24 Devin Rousso 2018-03-16 15:58:04 PDT
Created attachment 335980 [details]
Patch

I have to run out now, but I'll add a ChangeLog comment for the logic later
Comment 25 EWS Watchlist 2018-03-16 17:01:34 PDT Comment hidden (obsolete)
Comment 26 EWS Watchlist 2018-03-16 17:01:36 PDT Comment hidden (obsolete)
Comment 27 EWS Watchlist 2018-03-16 17:05:21 PDT Comment hidden (obsolete)
Comment 28 EWS Watchlist 2018-03-16 17:05:22 PDT Comment hidden (obsolete)
Comment 29 EWS Watchlist 2018-03-16 17:39:05 PDT Comment hidden (obsolete)
Comment 30 EWS Watchlist 2018-03-16 17:39:07 PDT Comment hidden (obsolete)
Comment 31 Devin Rousso 2018-03-17 00:06:34 PDT
Created attachment 336000 [details]
Patch

Should fix EWS
Comment 32 Devin Rousso 2018-03-17 00:16:56 PDT
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`.
Comment 33 EWS Watchlist 2018-03-17 01:09:14 PDT Comment hidden (obsolete)
Comment 34 EWS Watchlist 2018-03-17 01:09:15 PDT Comment hidden (obsolete)
Comment 35 EWS Watchlist 2018-03-17 01:14:03 PDT Comment hidden (obsolete)
Comment 36 EWS Watchlist 2018-03-17 01:14:04 PDT Comment hidden (obsolete)
Comment 37 EWS Watchlist 2018-03-17 01:37:07 PDT Comment hidden (obsolete)
Comment 38 EWS Watchlist 2018-03-17 01:37:08 PDT Comment hidden (obsolete)
Comment 39 Devin Rousso 2018-03-17 17:01:29 PDT
Created attachment 336020 [details]
Patch
Comment 40 Devin Rousso 2018-10-01 11:29:07 PDT
Comment on attachment 336020 [details]
Patch

r- this is not in a state ready for review.