[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 bugzilla-daemon at webkit.org
Tue Dec 19 16:46:03 PST 2017

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

Attachment 329817: [PATCH] Proposed Fix


--- 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

More information about the webkit-reviews mailing list