[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 bugzilla-daemon at webkit.org
Thu Mar 28 10:54:38 PDT 2019

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

Attachment 366110: [PATCH] Proposed Fix


--- 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:

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

> 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,

    let callFrames = this._resource.initiatorCallFrames;
    if (callFrames) {
	let callStackElement =

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

> Source/WebInspectorUI/UserInterface/Views/ResourceHeadersContentView.js:282
> +			       let bounds =

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 =
> +		       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

More information about the webkit-reviews mailing list