[webkit-reviews] review granted: [Bug 147897] Web Inspector: Network Tab - Improve graphical representation of network waterfall : [Attachment 323926] [PATCH] Proposed Fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Thu Oct 19 15:00:55 PDT 2017


Brian Burg <bburg at apple.com> has granted Joseph Pecoraro <joepeck at webkit.org>'s
request for review:
Bug 147897: Web Inspector: Network Tab - Improve graphical representation of
network waterfall
https://bugs.webkit.org/show_bug.cgi?id=147897

Attachment 323926: [PATCH] Proposed Fix

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




--- Comment #22 from Brian Burg <bburg at apple.com> ---
Comment on attachment 323926
  --> https://bugs.webkit.org/attachment.cgi?id=323926
[PATCH] Proposed Fix

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

r=me with one quibble about the new Table.js API

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:109
> +    top: calc(var(--navigation-bar-height) - var(--timeline-ruler-height));

The future is nice. In some ways.

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.css:160
> +.waterfall .block.mouse {

NIt: can we improve .mouse to .mouse-tracking or something more explanatory?

> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:1389
> +	       let rowIndex = this._rowIndexForResource(entry.resource);

I am not sure about this bit. It's possible that the mouse block could extend
beyond the cell frame if the cell takes up the full width. This would be more
likely to be encountered if a timeline overview has applied zoom/time range to
the waterfall charts.

NSTableView exposes -frameOfCellAtColumn:row: for this purpose
(https://developer.apple.com/documentation/appkit/nstableview/1524517-frameofce
llatcolumn?language=objc). Maybe you should add that instead of
cellForRowAndColumn, since 'cell' could be almost anything and digging into it
is messy, whereas the Table can calculate the frame of the cell without caring
about the contents. For the purposes of this method, anchoring to the frame is
fine even if the waterfall might not take it all up. Practically speaking, it's
all flush to the right so there isn't much adjustment that would happen with a
tighter frame, except perhaps to move the arrow a bit.

Lastly, taking a column index rather than WI.TableColumn as the argument here
will ensure by construction that this method is only called on columns that are
visible.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1106
> +	      
headerView.element.style.setProperty(WI.resolvedLayoutDirection() ===
WI.LayoutDirection.RTL ? "right" : "left", offset + "px");

Please hoist the resolved property name.

> Source/WebInspectorUI/UserInterface/Views/Table.js:1108
> +	       headerView.updateLayout(WI.View.LayoutReason.Resize);

Cool.


More information about the webkit-reviews mailing list