diff --git a/src/components/timeline/TrackVisualProgressGraph.js b/src/components/timeline/TrackVisualProgressGraph.js index decbb0422c..41f6cde3d0 100644 --- a/src/components/timeline/TrackVisualProgressGraph.js +++ b/src/components/timeline/TrackVisualProgressGraph.js @@ -102,7 +102,7 @@ class TrackVisualProgressCanvas extends React.PureComponent { // Create a path for the top of the chart. This is the line that will have // a stroke applied to it. x = - (deviceWidth * (progressGraphData[i].timestamp - rangeStart)) / + (deviceWidth * ((progressGraphData[i].timestamp ?? 0) - rangeStart)) / rangeLength; // Add on half the stroke's line width so that it won't be cut off the edge // of the graph. @@ -133,7 +133,7 @@ class TrackVisualProgressCanvas extends React.PureComponent { // of the canvas. ctx.lineTo(x + interval, deviceHeight); ctx.lineTo( - (deviceWidth * (progressGraphData[0].timestamp - rangeStart)) / + (deviceWidth * ((progressGraphData[0].timestamp ?? 0) - rangeStart)) / rangeLength + interval, deviceHeight @@ -221,15 +221,16 @@ class TrackVisualProgressGraphImpl extends React.PureComponent { const rangeLength = rangeEnd - rangeStart; const timeAtMouse = rangeStart + ((mouseX - left) / width) * rangeLength; if ( - timeAtMouse < progressGraphData[0].timestamp || + timeAtMouse < (progressGraphData[0].timestamp ?? 0) || timeAtMouse > - progressGraphData[progressGraphData.length - 1].timestamp + interval + (progressGraphData[progressGraphData.length - 1].timestamp ?? 0) + + interval ) { // We are outside the range of the samples, do not display hover information. this.setState({ hoveredVisualProgress: null }); } else { const graphTimestamps = progressGraphData.map( - ({ timestamp }) => timestamp + ({ timestamp }) => timestamp ?? 0 ); let hoveredVisualProgress = bisectionRight(graphTimestamps, timeAtMouse); if (hoveredVisualProgress === progressGraphData.length) { @@ -277,7 +278,8 @@ class TrackVisualProgressGraphImpl extends React.PureComponent { } = this.props; const rangeLength = rangeEnd - rangeStart; const left = - (width * (progressGraphData[graphDataIndex].timestamp - rangeStart)) / + (width * + ((progressGraphData[graphDataIndex].timestamp ?? 0) - rangeStart)) / rangeLength; const unitSampleCount = progressGraphData[graphDataIndex].percent / 100; diff --git a/src/profile-logic/process-profile.js b/src/profile-logic/process-profile.js index 14541240fa..238deb726f 100644 --- a/src/profile-logic/process-profile.js +++ b/src/profile-logic/process-profile.js @@ -28,6 +28,7 @@ import { isArtTraceFormat, convertArtTraceProfile } from './import/art-trace'; import { PROCESSED_PROFILE_VERSION, INTERVAL, + INTERVAL_END, INSTANT, } from '../app-logic/constants'; import { @@ -87,6 +88,7 @@ import type { PageList, ThreadIndex, BrowsertimeMarkerPayload, + MarkerPhase, } from 'firefox-profiler/types'; type RegExpResult = null | string[]; @@ -1736,18 +1738,30 @@ export function processVisualMetrics( (cat) => cat.name === 'Test' ); - function addMetricMarker( + function maybeAddMetricMarker( thread: Thread, name: string, - startTime: number, - endTime?: number, + phase: MarkerPhase, + startTime: number | null, + endTime: number | null, payload?: BrowsertimeMarkerPayload ) { + if ( + // All phases except INTERVAL_END should have a start time. + (phase !== INTERVAL_END && startTime === null) || + // Only INTERVAL and INTERVAL_END should have an end time. + ((phase === INTERVAL || phase === INTERVAL_END) && endTime === null) + ) { + // Do not add if some timestamps we expect are missing. + // This should ideally never happen but timestamps could be null due to + // browsertime bug here: https://github.com/sitespeedio/browsertime/issues/1746. + return; + } // Add the marker to the given thread. thread.markers.name.push(thread.stringTable.indexForString(name)); thread.markers.startTime.push(startTime); - thread.markers.endTime.push(endTime ? endTime : 0); - thread.markers.phase.push(endTime ? INTERVAL : INSTANT); + thread.markers.endTime.push(endTime); + thread.markers.phase.push(phase); thread.markers.category.push(testingCategoryIdx); thread.markers.data.push(payload ?? null); thread.markers.length++; @@ -1783,9 +1797,9 @@ export function processVisualMetrics( // Add the progress marker to the parent process main thread. const markerName = `${metricName} Progress`; - addMetricMarker(mainThread, markerName, startTime, endTime); + maybeAddMetricMarker(mainThread, markerName, INTERVAL, startTime, endTime); // Add the progress marker to the tab process main thread. - addMetricMarker(tabThread, markerName, startTime, endTime); + maybeAddMetricMarker(tabThread, markerName, INTERVAL, startTime, endTime); // Add progress markers for every visual progress change for more fine grained information. const progressMarkerSchema = { @@ -1805,19 +1819,21 @@ export function processVisualMetrics( }; // Add it to the parent process main thread. - addMetricMarker( + maybeAddMetricMarker( mainThread, changeMarkerName, + INSTANT, timestamp, - undefined, // endTime + null, // endTime payload ); // Add it to the tab process main thread. - addMetricMarker( + maybeAddMetricMarker( tabThread, changeMarkerName, + INSTANT, timestamp, - undefined, // endTime + null, // endTime payload ); } diff --git a/src/test/unit/process-profile.test.js b/src/test/unit/process-profile.test.js index 1ffe854ded..d95b7169cc 100644 --- a/src/test/unit/process-profile.test.js +++ b/src/test/unit/process-profile.test.js @@ -664,14 +664,15 @@ describe('profile meta processing', function () { }); describe('visualMetrics processing', function () { - function checkVisualMetricsForThread(thread: Thread) { - const metrics = [ - { name: 'Visual', changeMarkerLength: 7 }, - { name: 'ContentfulSpeedIndex', changeMarkerLength: 6 }, - { name: 'PerceptualSpeedIndex', changeMarkerLength: 6 }, - ]; - - for (const { name, changeMarkerLength } of metrics) { + function checkVisualMetricsForThread( + thread: Thread, + metrics: Array<{| + name: string, + hasProgressMarker: boolean, + changeMarkerLength: number, + |}> + ) { + for (const { name, hasProgressMarker, changeMarkerLength } of metrics) { // Check the visual metric progress markers. const metricProgressMarkerIndex = thread.stringTable.indexForString( `${name} Progress` @@ -679,7 +680,12 @@ describe('visualMetrics processing', function () { const metricProgressMarker = thread.markers.name.find( (name) => name === metricProgressMarkerIndex ); - expect(metricProgressMarker).toBeTruthy(); + + if (hasProgressMarker) { + expect(metricProgressMarker).toBeTruthy(); + } else { + expect(metricProgressMarker).toBeFalsy(); + } // Check the visual metric change markers. const metricChangeMarkerIndex = thread.stringTable.indexForString( @@ -712,7 +718,19 @@ describe('visualMetrics processing', function () { throw new Error('Could not find the parent process main thread.'); } - checkVisualMetricsForThread(parentProcessMainThread); + checkVisualMetricsForThread(parentProcessMainThread, [ + { name: 'Visual', hasProgressMarker: true, changeMarkerLength: 7 }, + { + name: 'ContentfulSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + { + name: 'PerceptualSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + ]); }); it('adds markers to the tab process', function () { @@ -734,6 +752,67 @@ describe('visualMetrics processing', function () { throw new Error('Could not find the tab process main thread.'); } - checkVisualMetricsForThread(tabProcessMainThread); + checkVisualMetricsForThread(tabProcessMainThread, [ + { name: 'Visual', hasProgressMarker: true, changeMarkerLength: 7 }, + { + name: 'ContentfulSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + { + name: 'PerceptualSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + ]); + }); + + it('does not add markers to the parent process if metric timestamps are null', function () { + const geckoProfile = createGeckoProfile(); + const visualMetrics = getVisualMetrics(); + + // Make all the VisualProgress timestamps null on purpose. + visualMetrics.VisualProgress = visualMetrics.VisualProgress.map( + (progress) => ({ ...progress, timestamp: null }) + ); + // Make only one ContentfulSpeedIndexProgress timestamp null on purpose. + ensureExists(visualMetrics.ContentfulSpeedIndexProgress)[0].timestamp = + null; + + // Add the visual metrics to the profile. + geckoProfile.meta.visualMetrics = visualMetrics; + // Make sure that the visual metrics are not changed. + expect(visualMetrics.VisualProgress).toHaveLength(7); + expect(visualMetrics.ContentfulSpeedIndexProgress).toHaveLength(6); + expect(visualMetrics.PerceptualSpeedIndexProgress).toHaveLength(6); + + // Processing the profile. + const processedProfile = processGeckoProfile(geckoProfile); + const parentProcessMainThread = processedProfile.threads.find( + (thread) => + thread.name === 'GeckoMain' && thread.processType === 'default' + ); + + if (!parentProcessMainThread) { + throw new Error('Could not find the parent process main thread.'); + } + + checkVisualMetricsForThread(parentProcessMainThread, [ + // Instead of 7, we should have 0 markers for Visual because we made all + // the timestamps null. + { name: 'Visual', hasProgressMarker: false, changeMarkerLength: 0 }, + // Instead of 6, we should have 5 markers for ContentfulSpeedIndex. + { + name: 'ContentfulSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 5, + }, + // We didn't change the PerceptualSpeedIndexProgress, so we should have 6. + { + name: 'PerceptualSpeedIndex', + hasProgressMarker: true, + changeMarkerLength: 6, + }, + ]); }); }); diff --git a/src/types/profile.js b/src/types/profile.js index 4638e1acc0..e57a2d92be 100644 --- a/src/types/profile.js +++ b/src/types/profile.js @@ -681,7 +681,8 @@ export type ProgressGraphData = {| // A percentage that describes the visual completeness of the webpage, ranging from 0% - 100% percent: number, // The time in milliseconds which the sample was taken. - timestamp: Milliseconds, + // This can be null due to https://github.com/sitespeedio/browsertime/issues/1746. + timestamp: Milliseconds | null, |}; /**