Bug 196316 - Web Inspector: Show Resource Initiator in Network Tab detail views
Summary: Web Inspector: Show Resource Initiator in Network Tab detail views
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-27 14:13 PDT by Joseph Pecoraro
Modified: 2019-03-28 11:47 PDT (History)
4 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (10.44 KB, patch)
2019-03-27 14:17 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[IMAGE] Dark (930.74 KB, image/png)
2019-03-27 14:18 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light (904.69 KB, image/png)
2019-03-27 14:18 PDT, Joseph Pecoraro
no flags Details
[PATCH] Proposed Fix (11.60 KB, patch)
2019-03-27 15:38 PDT, Joseph Pecoraro
drousso: review+
drousso: commit-queue-
Details | Formatted Diff | Diff
[IMAGE] Dark (858.21 KB, image/png)
2019-03-27 15:39 PDT, Joseph Pecoraro
no flags Details
[IMAGE] Light (839.72 KB, image/png)
2019-03-27 15:39 PDT, Joseph Pecoraro
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2019-03-27 14:13:05 PDT
Show Resource Initiator in Network Tab detail views

The Initiator information (such as a backtrace or location where it was requested) is only shown:

  • in the Network table (location link)
  • in the Resource Details Sidebar (location link)

But the full call frames are available and never used, and the Detail views don't include this info. We should show it somewhere.
Comment 1 Radar WebKit Bug Importer 2019-03-27 14:13:39 PDT
<rdar://problem/49352679>
Comment 2 Joseph Pecoraro 2019-03-27 14:17:51 PDT
Created attachment 366103 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2019-03-27 14:18:15 PDT
Created attachment 366104 [details]
[IMAGE] Dark
Comment 4 Joseph Pecoraro 2019-03-27 14:18:26 PDT
Created attachment 366105 [details]
[IMAGE] Light
Comment 5 Joseph Pecoraro 2019-03-27 14:38:53 PDT
I'd like to find some place to put the content that is not behind a click, but instead always visible. Perhaps an Initiator section near the bottom that always shows the stack. But for now I think this is a good idea.

It is possible that the use of the eye here conflates with the use of the eye in the Console. I think another icon we could use is a circle with 2 lines in it (=). I can try to implement such an icon.
Comment 6 Joseph Pecoraro 2019-03-27 15:38:55 PDT
Created attachment 366110 [details]
[PATCH] Proposed Fix
Comment 7 Joseph Pecoraro 2019-03-27 15:39:20 PDT
Created attachment 366111 [details]
[IMAGE] Dark
Comment 8 Joseph Pecoraro 2019-03-27 15:39:32 PDT
Created attachment 366112 [details]
[IMAGE] Light
Comment 9 Devin Rousso 2019-03-28 10:54:38 PDT
Comment on attachment 366110 [details]
[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.
Comment 10 Joseph Pecoraro 2019-03-28 11:24:31 PDT
Comment on attachment 366110 [details]
[PATCH] Proposed Fix

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

>> 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`.

If/when the backend sends a Console.StackTrace payload I'll use that, but for now it sends a list of Console.CallFrame payloads, so I'll stick with this for now.

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

Good point.
Comment 11 Joseph Pecoraro 2019-03-28 11:47:38 PDT
<https://trac.webkit.org/r243614>