[webkit-reviews] review granted: [Bug 195202] Web Inspector: CPU Usage Timeline - Statistics and Sources sections : [Attachment 363725] [PATCH] Proposed Fix

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Joseph Pecoraro
<joepeck at webkit.org>'s request for review:
Bug 195202: Web Inspector: CPU Usage Timeline - Statistics and Sources sections
https://bugs.webkit.org/show_bug.cgi?id=195202

Attachment 363725: [PATCH] Proposed Fix

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




--- Comment #20 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 363725
  --> https://bugs.webkit.org/attachment.cgi?id=363725
[PATCH] Proposed Fix

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

r=me

I still think we should try to move away from calling `updateLayout` (with the
exception of the "toggle" event), but in the interest of getting this landed
for further features we can do that in a followup (not to mention this is fully
working in the state it's in right now).

I've created two followup bugs:
 - <https://webkit.org/b/195356> Web Inspector: CPU Usage Timeline - leverage
CSS grid for Statistics and Sources sections
 - <https://webkit.org/b/195357> Web Inspector: CPU Usage Timeline - don't
eagerly render the Statistics and Sources sections

> LayoutTests/inspector/unit-tests/map-utilities.html:8
> +    let suite = InspectorTest.createSyncSuite("MapUtilities");

NIT: I'd just call this `Map` so that it matches as a prefix for the individual
test cases.

If you were feeling like it, you could also write tests for `Map.fromObject`
and `Map.prototype.take` :D

> LayoutTests/inspector/unit-tests/map-utilities.html:24
> +	       return true;

NIT: I've come to learn that this isn't actually necessary for a sync suite. 
You can omit a return and it assumes a pass.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.css:395
> +	   background-color: hsl(0, 0%, 33%);

`var(--gray-foreground-color)`?

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:338
> +	   let statisticsContainerElement =
createChartContainer(bottomOverviewElement, WI.UIString("Statistics"), "");

Rather than have an empty string, could you modify `createChartContainer` to
only set the `title` if a string is actually provided?	It looks weird to have
a falsy value hanging off the end of a function call.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:355
> +	   let sourcesContainerElement =
createChartContainer(bottomOverviewElement, WI.UIString("Sources"), "");

Ditto (>338).

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:745
> +    _layoutStatisticsAndSources()

Personally, I think this is a bit unnecessary, but considering how often it's
called, I can see why it is nice.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:795
> +	       let entries = Array.from(map.entries());

You can drop the `.entries()`.	The default iterator for a `Map` is the same as
calling `.entries()`.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1411
> +    _insertTableRow(table, rowList, heading, numberValue, labelValue,
followingRow = null)

Was there a reason you didn't change this to be an optional object?  All of the
places where you pass `""` as a parameter are pretty confusing to follow, to
the point where I actually had to count out the arguments to understand which
argument they corresponded to.	I think we should avoid cases where we pass
inlined falsy values to functions, as they are often very hard to follow.  It's
even more confusing given that one of the parameters actually is optional
(`followingRow`).  If you're worried about performance, then I understand not
wanting to use a temporary object, but at the very least reorder the parameters
and make so that the optional values (e.g. `heading` and `labelValue`) don't
have to be supplied in any way when not needed (at the same time, there's no
real reason to have `followingRow` have a default value either, since
`undefined` and `null` are both falsy).