[webkit-reviews] review granted: [Bug 196316] Web Inspector: Show Resource Initiator in Network Tab detail views : [Attachment 366110] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 196316: Web Inspector: Show Resource Initiator in Network Tab detail views
https://bugs.webkit.org/show_bug.cgi?id=196316

Attachment 366110: [PATCH] Proposed Fix

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




--- Comment #9 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 366110
  --> https://bugs.webkit.org/attachment.cgi?id=366110
[PATCH] Proposed Fix

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

r=me, nice!

Will be ready to land once the popover issue is resolved.

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:309
> +	       initiatorCallFrames:
this._initiatorCallFramesFromPayload(initiator),

isn't this so much nicer than having to update every `new WI.Resource` :)

> Source/WebInspectorUI/UserInterface/Controllers/NetworkManager.js:776
> +	   return callFrames.map((payload) =>
WI.CallFrame.fromPayload(WI.assumingMainTarget(), payload));

Should we be using `WI.StackTrace` instead?  That way, we "automatically" get
support for async stack traces (if we decide to support that).	You'd have to
mess with the `payload` a bit (`WI.StackTrace` expects a `callFrames` array in
the payload), but it may be simpler (and provide more API options), such as
possibly letting you eliminate the "custom" logic inside
`_initiatorSourceCodeLocationFromPayload`.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.css:62
> +    content: url(../Images/CallStack.svg);

NIT: I normally put `content` after and positional/sizing/spacing properties
(e.g. `margin-left`).

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:124
> +    hidden()

Please call `super.hidden()`.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:264
> +	       const options = {

You could inline this.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:276
> +		   link.insertAdjacentElement("afterend", callStackElement);

Alternatively, you could create a `document.createDocumentFragment` and move
the `this._summarySection.appendKeyValuePair` call to below the `if
(callFrames) {`.
```
    let fragment = document.createDocumentFragment();

    let link = WI.createSourceCodeLocationLink(initiatorLocation, {
	dontFloat: true,
	ignoreSearchTab: true,
    });
    fragment.appendChild(link);

    let callFrames = this._resource.initiatorCallFrames;
    if (callFrames) {
	let callStackElement =
fragment.appendChild(document.createElement("img"));
	...
    }

    let pair =
this._summarySection.appendKeyValuePair(WI.UIString("Initiator"), fragment);
    pair.classList.add("initiator");
```

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:282
> +			       let bounds =
WI.Rect.rectFromClientRect(callStackElement.getBoundingClientRect());

Since this section can get "refreshed" (meaning we'd create a new
`callStackElement` with a new "click" handler), you'd need to either save a
reference to `this._callStackElement`, hide/reset `this._popover` each time the
"click" handler is called, or update the `windowReziseHandler` each time the
"click" event handler is called.

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:298
> +		       let bounds =
WI.Rect.rectFromClientRect(callStackElement.getBoundingClientRect());
> +		       this._popover.present(bounds.pad(2), [WI.RectEdge.MAX_Y,
WI.RectEdge.MIN_Y, WI.RectEdge.MAX_X]);

Given that this is the same logic as `windowResizeHandler`, I'd pull both of
them into a single function `presentBelowCallStackElement` and call it in each
place.