[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