Web Inspector: group media network entries by the node that triggered the request

[Devin Rousso]

When working with media elements (e.g. <video> and <audio>), especially those with multiple video/audio/subtitle tracks, we could do a better job of displaying request information in a way that's easily relatable and understandable.  As an example, most (if not all) video files are loaded in segments via the “Range” request header, and each of these “loads” appeared in the table as a separate request to the exact same URL.  This is confusing, especially considering the fact that the difference between these requests only becomes apparent when one views the headers for the requests and perceives the difference in the “Range”.  We should improve the Network table to better display important information and group these types of loads together.

[Radar WebKit Bug Importer]

<rdar://problem/44438527>

[Devin Rousso]

Created attachment 349738 [details]
Patch

[EWS Watchlist]

Comment on attachment 349738 [details]
Patch

Attachment 349738 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9211146

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/model/frame-extra-scripts.html
inspector/model/script-resource-relationship.html
http/tests/inspector/network/resource-initiatorNode.html
inspector/model/stack-trace.html
inspector/canvas/requestNode.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 349745 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349738 [details]
Patch

Attachment 349738 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9211230

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/model/frame-extra-scripts.html
inspector/model/script-resource-relationship.html
http/tests/inspector/network/resource-initiatorNode.html
inspector/model/stack-trace.html
inspector/canvas/requestNode.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 349747 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349738 [details]
Patch

Attachment 349738 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9211334

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/model/frame-extra-scripts.html
inspector/model/script-resource-relationship.html
http/tests/inspector/network/resource-initiatorNode.html
http/tests/media/clearkey/collect-webkit-media-session.html
inspector/model/stack-trace.html
inspector/canvas/requestNode.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 349749 [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

[Devin Rousso]

Created attachment 349771 [details]
[Image] After Patch is applied

[Joseph Pecoraro]

Comment on attachment 349738 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=349738&action=review

> Source/WebCore/platform/network/ResourceRequestBase.h:239
> +    RefPtr<Node> m_initiatorNode;

As discussed in person, having ResourceRequest reference a WebCore::Node is a layering violation. The platform layer shouldn't know about WebCore types.

[Devin Rousso]

Created attachment 349858 [details]
Patch

Some of these `InspectorInstrumentation` calls are probably unnecessary, so I'll try removing them one by one later.  This is mainly to see if the tests pass on EWS, although it is still reviewable.

[EWS Watchlist]

Comment on attachment 349858 [details]
Patch

Attachment 349858 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9228999

New failing tests:
inspector/model/stack-trace.html
inspector/model/frame-extra-scripts.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html
inspector/model/script-resource-relationship.html

[EWS Watchlist]

Created attachment 349861 [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

[EWS Watchlist]

Comment on attachment 349858 [details]
Patch

Attachment 349858 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9229097

New failing tests:
inspector/model/stack-trace.html
inspector/model/frame-extra-scripts.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html
inspector/model/script-resource-relationship.html

[EWS Watchlist]

Created attachment 349862 [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

[EWS Watchlist]

Comment on attachment 349858 [details]
Patch

Attachment 349858 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9229112

New failing tests:
inspector/model/stack-trace.html
inspector/model/frame-extra-scripts.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html
inspector/model/script-resource-relationship.html

[EWS Watchlist]

Created attachment 349863 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 349913 [details]
Patch

[EWS Watchlist]

Comment on attachment 349913 [details]
Patch

Attachment 349913 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9245738

New failing tests:
inspector/model/stack-trace.html
inspector/model/frame-extra-scripts.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html
inspector/model/script-resource-relationship.html

[EWS Watchlist]

Created attachment 349918 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349913 [details]
Patch

Attachment 349913 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9245768

New failing tests:
inspector/model/stack-trace.html
inspector/model/frame-extra-scripts.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html
inspector/model/script-resource-relationship.html

[EWS Watchlist]

Created attachment 349920 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349913 [details]
Patch

Attachment 349913 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9245806

New failing tests:
inspector/model/stack-trace.html
inspector/model/frame-extra-scripts.html
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html
inspector/model/script-resource-relationship.html

[EWS Watchlist]

Created attachment 349930 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 349967 [details]
Patch

Fixed a typo that was causing the unexpected test failures.

[EWS Watchlist]

Comment on attachment 349967 [details]
Patch

Attachment 349967 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9250145

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 349980 [details]
Archive of layout-test-results from ews103 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349967 [details]
Patch

Attachment 349967 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9250440

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 349987 [details]
Archive of layout-test-results from ews117 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 349967 [details]
Patch

Attachment 349967 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9250615

New failing tests:
inspector/canvas/create-context-bitmaprenderer.html
inspector/canvas/create-context-webgl.html

[EWS Watchlist]

Created attachment 349988 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Devin Rousso]

Created attachment 350230 [details]
Patch

The canvas test failures were caused by `InspectorCanvas::buildObjectForCanvas` eagerly binding the canvas' node in `InspectorDOMAgent`, which will not be removed if the canvas' node is detached from the DOM since we unbind nodes in `InspectorDOMAgent` when they are removed from their parent.

[Devin Rousso]

Created attachment 350239 [details]
Patch

[Devin Rousso]

Created attachment 350463 [details]
Patch

localizedStrings :|

[Joseph Pecoraro]

Comment on attachment 350463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350463&action=review

Here is a first pass, I didn't review the NetworkTable changes closely, but I skimmed the rest of the patch to try and get a high level idea.

> LayoutTests/http/tests/inspector/network/har/har-basic.html:-39
> -            const loaderIdentifier = undefined;
> -            const targetId = undefined;
> -            const requestIdentifier = undefined;

why not keep these, they would still be undefined below, right?

> Source/WebCore/ChangeLog:21
> +        Handles initial (e.g. DNT) resource requests.

What is DNT here? Do Not Track?

> Source/WebCore/ChangeLog:29
> +        (MediaResourceLoader::requestResource):
> +        Handles byte-range requests.

I wonder why the other locations don't set the requester like here:

    request.setRequester(ResourceRequest::Requester::Media)

> Source/WebCore/ChangeLog:71
> +        Don't try to push the canvas' node to the frontend, as this will create a dangling node in
> +        `InspectorDOMAgent` if the canvas' node is detached from the DOM.

I think you can mention you intend to add this back in the future? I think you had mentioned this might be the case after updating DOM.NodeId to uniquely identify nodes through the entire node's lifetime.

> Source/WebCore/platform/network/ResourceRequestBase.h:239
> +    int m_initiatorNodeIdentifier;

All of these others have an initial value, you should set that here as well:

    int m_initatorNodeIdentifier { 0 };

> Source/WebInspectorUI/ChangeLog:85
> +        * UserInterface/Images/Range.svg: Added.

I think `ByteRange.svg` might be a better name than `Range.svg`.

I had always thought a [#] might be better than [R].
Another idea I've had for other byte-wise data was something like:
    
    _______
    | 0 1 | 
    | 1 0 |
    ⎺⎺⎺⎺⎺⎺⎺

JonDavis / mattbaker / xenon may know what it takes to create a new such icon path from a previously unused character. I have never created a new one myself!

> Source/WebInspectorUI/UserInterface/Images/Range.svg:2
> +<!-- Copyright © 2013 Apple Inc. All rights reserved. -->

2013 or 2018?

> Source/WebInspectorUI/UserInterface/Models/Resource.js:78
> +        this._target = targetIdentifier ? WI.targetManager.targetForIdentifier(targetIdentifier) : WI.mainTarget;

We use the identifier `targetId` everywhere in the frontend, and never `targetIdentifier`. I'm not sure we'd want to change that here.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:101
> +.network-table li.selected .cell.domain > .lock {
> +    filter: invert();
> +}

I'm not sure this is needed, we shouldn't ever be showing the whole row as selected in the Network Tab. (Maybe if we bring this Network Table to the Timeline Tab it would be needed).

[Devin Rousso]

Comment on attachment 350463 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=350463&action=review

>> LayoutTests/http/tests/inspector/network/har/har-basic.html:-39
>> -            const requestIdentifier = undefined;
> 
> why not keep these, they would still be undefined below, right?

Since `WI.Resource` now uses an object-bag, these values are unnecessary as they will effectively get set to undefined when the object-bag gets destructured.  Additionally, this makes the test a bit easier to read because now it's clear what's been specified and what's left "blank".

>> Source/WebCore/ChangeLog:21
>> +        Handles initial (e.g. DNT) resource requests.
> 
> What is DNT here? Do Not Track?

Yup!

>> Source/WebCore/ChangeLog:71
>> +        `InspectorDOMAgent` if the canvas' node is detached from the DOM.
> 
> I think you can mention you intend to add this back in the future? I think you had mentioned this might be the case after updating DOM.NodeId to uniquely identify nodes through the entire node's lifetime.

Yeah, I'll add a comment linking to one of these:
 - <https://webkit.org/b/178282>
 - <https://webkit.org/b/189687>
 - <https://webkit.org/b/189776>

>> Source/WebInspectorUI/UserInterface/Images/Range.svg:2
>> +<!-- Copyright © 2013 Apple Inc. All rights reserved. -->
> 
> 2013 or 2018?

I would think 2013 since this file is a direct copy of one of the other [R] icons, but I'm not the best person to ask about copyright 😅

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:78
>> +        this._target = targetIdentifier ? WI.targetManager.targetForIdentifier(targetIdentifier) : WI.mainTarget;
> 
> We use the identifier `targetId` everywhere in the frontend, and never `targetIdentifier`. I'm not sure we'd want to change that here.

I think my main motivation was to match `loaderIdentifier` and `requestIdentifier`, but it's unnecessary.  I'll revert.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:101
>> +}
> 
> I'm not sure this is needed, we shouldn't ever be showing the whole row as selected in the Network Tab. (Maybe if we bring this Network Table to the Timeline Tab it would be needed).

When looking at this patch alone, it is, because it's allowed for the user to select a node's row.  The followup <https://webkit.org/b/189773> removes this functionality by replacing the rest of the columns with the timeline.

[Devin Rousso]

Created attachment 351480 [details]
Patch

[BJ Burg]

Comment on attachment 351480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review

> Source/WebCore/ChangeLog:11
> +        Add extra arguments to functions that create `ResourceRequest` objets for media resources so

Nit: objects

> Source/WebCore/html/track/LoadableTextTrack.cpp:-85
> -    if (!m_loader->load(m_url, m_trackElement->mediaElementCrossOriginAttribute(), m_trackElement->isInUserAgentShadowTree()))

Why did this change?

> Source/WebCore/inspector/InspectorCanvas.cpp:-253
> -        else {

I don't quite understand why this change belongs in this patch.

> Source/WebCore/loader/TextTrackLoader.cpp:147
> +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element)

... Oh, i see. Jer, is this a layering violation for this part of the code?

> Source/WebCore/platform/network/ResourceRequestBase.h:176
> +    int initiatorNodeIdentifier() const { return m_initiatorNodeIdentifier; }

Please use Identified.h templates for this. It should be std::optional too.

[Devin Rousso]

Comment on attachment 351480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review

>> Source/WebCore/inspector/InspectorCanvas.cpp:-253
>> -        else {
> 
> I don't quite understand why this change belongs in this patch.

It's more-so that this was never used by the frontend.  Since this patch adds calls to `WI.domManager.ensureDocument()` whenever the document changes, it would effect this code.  The change to "inspector/canvas/requestNode.html" is for the same reason.

>> Source/WebCore/loader/TextTrackLoader.cpp:147
>> +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element)
> 
> ... Oh, i see. Jer, is this a layering violation for this part of the code?

I might need some clarity as to what we consider a layering violation.

`TextTrackLoader` holds a reference to a `TextTrackLoaderClient`, which I think is always a `LoadableTextTrack`, which has access to its `HTMLTrackElement`.  Not sure if that is a similar concept to our "delegate" in WebInspector, but if so I can probably reach the `HTMLTrackElement` via the client.

>> Source/WebCore/platform/network/ResourceRequestBase.h:176
>> +    int initiatorNodeIdentifier() const { return m_initiatorNodeIdentifier; }
> 
> Please use Identified.h templates for this. It should be std::optional too.

I've never used `Identified` before.  Based on what I can understand from the code, I'm not sure that it's what we want here.  The reason this member exists is so that we can relate a specific request to the "node" that effectively triggered that request.  In this patch, this value is set to `InspectorDOMAgentt`s id for the node, which can then be properly propagated until it reaches `InspectorNetworkAgent`.  This value isn't supposed to be an identifier for the request itself, but instead used as a way for the inspector to relate this request to something else it has already identified.

Making it an `optional` seem's like a good idea though.

[Jer Noble]

(In reply to Brian Burg from comment #38)
> Comment on attachment 351480 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351480&action=review
>
> > Source/WebCore/loader/TextTrackLoader.cpp:147
> > +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element)
> 
> ... Oh, i see. Jer, is this a layering violation for this part of the code?

I don't think so; there's already a bunch of code in WebCore/loader than references HTMLMediaElements (as well as other HTML element types).

[BJ Burg]

Comment on attachment 351480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review

>>> Source/WebCore/loader/TextTrackLoader.cpp:147
>>> +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element)
>> 
>> ... Oh, i see. Jer, is this a layering violation for this part of the code?
> 
> I might need some clarity as to what we consider a layering violation.
> 
> `TextTrackLoader` holds a reference to a `TextTrackLoaderClient`, which I think is always a `LoadableTextTrack`, which has access to its `HTMLTrackElement`.  Not sure if that is a similar concept to our "delegate" in WebInspector, but if so I can probably reach the `HTMLTrackElement` via the client.

Right. I'm not sure if it's okay in this code for a Loader to hold on to an Element subclass instance, or if they are supposed to be abstracted away with a *Client interface.

>>> Source/WebCore/platform/network/ResourceRequestBase.h:176
>>> +    int initiatorNodeIdentifier() const { return m_initiatorNodeIdentifier; }
>> 
>> Please use Identified.h templates for this. It should be std::optional too.
> 
> I've never used `Identified` before.  Based on what I can understand from the code, I'm not sure that it's what we want here.  The reason this member exists is so that we can relate a specific request to the "node" that effectively triggered that request.  In this patch, this value is set to `InspectorDOMAgentt`s id for the node, which can then be properly propagated until it reaches `InspectorNetworkAgent`.  This value isn't supposed to be an identifier for the request itself, but instead used as a way for the inspector to relate this request to something else it has already identified.
> 
> Making it an `optional` seem's like a good idea though.

I read into this more, and I think Identified would be a great replacement for `int`, but it would require significant code changes in InspectorDOMAgent. Perhaps let's defer this change til when you rework that agent? In the meantime, I would guess that other engineers prefer at least a more informative typedef for the ResourceRequest member, like Inspector::NodeIdentifier.

> Source/WebInspectorUI/ChangeLog:10
> +        node that initiated the load (if appliable). When grouped by node, a tree-like layout of the

Nit: if applicable

> Source/WebInspectorUI/ChangeLog:20
> +        Whenever the frame navigates, re-reqeust the document so that `NetworkAgent` is able to send

Nit: re-request

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:157
> +                mainResource = new WI.Resource(framePayload.url, {

I like the new style, but please make a separate patch for it. It's kind of messy and this patch is already large.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:573
> +    _addNewResourceToFrameOrTarget(requestIdentifier, frameIdentifier, loaderIdentifier, url, type, requestMethod, requestHeaders, requestData, elapsedTime, walltime, frameName, frameSecurityOrigin, initiatorSourceCodeLocation, initiatorNode, originalRequestWillBeSentTimestamp, targetId)

Thought: it's regrettable that this function signature is so coupled to WI.Resource. Maybe the function should just take a WI.Resource and register it, someday.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:-570
> -                resource = new WI.Resource(url, null, type, loaderIdentifier, targetId, requestIdentifier, requestMethod, requestHeaders, requestData, elapsedTime, walltime, initiatorSourceCodeLocation, originalRequestWillBeSentTimestamp);

Thought: similarly, there is a bunch of weird knowledge here. I wonder if these code paths are only triggered by one or a few callers.

[BJ Burg]

Comment on attachment 351480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review

This is almost ready to land. Here's what's left to do:

- Fix localization issue for byte range
- Move WI.Resource constructor refactor to another patch
- Address transferSize vs resourceSize computation for initiator node entries

>>>>> Source/WebCore/loader/TextTrackLoader.cpp:147
>>>>> +bool TextTrackLoader::load(const URL& url, HTMLTrackElement& element)
>>>> 
>>>> ... Oh, i see. Jer, is this a layering violation for this part of the code?
>>> 
>>> I might need some clarity as to what we consider a layering violation.
>>> 
>>> `TextTrackLoader` holds a reference to a `TextTrackLoaderClient`, which I think is always a `LoadableTextTrack`, which has access to its `HTMLTrackElement`.  Not sure if that is a similar concept to our "delegate" in WebInspector, but if so I can probably reach the `HTMLTrackElement` via the client.
>> 
>> I don't think so; there's already a bunch of code in WebCore/loader than references HTMLMediaElements (as well as other HTML element types).
> 
> Right. I'm not sure if it's okay in this code for a Loader to hold on to an Element subclass instance, or if they are supposed to be abstracted away with a *Client interface.

Ok, cool.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:389
> +        let setTextContent = (accessor) => {

Very cool.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:390
> +            let uniqueValues = this._uniqueValuesForDOMNodeEntry(entry, accessor);

Thought: It would be a little more definitive to do:

let entryRepresentsAnInitiator = !entry.resource // or some other condition
if (entryRepresentsAnInitiator) {
let uniqueValues = this._uniqueValuesForInitiatorEntry(entry, accessor);
if (uniqueValues.size() > 1)
...

We may want to expose non-node initiators (i.e., script) in the future and display them the same way. This would codify how initiators ought to be displayed when Group By Node (sic) is enabled.

EDIT: looking at the screenshot, it seems I'm confusing initiatorNode with the script location that's labeled 'Initiator'. Let's keep this as-is; in the future it would be worth exploring this idea (initiator tree in network table). It would be tricky to represent multi-level initiator chains without running out of room.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:443
> +            var resourceSize = entry.resourceSize;

Any reason to not use `let` and introduce a block scope?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:446
> +                resourceSize = resourceEntries.reduce((accumulator, current) => accumulator + (current.transferSize || 0), 0);

Why does this use transferSize instead of resourceSize? This seems wrong.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:449
>          case "transferSize":

Does the transferSize case work correctly when some of the things are (memory) cached? EDIT: I see, you added support in _populateTransferSizeCell. Nice.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:519
> +                    nameElement.textContent = WI.UIString("Byte Range %s").format(resource.requestHeaders["Range"].replace("bytes=", ""));

I have a feeling this is not going to localize well.
- This is a raw header, the value might be garbage.
- In the English localization, it should use an en-dash to represent a range of numbers.
- In other localizations, the range may be represented with different punctuation.

I'd recommend to extract the two numbers separately then format them into a localized string like "Byte Range %d–%d". Maybe the resource itself could expose the byte range as a getter that returns null or an object {start: end:}.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:-669
> -            comparator = comparator = (a, b) => a.startTime - b.startTime;

Haha

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:823
> +        this._entriesSortComparator = (a, b) => reverseFactor * comparator(substitute(a, b), substitute(b, a));

This really needs a comment and/or a better name than 'substitute'. I'm caffeinated and still going crosseyed trying to understand it. I think it's replacing `entry` with the first resource that shares the same initiator node if it has one and `other` does not have the same initiator node. But the net effect isn't obvious.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1033
> +

Nit: extra lines

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1107
> +        function updateEntry(existing) {

Capturing 'entry' here isn't making this any clearer. Maybe updateExistingFromEntry(existing, entry).

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1407
> +    _entryForDOMNode(domNode, requiredFields)

What's requiredFields for? I see we pass Object.keys(resourceEntry) but nothing happens with it.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1409
> +        return {

I got a little confused that DOM node "entries" are completely different from resource entries, which are part of the Table API.

[Devin Rousso]

Comment on attachment 351480 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351480&action=review

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:446
>> +                resourceSize = resourceEntries.reduce((accumulator, current) => accumulator + (current.transferSize || 0), 0);
> 
> Why does this use transferSize instead of resourceSize? This seems wrong.

I think this is a copypasta mistake 😅

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1107
>> +        function updateEntry(existing) {
> 
> Capturing 'entry' here isn't making this any clearer. Maybe updateExistingFromEntry(existing, entry).

If I do that, then we have a shadowed variable `entry`.  I'll think of a better name.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1407
>> +    _entryForDOMNode(domNode, requiredFields)
> 
> What's requiredFields for? I see we pass Object.keys(resourceEntry) but nothing happens with it.

Leftover from a previous version.

>> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1409
>> +        return {
> 
> I got a little confused that DOM node "entries" are completely different from resource entries, which are part of the Table API.

I don't think the Table API says anything about "entries", or really even knows about it.  It just asks for a number of rows and then tells us which `rowIndex` it's populating when it tries to render.  This follows that "pattern" in the sense that this is an "entry", just not for a resource.

[Devin Rousso]

Created attachment 351728 [details]
Patch

[BJ Burg]

Comment on attachment 351728 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=351728&action=review

r=me, great work!

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:818
> +        // If the entry has an `initiatorNode`, use that node's "first" resource as the value of

Thanks.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:821
> +        // next to eachother, as they will all effectively be sorted by the first resource.

Nit: each other

[Devin Rousso]

Created attachment 351785 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 351785 [details]
Patch

Clearing flags on attachment: 351785

Committed r236927: <https://trac.webkit.org/changeset/236927>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Looks like the new test http/tests/inspector/network/resource-initiatorNode.html

added in https://trac.webkit.org/changeset/236927/webkit

is a flakey timeout.

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Finspector%2Fnetwork%2Fresource-initiatorNode.html

It is highly intermittent and only showed up recently.