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.
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
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
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
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.
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.
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
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
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
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
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
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
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
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
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
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.
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.
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.
(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).
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.
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.
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.
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
2018-09-13 23:24 PDT, Devin Rousso
2018-09-14 00:39 PDT, EWS Watchlist
2018-09-14 00:57 PDT, EWS Watchlist
2018-09-14 01:26 PDT, EWS Watchlist
2018-09-14 10:26 PDT, Devin Rousso
2018-09-15 11:00 PDT, Devin Rousso
2018-09-15 12:13 PDT, EWS Watchlist
2018-09-15 12:30 PDT, EWS Watchlist
2018-09-15 12:53 PDT, EWS Watchlist
2018-09-17 11:25 PDT, Devin Rousso
2018-09-17 12:46 PDT, EWS Watchlist
2018-09-17 13:02 PDT, EWS Watchlist
2018-09-17 13:25 PDT, EWS Watchlist
2018-09-17 16:18 PDT, Devin Rousso
2018-09-17 17:40 PDT, EWS Watchlist
2018-09-17 18:24 PDT, EWS Watchlist
2018-09-17 18:38 PDT, EWS Watchlist
2018-09-20 10:44 PDT, Devin Rousso
2018-09-20 11:44 PDT, Devin Rousso
2018-09-21 17:31 PDT, Devin Rousso
2018-10-02 22:37 PDT, Devin Rousso
2018-10-06 10:51 PDT, Devin Rousso
2018-10-08 10:09 PDT, Devin Rousso