[webkit-reviews] review granted: [Bug 180996] Web Inspector: Network Table - Redesign the waterfall popover showing timing data : [Attachment 329817] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Matt Baker <mattbaker at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 180996: Web Inspector: Network Table - Redesign the waterfall popover
showing timing data
https://bugs.webkit.org/show_bug.cgi?id=180996

Attachment 329817: [PATCH] Proposed Fix

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




--- Comment #11 from Matt Baker <mattbaker at apple.com> ---
Comment on attachment 329817
  --> https://bugs.webkit.org/attachment.cgi?id=329817
[PATCH] Proposed Fix

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

r=me, with a couple nits/comments.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:32
> +    color: gray;

Style: every use of `gray` could be replaced with
var(--text-color-gray-medium), but I'll leave it up to you. We should probably
just update the variable's value to be gray instead of hsl(0, 0%, 52%).

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.css:41
> +    padding-left: 5px;

Use -webkit-padding-start for this to be RTL ready.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:33
> +	   console.assert(!fixedWidth || fixedWidth >= 100, "fixedWidth must be
at least wide enough for strings");

Nit: assert description should end in a period.

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingBreakdownView.js:56
> +	   if (additionalClassName)

I was about to suggest row.classList.add("header", additionalClassName), but
learned that `null` and `undefined` get converted to "null" and "undefined" and
added as classes!

> Source/WebInspectorUI/UserInterface/Views/ResourceTimingContentView.js:97
> +	   breakdownView.updateLayout();

The breakdownView isn't being added to the view hierarchy. This is okay, so
long as it never needs to layout again (which seems to be the case, as it
implements View.initialLayout but not View.layout). The call to updateLayout
does the trick just fine here (a scheduled layout would never run), but thought
it was worth mentioning.

Earlier ResourceTimingBreakdownView is used in as popover, which isn't a part
of the View hierarchy currently, so updateLayout is required. That said,
eventually we should make Popover a view that is a child of the root
(document.body).