[webkit-reviews] review denied: [Bug 189606] Web Inspector: group media network entries by the node that triggered the request : [Attachment 351480] Patch

[bugzilla-daemon at webkit.org]

Brian Burg <bburg at apple.com> has denied Devin Rousso <drousso at apple.com>'s
request for review:
Bug 189606: Web Inspector: group media network entries by the node that
triggered the request
https://bugs.webkit.org/show_bug.cgi?id=189606

Attachment 351480: Patch

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




--- Comment #42 from Brian Burg <bburg at apple.com> ---
Comment on attachment 351480
  --> https://bugs.webkit.org/attachment.cgi?id=351480
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.