Bug 177553 – Web Inspector: Detail Views for resources in Network Tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Detail Views for resources in Network Tab

In the new network tab, take advantage of the main content area to show detailed data about a resource instead of cramming such data into a sidebar. Having more space is easier to work with, and allows us to be a little freer / custom with styling rather than trying to fit it in the sidebar.

[Joseph Pecoraro]

Created attachment 321990 [details]
[IMAGE] Detail View with Source

[Joseph Pecoraro]

Created attachment 321991 [details]
[IMAGE] Detail View with Image

[Joseph Pecoraro]

Created attachment 321992 [details]
[IMAGE] Placeholder Detail View (NavBar without ContentView navigation items)

[Joseph Pecoraro]

Created attachment 321993 [details]
[IMAGE] Detail View Close Button Behavior

[Joseph Pecoraro]

Created attachment 321994 [details]
[PATCH] Proposed Fix

[Devin Rousso]

Comment on attachment 321994 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Main.html:600
> +    <script src="Views/FlexibleSpaceWithItemNavigationItem.js"></script>

Is there no better name for this?  :(

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:33
> +        this.element.__contentBrowser = this;

We already add `__view` to the element, so could you "piggy-back" on that?

> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceNavigationItem.css:39
> +    margin-left: auto;

You can combine this selector with the previous to form a single rule.

> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceNavigationItem.css:42
> +body[dir=rtl] :matches(.navigation-bar, .toolbar) .item.flexible-space.align-end > .item {

Ditto (39).

> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceWithItemNavigationItem.js:26
> +WI.FlexibleSpaceWithItemNavigationItem = class FlexibleSpaceWithItemNavigationItem extends WI.NavigationItem

The diff for this file might be messed up.  Bugzilla thinks it's not a new file :/

> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceWithItemNavigationItem.js:28
> +    constructor(navigationItem, alignItem)

`alignItem` seems to be used for a CSS class, so it could probably use a better name.  Maybe "alignStyle"?

> Source/WebInspectorUI/UserInterface/Views/HierarchicalPathNavigationItem.js:66
> +        for (let i = 0; this._components && i < this._components.length; ++i)

If you're going to change from var -> let, you might as well update it to a for...of too.

> Source/WebInspectorUI/UserInterface/Views/HierarchicalPathNavigationItem.js:71
> +        for (let i = 0; i < this._components.length; ++i)

Ditto (66).

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:72
> +    bottom: 0;

NIT: I usually keep the ordering as:
 - top
 - right
 - bottom
 - left

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:35
> +        this._delegate = null;

I think our typical pattern is to supply the delegate as an argument to the constructor, and not allow reassignment.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:50
> +    get delegate() { return this._delegate; }

Along the lines of (35), this isn't used, so you could remove it.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:53
> +    shown()

I think we are trying to move away from shown/hidden and more towards attached/detached, as those are more core to WI.View.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:82
> +        let contentViewNavigationItemsGroup = new WI.GroupNavigationItem([]);

I'd rather you modify GroupNavigationItem to support this.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:88
> +        this._contentBrowser = new WI.ContentBrowser(null, this, disableBackForward, disableFindBanner, contentViewNavigationItemsFlexItem, contentViewNavigationItemsGroup);

If you create const variables for some of the parameters, I'd ask that you do it for all of them.  I originally thought that the `null` was for `representedObject`, not `element`.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:90
> +        let index = 0;

Is this for future-proofing such that the items you add are always at the beginning, or is there some other reason this needs to be done?  FWICT in WI.ContentBrowser, it adds a WI.HierarchicalPathNavigationItem and WI.FlexibleSpaceNavigationItem to the NavigationBar, so if this is to make sure your items are before those, then at the very least it might be worth a comment.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:123
> +                found = true;
> +                break;

Instead of maintaining this `found` variable, you could just early return here.

> Source/WebInspectorUI/UserInterface/Views/Table.css:102
> +    top: 0;
> +    bottom: 0;
> +    left: 0;
> +    right: 0;

NIT: see above comment about ordering.

[Matt Baker]

Comment on attachment 321994 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/ChangeLog:105
> +        proper ContentView lifecycle events.

Nit: life cycle

> Source/WebInspectorUI/UserInterface/Base/Setting.js:126
> +    networkDetailContentViewIdentifier: new WI.Setting("network-detail-content-view-identifier", "preview"),

This could have a more descriptive name, like `selectedNetworkDetailContentViewIdentifier`.

>> Source/WebInspectorUI/UserInterface/Main.html:600
>> +    <script src="Views/FlexibleSpaceWithItemNavigationItem.js"></script>
> 
> Is there no better name for this?  :(

What about extending the existing FlexibleSpaceNavigationItem? That class is just a property override at the moment, and this really feels like a special kind of flexible space:

class FlexibleSpaceNavigationItem extends WI.NavigationItem
{
    constructor(navigationItem, alignItem)
    {
        super();

        this._navigationItem = null;

        if (navigationItem) {
            this._navigationItem = navigationItem;
            this.element.classList.add(alignItem || "align-start");
        }
    }

    // Protected

    get additionalClassNames()
    {
        return ["flexible-space"];
    }

    updateLayout(expandOnly)
    {
        ...
    }
}

>> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceNavigationItem.css:39
>> +    margin-left: auto;
> 
> You can combine this selector with the previous to form a single rule.

I think you can further simplify the two combined rules and remove body[dir=] from the selector by using -webkit-margin-start / -webkit-margin-end.

[Joseph Pecoraro]

Comment on attachment 321994 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:33
>> +        this.element.__contentBrowser = this;
> 
> We already add `__view` to the element, so could you "piggy-back" on that?

I didn't even think about that. Yep, I'll make the change.

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:72
>> +    bottom: 0;
> 
> NIT: I usually keep the ordering as:
>  - top
>  - right
>  - bottom
>  - left

There are 8 results for that, and 33 results for the ascending triangle of beauty:

    top: 0;
    left: 0;
    right: 0;
    bottom: 0;

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:35
>> +        this._delegate = null;
> 
> I think our typical pattern is to supply the delegate as an argument to the constructor, and not allow reassignment.

Yeah, I had considered that. I'll do it.

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:90
>> +        let index = 0;
> 
> Is this for future-proofing such that the items you add are always at the beginning, or is there some other reason this needs to be done?  FWICT in WI.ContentBrowser, it adds a WI.HierarchicalPathNavigationItem and WI.FlexibleSpaceNavigationItem to the NavigationBar, so if this is to make sure your items are before those, then at the very least it might be worth a comment.

Yes, this inserts at the start, and I use index so I can just keep inserting at index++ to insert at 0, 1, 2, 3, 4. I can add a comment.

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:123
>> +                break;
> 
> Instead of maintaining this `found` variable, you could just early return here.

Nice.

[Joseph Pecoraro]

Created attachment 322249 [details]
[PATCH] Proposed Fix

Addressed review comments.

[Joseph Pecoraro]

Comment on attachment 322249 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:419
>          let newNavigationItems = [];

FWIW in the early return a few lines above) I will add:

>          if (!currentContentView) {
>              this._removeAllNavigationItems();
>              this._currentContentViewNavigationItems = [];
> +            if (this._currentContentViewNavigationItemsGroup)
> +                this._currentContentViewNavigationItems.push(this._contentViewSelectionPathNavigationItem);
>              return;
>          }

So if there is a group the content view navigation items always contains the path navigation item. I hit this when adding a JSON view.

[Devin Rousso]

Comment on attachment 322249 [details]
[PATCH] Proposed Fix

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

r=me.  My only general comment is that it would be nice to have some hover styling, or to possibly remove the `cursor: pointer;` style.  Either would work for me :)

> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceNavigationItem.js:28
> +    constructor(navigationItem, alignItem)

WI.ScrubberNavigationItem expects the first parameter to be `identifier`.  I'd recommend changing WI.ScrubberNavigationItem to match.

> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceNavigationItem.js:56
> +        if (expandOnly) {

Instead of having this as a separate if, you can merge it with the previous one.

    if (!expandOnly || !this._navigationItem)
        return;

> Source/WebInspectorUI/UserInterface/Views/HierarchicalPathNavigationItem.js:66
> +        for (let i = 0; this._components && i < this._components.length; ++i)

You can rewrite this with a for...of.

    if (this._components) {
        for (let component of this._components)
            component.removeEventListener(WI.HierarchicalPathComponent.Event.SiblingWasSelected, this._siblingPathComponentWasSelected, this);
    }

> Source/WebInspectorUI/UserInterface/Views/HierarchicalPathNavigationItem.js:71
> +        for (let i = 0; i < this._components.length; ++i)

Ditto (66).

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:31
> +    bottom: 0;

NIT: this does look nice, but I still prefer "top, right, bottom, left" as it follows the longhand ordering of other layout properties.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.css:50
> +    background: var(--button-background-color-hover);

NIT: can you use `background-color`?

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:79
> +        let contentViewNavigationItemsGroup = new WI.GroupNavigationItem([]);

NIT: considering that you add a setter for `navigationItems` on WI.GroupNavigationItem, can you also modify the constructor to support null/undefined arguments?  This looks very annoying.

> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:85
> +        this._contentBrowser = new WI.ContentBrowser(null, this, disableBackForward, disableFindBanner, contentViewNavigationItemsFlexItem, contentViewNavigationItemsGroup);

Style: you should also have a const variable for `element`, which is null.

    const element = null;
    this._contentBrowser = new WI.ContentBrowser(element, this, disableBackForward, disableFindBanner, contentViewNavigationItemsFlexItem, contentViewNavigationItemsGroup);

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:671
> +        // FIXME: It would be nice to avoid this.
> +        // Currently the ResourceDetailView is in the heirarchy but has not yet done a layout so we
> +        // end up seeing the table behind it. This forces us to layout now instead of after a beat.
> +        this.updateLayout();

This might be because you call `this._resourceDetailView.shown()` above, before the ResourceDetailView has had a chance to call `initialLayout`.  Perhaps you should move shown() into a rAF call, or do some setTimeout trickery.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:680
> +        this._resourceDetailView.element.style[side] = this._nameColumn.width + "px";

Style: to avoid using brace operator, you could use setProperty.

    this._resourceDetailView.element.style.setProperty(side, this._nameColumn.width + "px");
    this._table.scrollContainer.style.setProperty("width", this._nameColumn.width + "px");

> Source/WebInspectorUI/UserInterface/Views/Table.js:128
> +    get scrollContainer() { return this._scrollContainerElement; }

Style: remove extra space above this line.

[Joseph Pecoraro]

Comment on attachment 322249 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Views/FlexibleSpaceNavigationItem.js:56
>> +        if (expandOnly) {
> 
> Instead of having this as a separate if, you can merge it with the previous one.
> 
>     if (!expandOnly || !this._navigationItem)
>         return;

I want to keep them separate. I realize this is a style thing but the intent of each is different:

    • First if: if this is a flexible space without an item do nothing other than the basic layout
    • Second if: if we have an item and we are expandOnly do extra work.

>> Source/WebInspectorUI/UserInterface/Views/NetworkResourceDetailView.js:85
>> +        this._contentBrowser = new WI.ContentBrowser(null, this, disableBackForward, disableFindBanner, contentViewNavigationItemsFlexItem, contentViewNavigationItemsGroup);
> 
> Style: you should also have a const variable for `element`, which is null.
> 
>     const element = null;
>     this._contentBrowser = new WI.ContentBrowser(element, this, disableBackForward, disableFindBanner, contentViewNavigationItemsFlexItem, contentViewNavigationItemsGroup);

I think we can remove this param since all `new WI.ContentBrowser` pass `null` here. (Expect a follow-up). Also I don't want to make a variable because then if I search for "new WI.ContentBrowser" it will look like there is a callsite that passes an element, when in fact its just a null.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/222868/webkit>

[Radar WebKit Bug Importer]

<rdar://problem/34817593>