[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 bugzilla-daemon at webkit.org
Mon Apr 1 10:30:37 PDT 2019

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

Attachment 366348: [PATCH] Proposed Fix


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


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

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,

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

More information about the webkit-reviews mailing list