From 8f243f71285bf73235ebedf75d21dada0ef0f4eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Fri, 15 Nov 2024 16:13:08 +0100 Subject: [PATCH 1/7] Show sample tooltips on sample graph hover --- src/components/shared/thread/SampleGraph.js | 136 ++++++++++++++++---- src/components/timeline/TrackThread.js | 2 + 2 files changed, 113 insertions(+), 25 deletions(-) diff --git a/src/components/shared/thread/SampleGraph.js b/src/components/shared/thread/SampleGraph.js index 9b2525495e..8592ba5d6c 100644 --- a/src/components/shared/thread/SampleGraph.js +++ b/src/components/shared/thread/SampleGraph.js @@ -11,6 +11,11 @@ import { getSampleIndexClosestToCenteredTime } from 'firefox-profiler/profile-lo import { bisectionRight } from 'firefox-profiler/utils/bisect'; import { withSize } from 'firefox-profiler/components/shared/WithSize'; import { BLUE_70, BLUE_40 } from 'photon-colors'; +import { + Tooltip, + MOUSE_OFFSET, +} from 'firefox-profiler/components/tooltip/Tooltip'; +import { SampleTooltipContents } from 'firefox-profiler/components/shared/SampleTooltipContents'; import './SampleGraph.css'; @@ -20,8 +25,17 @@ import type { IndexIntoSamplesTable, Milliseconds, SelectedState, + CssPixels, + TimelineType, + ImplementationFilter, } from 'firefox-profiler/types'; import type { SizeProps } from 'firefox-profiler/components/shared/WithSize'; +import type { CpuRatioInTimeRange } from './ActivityGraphFills'; + +export type HoveredPixelState = {| + +sample: IndexIntoSamplesTable | null, + +cpuRatioInTimeRange: CpuRatioInTimeRange | null, +|}; type Props = {| +className: string, @@ -33,13 +47,21 @@ type Props = {| +categories: CategoryList, +onSampleClick: ( event: SyntheticMouseEvent<>, - sampleIndex: IndexIntoSamplesTable + sampleIndex: IndexIntoSamplesTable | null ) => void, +trackName: string, + +timelineType: TimelineType, + implementationFilter: ImplementationFilter, ...SizeProps, |}; -export class ThreadSampleGraphImpl extends PureComponent { +type State = { + hoveredPixelState: null | HoveredPixelState, + mouseX: CssPixels, + mouseY: CssPixels, +}; + +export class ThreadSampleGraphImpl extends PureComponent { _canvas: null | HTMLCanvasElement = null; _takeCanvasRef = (canvas: HTMLCanvasElement | null) => (this._canvas = canvas); @@ -47,6 +69,11 @@ export class ThreadSampleGraphImpl extends PureComponent { renderScheduled: false, inView: false, }; + state = { + hoveredPixelState: null, + mouseX: 0, + mouseY: 0, + }; _renderCanvas() { if (!this._canvasState.inView) { @@ -185,35 +212,79 @@ export class ThreadSampleGraphImpl extends PureComponent { drawSamples(idleSamples, lighterBlue); } - _onClick = (event: SyntheticMouseEvent<>) => { - const canvas = this._canvas; - if (canvas) { - const { rangeStart, rangeEnd, thread } = this.props; - const r = canvas.getBoundingClientRect(); - - const x = event.pageX - r.left; - const time = rangeStart + (x / r.width) * (rangeEnd - rangeStart); - - const sampleIndex = getSampleIndexClosestToCenteredTime( - thread.samples, - time - ); - - if (thread.samples.stack[sampleIndex] === null) { - // If the sample index refers to a null sample, that sample - // has been filtered out and means that there was no stack bar - // drawn at the place where the user clicked. Do nothing here. - return; - } + _onClick = (event: SyntheticMouseEvent) => { + const hoveredSample = this._getSampleAtMouseEvent(event); + this.props.onSampleClick(event, hoveredSample?.sample ?? null); + }; + + _onMouseLeave = () => { + this.setState({ hoveredPixelState: null }); + }; - this.props.onSampleClick(event, sampleIndex); + _onMouseMove = (event: SyntheticMouseEvent) => { + const canvas = event.currentTarget; + if (!canvas) { + return; } + + const rect = canvas.getBoundingClientRect(); + this.setState({ + hoveredPixelState: this._getSampleAtMouseEvent(event), + mouseX: event.pageX, + // Have the tooltip align to the bottom of the track. + mouseY: rect.bottom - MOUSE_OFFSET, + }); }; + _getSampleAtMouseEvent( + event: SyntheticMouseEvent + ): null | HoveredPixelState { + const canvas = this._canvas; + if (!canvas) { + return null; + } + + const { rangeStart, rangeEnd, thread } = this.props; + const r = canvas.getBoundingClientRect(); + + const x = event.pageX - r.left; + const time = rangeStart + (x / r.width) * (rangeEnd - rangeStart); + + const sampleIndex = getSampleIndexClosestToCenteredTime( + thread.samples, + time + ); + + if (thread.samples.stack[sampleIndex] === null) { + // If the sample index refers to a null sample, that sample + // has been filtered out and means that there was no stack bar + // drawn at the place where the user clicked. Do nothing here. + return null; + } + + return { + sample: sampleIndex, + cpuRatioInTimeRange: null, + }; + } + render() { - const { className, trackName } = this.props; + const { + className, + trackName, + timelineType, + categories, + implementationFilter, + thread, + } = this.props; + const { hoveredPixelState, mouseX, mouseY } = this.state; + return ( -
+
{

Stack Graph for {trackName}

This graph charts the stack height of each sample.

 + {hoveredPixelState === null ? null : ( + + + + )}
); diff --git a/src/components/timeline/TrackThread.js b/src/components/timeline/TrackThread.js index 0ad7c5f392..b483b569ba 100644 --- a/src/components/timeline/TrackThread.js +++ b/src/components/timeline/TrackThread.js @@ -277,6 +277,8 @@ class TimelineTrackThreadImpl extends PureComponent { samplesSelectedStates={samplesSelectedStates} categories={categories} onSampleClick={this._onSampleClick} + timelineType={timelineType} + implementationFilter={implementationFilter} /> ) : null} {isExperimentalCPUGraphsEnabled && From dd27436f88b69859da85ae2182e09110fbc6caed Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Fri, 15 Nov 2024 17:49:49 +0100 Subject: [PATCH 2/7] Only show the tooltip when we are no top of the sample box in sample graph Previously we were always returning the closest sample to the mouse hover/click location. Which was okay when we were clicking to select the boxes before. But now we are also showing tooltips on hover which can be misleading if they were shown when we are not hovering over them. So to make this experience less confusing, this commit changes the behavior to only select or hover a specific sample when the mouse is actually on top of the sample box that we draw. --- src/components/shared/thread/SampleGraph.js | 25 +++++++++- src/profile-logic/profile-data.js | 52 ++++++++++++--------- 2 files changed, 54 insertions(+), 23 deletions(-) diff --git a/src/components/shared/thread/SampleGraph.js b/src/components/shared/thread/SampleGraph.js index 8592ba5d6c..d53e00088a 100644 --- a/src/components/shared/thread/SampleGraph.js +++ b/src/components/shared/thread/SampleGraph.js @@ -244,17 +244,38 @@ export class ThreadSampleGraphImpl extends PureComponent { return null; } - const { rangeStart, rangeEnd, thread } = this.props; + const { rangeStart, rangeEnd, thread, interval } = this.props; const r = canvas.getBoundingClientRect(); const x = event.pageX - r.left; const time = rangeStart + (x / r.width) * (rangeEnd - rangeStart); + const range = [rangeStart, rangeEnd]; + const rangeLength = range[1] - range[0]; + const xPixelsPerMs = canvas.width / rangeLength; + const trueIntervalPixelWidth = interval * xPixelsPerMs; + const multiplier = trueIntervalPixelWidth < 2.0 ? 1.2 : 1.0; + const drawnIntervalWidth = Math.max( + 0.8, + trueIntervalPixelWidth * multiplier + ); + const drawnSampleWidth = Math.min(drawnIntervalWidth, 10) / 2; + + const maxTimeDistance = + (drawnSampleWidth / 2 / r.width) * (rangeEnd - rangeStart); + const sampleIndex = getSampleIndexClosestToCenteredTime( thread.samples, - time + time, + maxTimeDistance ); + if (sampleIndex === null) { + // No sample that is close enough found. Mouse doesn't hover any of the + // sample boxes in the sample graph. + return null; + } + if (thread.samples.stack[sampleIndex] === null) { // If the sample index refers to a null sample, that sample // has been filtered out and means that there was no stack bar diff --git a/src/profile-logic/profile-data.js b/src/profile-logic/profile-data.js index 7b04427eeb..bafe65e868 100644 --- a/src/profile-logic/profile-data.js +++ b/src/profile-logic/profile-data.js @@ -2325,41 +2325,51 @@ export function getSampleIndexClosestToStartTime( * uses the adjusted time. In this context, adjusted time means that `time` array * represent the "center" of the sample, and raw values represent the "start" of * the sample. + * + * Additionally it also checks for a maxTimeDistance threshold. If the time to + * sample distance is higher than that, it just returns null, which indicates + * no sample found. */ export function getSampleIndexClosestToCenteredTime( samples: SamplesTable, - time: number -): IndexIntoSamplesTable { + time: number, + maxTimeDistance: number +): IndexIntoSamplesTable | null { + // Helper function to compute the "center" of a sample + const getCenterTime = (index: number): number => { + if (samples.weight) { + return samples.time[index] + Math.abs(samples.weight[index]) / 2; + } + return samples.time[index]; + }; + // Bisect to find the index of the first sample after the provided time. const index = bisectionRight(samples.time, time); if (index === 0) { - return 0; + // Time is before the first sample + return maxTimeDistance >= Math.abs(getCenterTime(0) - time) ? 0 : null; } - if (index === samples.length) { - return samples.length - 1; + if (index === samples.time.length) { + // Time is after the last sample + const lastIndex = samples.time.length - 1; + return maxTimeDistance >= Math.abs(getCenterTime(lastIndex) - time) + ? lastIndex + : null; } - // Check the distance between the provided time and the center of the bisected sample - // and its predecessor. - const previousIndex = index - 1; - let distanceToThis; - let distanceToLast; - - if (samples.weight) { - const samplesWeight = samples.weight; - const weight = Math.abs(samplesWeight[index]); - const previousWeight = Math.abs(samplesWeight[previousIndex]); + // Calculate distances to the centered time for both the current and previous samples + const distanceToNext = Math.abs(getCenterTime(index) - time); + const distanceToPrevious = Math.abs(getCenterTime(index - 1) - time); - distanceToThis = samples.time[index] + weight / 2 - time; - distanceToLast = time - (samples.time[previousIndex] + previousWeight / 2); - } else { - distanceToThis = samples.time[index] - time; - distanceToLast = time - samples.time[previousIndex]; + if (distanceToNext <= distanceToPrevious) { + // If `distanceToNext` is closer but exceeds `maxTimeDistance`, return null. + return distanceToNext <= maxTimeDistance ? index : null; } - return distanceToThis < distanceToLast ? index : index - 1; + // Otherwise, `distanceToPrevious` is closer. Again check if it exceeds `maxTimeDistance`. + return distanceToPrevious <= maxTimeDistance ? index - 1 : null; } export function getFriendlyThreadName( From 7ec04adfcbe968309e5844cb230bd0b5df1ffe90 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Thu, 9 Jan 2025 15:38:38 +0100 Subject: [PATCH 3/7] Fix the incorrect sample position calculation in the sample graph tests Previously we were using the exact same algorithm as the activity graph. Activity graph tests get the value in between this and the next same time. But for these tests, we actually want the exact sample time. --- src/test/components/SampleGraph.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/components/SampleGraph.test.js b/src/test/components/SampleGraph.test.js index 18bd2adcb4..9e750beac1 100644 --- a/src/test/components/SampleGraph.test.js +++ b/src/test/components/SampleGraph.test.js @@ -49,8 +49,8 @@ const GRAPH_HEIGHT = 10; function getSamplesPixelPosition( sampleIndex: IndexIntoSamplesTable ): CssPixels { - // Compute the pixel position of the center of a given sample. - return sampleIndex * PIXELS_PER_SAMPLE + PIXELS_PER_SAMPLE * 0.5; + // Compute the pixel position of the exact sample. + return sampleIndex * PIXELS_PER_SAMPLE; } function getSamplesProfile() { From ec7941ffa4ef2009fa062c77efea2bd34d84e806 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Thu, 9 Jan 2025 16:15:43 +0100 Subject: [PATCH 4/7] Add some tests for the changed SampleGraph behavior --- src/test/components/SampleGraph.test.js | 88 ++++++++++++++++- .../__snapshots__/SampleGraph.test.js.snap | 97 +++++++++++++++++++ 2 files changed, 184 insertions(+), 1 deletion(-) diff --git a/src/test/components/SampleGraph.test.js b/src/test/components/SampleGraph.test.js index 9e750beac1..59e95d020e 100644 --- a/src/test/components/SampleGraph.test.js +++ b/src/test/components/SampleGraph.test.js @@ -8,6 +8,7 @@ import * as React from 'react'; import { Provider } from 'react-redux'; import { render } from 'firefox-profiler/test/fixtures/testing-library'; +import { fireEvent } from '@testing-library/react'; import { selectedThreadSelectors } from 'firefox-profiler/selectors/per-thread'; import { ensureExists } from 'firefox-profiler/utils/flow'; import { TimelineTrackThread } from 'firefox-profiler/components/timeline/TrackThread'; @@ -17,7 +18,12 @@ import { } from '../fixtures/mocks/canvas-context'; import { mockRaf } from '../fixtures/mocks/request-animation-frame'; import { storeWithProfile } from '../fixtures/stores'; -import { fireFullClick } from '../fixtures/utils'; +import { + fireFullClick, + getMouseEvent, + addRootOverlayElement, + removeRootOverlayElement, +} from '../fixtures/utils'; import { getProfileFromTextSamples } from '../fixtures/profiles/processed-profile'; import { autoMockElementSize, @@ -67,6 +73,8 @@ describe('SampleGraph', function () { autoMockCanvasContext(); autoMockElementSize({ width: GRAPH_WIDTH, height: GRAPH_HEIGHT }); autoMockIntersectionObserver(); + beforeEach(addRootOverlayElement); + afterEach(removeRootOverlayElement); function setup(profile: Profile = getSamplesProfile()) { const store = storeWithProfile(profile); @@ -100,6 +108,17 @@ describe('SampleGraph', function () { }); } + // Hover over the sample graph. + function hoverSampleGraph(index: IndexIntoSamplesTable) { + fireEvent( + sampleGraphCanvas, + getMouseEvent('mousemove', { + pageX: getSamplesPixelPosition(index), + pageY: GRAPH_HEIGHT / 2, + }) + ); + } + // This function gets the selected call node path as a list of function names. function getCallNodePath() { return selectedThreadSelectors @@ -126,6 +145,7 @@ describe('SampleGraph', function () { store, sampleGraphCanvas, clickSampleGraph, + hoverSampleGraph, getCallNodePath, getContextDrawCalls, }; @@ -159,6 +179,72 @@ describe('SampleGraph', function () { clickSampleGraph(2); expect(getCallNodePath()).toEqual(['A', 'B', 'H', 'I']); }); + + it('clicking outside of any sample removes any selection', function () { + const { clickSampleGraph, getCallNodePath, sampleGraphCanvas } = setup(); + + // Starting while nothing is selected. + expect(getCallNodePath()).toEqual([]); + + // Selecting the sample with index 1. + // The full call node at this sample is: + // A -> B -> C -> F -> G + clickSampleGraph(1); + expect(getCallNodePath()).toEqual(['A', 'B', 'C', 'F', 'G']); + + // Now we are selecting outside of the sample, which should remove the selection. + fireFullClick(sampleGraphCanvas, { + pageX: getSamplesPixelPosition(1) + PIXELS_PER_SAMPLE / 2, + pageY: GRAPH_HEIGHT / 2, + }); + expect(getCallNodePath()).toEqual([]); + }); + + it('shows the correct tooltip when hovered', function () { + const { hoverSampleGraph, getCallNodePath } = setup(); + + // Hovering the sample with index 1. + // The full call node at this sample is: + // A -> B -> C -> F -> G + hoverSampleGraph(1); + + // We didn't click, so selection should not change in the selected node path. + expect(getCallNodePath()).toEqual([]); + + // Make sure that we have a tooltip. + expect( + ensureExists( + document.querySelector('.tooltip'), + 'A tooltip component must exist for this test.' + ) + ).toMatchSnapshot(); + }); + + it('does not show a tooltip when outside of a sample is hovered', function () { + const { hoverSampleGraph, getCallNodePath, sampleGraphCanvas } = setup(); + + // The full call node at this sample is: + // A -> B -> C -> F -> G + + hoverSampleGraph(1); + // We didn't click, so selection should not change. + expect(getCallNodePath()).toEqual([]); + + // Make sure that we have a tooltip. + expect(document.querySelector('.tooltip')).toBeTruthy(); + + // Now we are hovering outside of the samples. + fireEvent( + sampleGraphCanvas, + getMouseEvent('mousemove', { + pageX: getSamplesPixelPosition(1) + PIXELS_PER_SAMPLE / 2, + pageY: GRAPH_HEIGHT / 2, + }) + ); + + // There should be no tooltip this time + expect(document.querySelector('.tooltip')).toBeFalsy(); + }); }); }); diff --git a/src/test/components/__snapshots__/SampleGraph.test.js.snap b/src/test/components/__snapshots__/SampleGraph.test.js.snap index 847abd8b71..898494be51 100644 --- a/src/test/components/__snapshots__/SampleGraph.test.js.snap +++ b/src/test/components/__snapshots__/SampleGraph.test.js.snap @@ -1,5 +1,102 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP +exports[`SampleGraph ThreadSampleGraph shows the correct tooltip when hovered 1`] = ` +
+
+
+ Category: +
+
+ + Graphics +
+
+
+
+ Stack: +
+
+
    +
  • + + G + +
    • +
  • + + F + +
    • +
  • + + C + +
    • +
  • + + B + +
    • +
  • + + A + +
    • +
+
+`; + exports[`SampleGraph matches the 2d canvas draw snapshot 1`] = ` Array [ Array [ From 4472715516c5b1a7819a036405d41a57046c54ac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Tue, 14 Jan 2025 14:06:31 +0100 Subject: [PATCH 5/7] Fix some small nits --- src/components/shared/thread/SampleGraph.js | 19 ++++++++----------- src/test/components/SampleGraph.test.js | 4 ++++ 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/src/components/shared/thread/SampleGraph.js b/src/components/shared/thread/SampleGraph.js index d53e00088a..74d3bf13ec 100644 --- a/src/components/shared/thread/SampleGraph.js +++ b/src/components/shared/thread/SampleGraph.js @@ -51,7 +51,7 @@ type Props = {| ) => void, +trackName: string, +timelineType: TimelineType, - implementationFilter: ImplementationFilter, + +implementationFilter: ImplementationFilter, ...SizeProps, |}; @@ -130,8 +130,7 @@ export class ThreadSampleGraphImpl extends PureComponent { canvas.width = Math.round(width * devicePixelRatio); canvas.height = Math.round(height * devicePixelRatio); const ctx = canvas.getContext('2d'); - const range = [rangeStart, rangeEnd]; - const rangeLength = range[1] - range[0]; + const rangeLength = rangeEnd - rangeStart; const xPixelsPerMs = canvas.width / rangeLength; const trueIntervalPixelWidth = interval * xPixelsPerMs; const multiplier = trueIntervalPixelWidth < 2.0 ? 1.2 : 1.0; @@ -141,8 +140,8 @@ export class ThreadSampleGraphImpl extends PureComponent { ); const drawnSampleWidth = Math.min(drawnIntervalWidth, 10); - const firstDrawnSampleTime = range[0] - drawnIntervalWidth / xPixelsPerMs; - const lastDrawnSampleTime = range[1]; + const firstDrawnSampleTime = rangeStart - drawnIntervalWidth / xPixelsPerMs; + const lastDrawnSampleTime = rangeEnd; const firstDrawnSampleIndex = bisectionRight( thread.samples.time, @@ -172,7 +171,7 @@ export class ThreadSampleGraphImpl extends PureComponent { continue; } const xPos = - (sampleTime - range[0]) * xPixelsPerMs - drawnSampleWidth / 2; + (sampleTime - rangeStart) * xPixelsPerMs - drawnSampleWidth / 2; let samplesBucket; if ( samplesSelectedStates !== null && @@ -247,11 +246,10 @@ export class ThreadSampleGraphImpl extends PureComponent { const { rangeStart, rangeEnd, thread, interval } = this.props; const r = canvas.getBoundingClientRect(); - const x = event.pageX - r.left; + const x = event.nativeEvent.offsetX; const time = rangeStart + (x / r.width) * (rangeEnd - rangeStart); - const range = [rangeStart, rangeEnd]; - const rangeLength = range[1] - range[0]; + const rangeLength = rangeEnd - rangeStart; const xPixelsPerMs = canvas.width / rangeLength; const trueIntervalPixelWidth = interval * xPixelsPerMs; const multiplier = trueIntervalPixelWidth < 2.0 ? 1.2 : 1.0; @@ -261,8 +259,7 @@ export class ThreadSampleGraphImpl extends PureComponent { ); const drawnSampleWidth = Math.min(drawnIntervalWidth, 10) / 2; - const maxTimeDistance = - (drawnSampleWidth / 2 / r.width) * (rangeEnd - rangeStart); + const maxTimeDistance = (drawnSampleWidth / 2 / r.width) * rangeLength; const sampleIndex = getSampleIndexClosestToCenteredTime( thread.samples, diff --git a/src/test/components/SampleGraph.test.js b/src/test/components/SampleGraph.test.js index 59e95d020e..8b0af40df0 100644 --- a/src/test/components/SampleGraph.test.js +++ b/src/test/components/SampleGraph.test.js @@ -104,6 +104,7 @@ describe('SampleGraph', function () { function clickSampleGraph(index: IndexIntoSamplesTable) { fireFullClick(sampleGraphCanvas, { pageX: getSamplesPixelPosition(index), + offsetX: getSamplesPixelPosition(index), pageY: GRAPH_HEIGHT / 2, }); } @@ -114,6 +115,7 @@ describe('SampleGraph', function () { sampleGraphCanvas, getMouseEvent('mousemove', { pageX: getSamplesPixelPosition(index), + offsetX: getSamplesPixelPosition(index), pageY: GRAPH_HEIGHT / 2, }) ); @@ -195,6 +197,7 @@ describe('SampleGraph', function () { // Now we are selecting outside of the sample, which should remove the selection. fireFullClick(sampleGraphCanvas, { pageX: getSamplesPixelPosition(1) + PIXELS_PER_SAMPLE / 2, + offsetX: getSamplesPixelPosition(1) + PIXELS_PER_SAMPLE / 2, pageY: GRAPH_HEIGHT / 2, }); expect(getCallNodePath()).toEqual([]); @@ -238,6 +241,7 @@ describe('SampleGraph', function () { sampleGraphCanvas, getMouseEvent('mousemove', { pageX: getSamplesPixelPosition(1) + PIXELS_PER_SAMPLE / 2, + offsetX: getSamplesPixelPosition(1) + PIXELS_PER_SAMPLE / 2, pageY: GRAPH_HEIGHT / 2, }) ); From 34909ce70f84313c531059e24a9e12ade63cda6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Tue, 14 Jan 2025 14:43:57 +0100 Subject: [PATCH 6/7] Split the sample graph canvas code into a new component --- src/components/shared/thread/SampleGraph.js | 118 ++++++++++++++------ 1 file changed, 82 insertions(+), 36 deletions(-) diff --git a/src/components/shared/thread/SampleGraph.js b/src/components/shared/thread/SampleGraph.js index 74d3bf13ec..8da87b6277 100644 --- a/src/components/shared/thread/SampleGraph.js +++ b/src/components/shared/thread/SampleGraph.js @@ -61,7 +61,24 @@ type State = { mouseY: CssPixels, }; -export class ThreadSampleGraphImpl extends PureComponent { +type CanvasProps = {| + +className: string, + +thread: Thread, + +samplesSelectedStates: null | SelectedState[], + +interval: Milliseconds, + +rangeStart: Milliseconds, + +rangeEnd: Milliseconds, + +categories: CategoryList, + +trackName: string, + ...SizeProps, +|}; + +/** + * This component controls the rendering of the canvas. Every render call through + * React triggers a new canvas render. Because of this, it's important to only pass + * in the props that are needed for the canvas draw call. + */ +class ThreadSampleGraphCanvas extends React.PureComponent { _canvas: null | HTMLCanvasElement = null; _takeCanvasRef = (canvas: HTMLCanvasElement | null) => (this._canvas = canvas); @@ -69,11 +86,6 @@ export class ThreadSampleGraphImpl extends PureComponent { renderScheduled: false, inView: false, }; - state = { - hoveredPixelState: null, - mouseX: 0, - mouseY: 0, - }; _renderCanvas() { if (!this._canvasState.inView) { @@ -211,6 +223,33 @@ export class ThreadSampleGraphImpl extends PureComponent { drawSamples(idleSamples, lighterBlue); } + render() { + const { trackName } = this.props; + + return ( + + +

Stack Graph for {trackName}

+

This graph charts the stack height of each sample.

+ + + ); + } +} + +export class ThreadSampleGraphImpl extends PureComponent { + state = { + hoveredPixelState: null, + mouseX: 0, + mouseY: 0, + }; + _onClick = (event: SyntheticMouseEvent) => { const hoveredSample = this._getSampleAtMouseEvent(event); this.props.onSampleClick(event, hoveredSample?.sample ?? null); @@ -238,7 +277,7 @@ export class ThreadSampleGraphImpl extends PureComponent { _getSampleAtMouseEvent( event: SyntheticMouseEvent ): null | HoveredPixelState { - const canvas = this._canvas; + const canvas = event.currentTarget; if (!canvas) { return null; } @@ -250,7 +289,7 @@ export class ThreadSampleGraphImpl extends PureComponent { const time = rangeStart + (x / r.width) * (rangeEnd - rangeStart); const rangeLength = rangeEnd - rangeStart; - const xPixelsPerMs = canvas.width / rangeLength; + const xPixelsPerMs = r.width / rangeLength; const trueIntervalPixelWidth = interval * xPixelsPerMs; const multiplier = trueIntervalPixelWidth < 2.0 ? 1.2 : 1.0; const drawnIntervalWidth = Math.max( @@ -294,6 +333,12 @@ export class ThreadSampleGraphImpl extends PureComponent { categories, implementationFilter, thread, + interval, + rangeStart, + rangeEnd, + samplesSelectedStates, + width, + height, } = this.props; const { hoveredPixelState, mouseX, mouseY } = this.state; @@ -302,35 +347,36 @@ export class ThreadSampleGraphImpl extends PureComponent { className={className} onMouseMove={this._onMouseMove} onMouseLeave={this._onMouseLeave} + onClick={this._onClick} > - - -

Stack Graph for {trackName}

-

This graph charts the stack height of each sample.

- - {hoveredPixelState === null ? null : ( - - - - )} - + + + {hoveredPixelState === null ? null : ( + + + + )}
); } From ede10057155065eeb2ef7898b983dc5c13982e45 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Naz=C4=B1m=20Can=20Alt=C4=B1nova?= Date: Wed, 15 Jan 2025 13:54:49 +0100 Subject: [PATCH 7/7] Add a comment explaining the copies of some variables in the sample graph --- src/components/shared/thread/SampleGraph.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/components/shared/thread/SampleGraph.js b/src/components/shared/thread/SampleGraph.js index 8da87b6277..f859f60d7b 100644 --- a/src/components/shared/thread/SampleGraph.js +++ b/src/components/shared/thread/SampleGraph.js @@ -288,6 +288,10 @@ export class ThreadSampleGraphImpl extends PureComponent { const x = event.nativeEvent.offsetX; const time = rangeStart + (x / r.width) * (rangeEnd - rangeStart); + // These values are copied from the `drawCanvas` method to compute the + // `drawnSampleWidth` instead of extracting into a new function. Extracting + // into a new function is not really idea for performance reasons since we + // need these values for other values in `drawCanvas`. const rangeLength = rangeEnd - rangeStart; const xPixelsPerMs = r.width / rangeLength; const trueIntervalPixelWidth = interval * xPixelsPerMs;