Bug 195296 - Web Inspector: Timelines: convert WI.RangeChart to use a <canvas>
Summary: Web Inspector: Timelines: convert WI.RangeChart to use a <canvas>
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-03-04 15:15 PST by Devin Rousso
Modified: 2022-03-28 09:15 PDT (History)
4 users (show)

See Also:

Attachments
Patch (10.00 KB, patch)
2019-03-04 15:40 PST, Devin Rousso 		no flags Details | Formatted Diff | Diff
Patch (10.56 KB, patch)
2019-03-04 17:54 PST, Devin Rousso 		no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews123 for ios-simulator-wk2 (3.68 MB, application/zip)
2019-03-04 23:51 PST, EWS Watchlist 		no flags Details
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-03-04 15:15:43 PST 
Creating/drawing all those <rect>s is very expensive :(
Comment 1 Devin Rousso 2019-03-04 15:40:02 PST 
Created attachment 363559 [details]
Patch
Comment 2 Joseph Pecoraro 2019-03-04 15:49:20 PST 
Comment on attachment 363559 [details]
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.
Comment 3 Devin Rousso 2019-03-04 17:54:49 PST 
Created attachment 363576 [details]
Patch
Comment 4 Joseph Pecoraro 2019-03-04 19:04:43 PST 
So this produced some drawing issues on macOS for very small strokes. We are considering not doing it, or tweaking it.
Comment 5 EWS Watchlist 2019-03-04 23:51:09 PST Comment hidden (obsolete)
Comment 6 EWS Watchlist 2019-03-04 23:51:11 PST Comment hidden (obsolete)