Implement IntervalMarkerOverview using canvas. #168
Conversation
Codecov Report
@@ Coverage Diff @@
## master #168 +/- ##
=======================================
Coverage 48.07% 48.07%
=======================================
Files 34 34
Lines 1953 1953
=======================================
Hits 939 939
Misses 1014 1014Continue to review full report at Codecov.
|
mstange
force-pushed
the
canvas-tracing-markers
branch
from
d0d11a1
to
e951b81
Compare
| import { withSize } from '../with-size'; | ||
|
|
||
| type MarkerState = 'PRESSED' | 'HOVERED' | 'NONE'; |
There was a problem hiding this comment.
At some point I'll stop gushing about flow, but I love this.
Also, flow won't be checked unless you have.// @flow at the top of flow files.
There was a problem hiding this comment.
Oh, thanks for the note about // @flow.
|
|
||
| // Walk the list of intervalMarkers backwards to find the marker under the | ||
| // mouse. Later markers are drawn on top and we want to find the topmost one. | ||
| for (let i = intervalMarkers.length - 1; i >= 0; i--) { |
There was a problem hiding this comment.
The comment tries to explain it: "Later markers are drawn on top and we want to find the topmost one." The word "later" is probably not the right one here - maybe "markers at a higher array index"? "subsequent"?
There was a problem hiding this comment.
How about: "Markers are drawn in array order; the one drawn last is on top. So if there are multiple markers under the mouse, we want to find the one with the highest array index. That's why we walk the list of intervalMarkers backwards."
| } | ||
|
|
||
| _hitTest(e) { | ||
| const r = this._canvas.getBoundingClientRect(); |
There was a problem hiding this comment.
Thanks, will steal this for the timeline.
| this.props.onSelect(this.props.threadIndex, +e.target.getAttribute('data-start'), +e.target.getAttribute('data-end')); | ||
| _onMouseMove(e) { | ||
| const hoveredItem = this._hitTest(e); | ||
| this.setState({ hoveredItem }); |
There was a problem hiding this comment.
I think we can save some object allocations and component update checks here.
if (this.state.hoveredItem !== hoveredItem) {
this.setState({ hoveredItem });
}
There was a problem hiding this comment.
yeah ok, shouldn't matter much but I'll do it.
| const mouseDownItem = this._hitTest(e); | ||
| this.setState({ mouseDownItem }); | ||
| if (mouseDownItem !== null) { | ||
| if (e.target.setCapture) { |
There was a problem hiding this comment.
Whoa, I had no idea setCapture existed. Nice! Is it not well supported, hence the if?
There was a problem hiding this comment.
Correct. Chrome doesn't support it :(
| ctx = c.getContext('2d'); | ||
| ctx.clearRect(0, 0, pixelWidth, pixelHeight); | ||
| } | ||
| ctx.scale(devicePixelRatio, devicePixelRatio); |
There was a problem hiding this comment.
I believe setting the scale every draw can have some performance impacts on Gecko, at least it did when I was measuring it on the flame chart.
| @@ -0,0 +1,105 @@ | |||
| /* stolen from the light theme at devtools/client/themes/variables.css */ | |||
There was a problem hiding this comment.
👍 I like this broken out like this.
We were using DOM elements before which wasn't good for performance.
mstange
force-pushed
the
canvas-tracing-markers
branch
from
e951b81
to
09c3019
Compare
We were using DOM elements before which wasn't good for performance.