[webkit-reviews] review granted: [Bug 196421] Web Inspector: CPU Usage Timeline - Adjust Energy Impact Threshholds : [Attachment 366348] [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 196421: Web Inspector: CPU Usage Timeline - Adjust Energy Impact
Threshholds
https://bugs.webkit.org/show_bug.cgi?id=196421

Attachment 366348: [PATCH] Proposed Fix

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




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

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

r=me

> Source/WebInspectorUI/ChangeLog:19
> +	   - Reduce the size of the Medium section, and increase the High
section

Missing "." at the end of this line.  I'd also remove the "-" and indent, as
this "point" isn't related to a Low/High/Very High, and is instead a more
general comment (e.g. it's not part of a "list").

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1106
>	       // Low. (<=3% CPU, mapped to 0-10)

Given that these comments will have to be updated any time we change the
thresholds, I think it might be "easier" to just remove them altogether.  Each
"section" is already semi-obvious by the `UIString`s in each block.

> Source/WebInspectorUI/UserInterface/Views/CPUTimelineView.js:1109
> +	       this._energyChart.value = mapWithBias(average, 0,
CPUTimelineView.lowEnergyThreshold, 0, CPUTimelineView.lowEnergyGraphBoundary,
0.85);

Do you also want to make the biases into `static get`?