[webkit-reviews] review granted: [Bug 177553] Web Inspector: Detail Views for resources in Network Tab : [Attachment 322249] [PATCH] Proposed Fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Oct 3 17:56:32 PDT 2017


Devin Rousso <webkit at devinrousso.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 177553: Web Inspector: Detail Views for resources in Network Tab
https://bugs.webkit.org/show_bug.cgi?id=177553

Attachment 322249: [PATCH] Proposed Fix

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




--- Comment #11 from Devin Rousso <webkit at devinrousso.com> ---
Comment on attachment 322249
  --> https://bugs.webkit.org/attachment.cgi?id=322249
[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.SiblingWasSele
cted, 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.


More information about the webkit-reviews mailing list