[webkit-reviews] review granted: [Bug 195296] Web Inspector: Timelines: convert WI.RangeChart to use a <canvas> : [Attachment 363559] Patch
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon Mar 4 15:49:20 PST 2019
Joseph Pecoraro <joepeck at webkit.org> has granted Devin Rousso
<drousso at apple.com>'s request for review:
Bug 195296: Web Inspector: Timelines: convert WI.RangeChart to use a <canvas>
https://bugs.webkit.org/show_bug.cgi?id=195296
Attachment 363559: Patch
https://bugs.webkit.org/attachment.cgi?id=363559&action=review
--- Comment #2 from Joseph Pecoraro <joepeck at webkit.org> ---
Comment on attachment 363559
--> https://bugs.webkit.org/attachment.cgi?id=363559
Patch
View in context: https://bugs.webkit.org/attachment.cgi?id=363559&action=review
r=me
> Source/WebInspectorUI/ChangeLog:3
> + Web Inspector: Timelines: convert WI.RangeChart to use a <canvas>
Awesome! Can you put in the ChangeLog that you were seeing ~4-5x speedup on
large ranges.
> Source/WebInspectorUI/UserInterface/Views/CPUUsageIndicatorView.js:102
> + let computedStyle = window.getComputedStyle(this.element);
We should only do this in the `!WI.CPUUsageIndicatorView._cachedFillColor`
block to avoid doing it every time.
> Source/WebInspectorUI/UserInterface/Views/RangeChart.js:31
> // in the chart by providing an (x, w, class) via `addRange`.
This API changed to not be a className but instead be a set of fill/stroke
colors.
> Source/WebInspectorUI/UserInterface/Views/RangeChart.js:-43
> -//
> -// SVG:
> -//
> -// - There is a single rect for each range.
> -//
> -// <div class="range-chart">
> -// <svg viewBox="0 0 800 75">
> -// <rect width="<w>" height="100%" transform="translateX(<x>)"
class="<class>" />
> -// <rect width="<w>" height="100%" transform="translateX(<x>)"
class="<class>" />
> -// ...
> -// </svg>
> -// </div>
Might be worth saying this uses a canvas and draws rears.
> Source/WebInspectorUI/UserInterface/Views/RangeChart.js:95
> + x *= window.devicePixelRatio;
> + width *= devicePixelRatio;
Nit: `window.devicePixelRatio`.
> Source/WebInspectorUI/UserInterface/Views/RangeChart.js:97
> + if (fillStyle) {
I think we should expect there to be fillStyles / strokeStyles and avoid the if
checks. Or perhaps just if check the stroke. It seems crazy to not have a fill
style at least.
More information about the webkit-reviews
mailing list