REGRESSION(r212853): Comparisons to baseline no longer shows up
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Feb 2017 08:02:02 +0000 (08:02 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 25 Feb 2017 08:02:02 +0000 (08:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=168863

Reviewed by Joseph Pecoraro.

The bug was caused by ChartStatusView's code not being updated to use TimeSeriesView's.
Updated the code to use TimeSeriesView's methods to fix the bug.

Also made InteractiveTimeSeriesChart's currentPoint to return a (TimeSeriesView, point, isLocked) tuple
to consolidate it with lockedIndicator() to work towards making the baseline data points selectable.

* browser-tests/time-series-chart-tests.js: Updated the test cases to use currentIndicator, and added
test cases for newly added lastPointInTimeRange.

* public/v3/components/chart-pane.js:
(ChartPane.prototype.serializeState): Updated to use currentIndicator.
(ChartPane.prototype._renderFilteringPopover): Ditto.

* public/v3/components/chart-status-view.js:
(ChartStatusView.prototype.updateStatusIfNeeded): Use currentIndicator for an interative time series.
Fixed the non-interactive chart's code path for TimeSeriesView.
(ChartStatusView.prototype._computeChartStatus): Modernized the code.
(ChartStatusView.prototype._findLastPointPriorToTime): Deleted. Replaced by TimeSeriesView's
lastPointInTimeRange.

* public/v3/components/interactive-time-series-chart.js:
(InteractiveTimeSeriesChart.prototype.currentIndicator):
(InteractiveTimeSeriesChart.prototype.moveLockedIndicatorWithNotification):
(InteractiveTimeSeriesChart.prototype._renderChartContent):
(InteractiveTimeSeriesChart):

* public/v3/models/time-series.js:
(TimeSeriesView.prototype.lastPointInTimeRange): Added.
(TimeSeriesView.prototype._reverse): Added. Traverses the view in the reverse order.
* unit-tests/time-series-tests.js:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@212997 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/browser-tests/time-series-chart-tests.js
Websites/perf.webkit.org/public/v3/components/chart-status-view.js
Websites/perf.webkit.org/public/v3/components/interactive-time-series-chart.js
Websites/perf.webkit.org/public/v3/models/time-series.js
Websites/perf.webkit.org/public/v3/pages/chart-pane.js
Websites/perf.webkit.org/unit-tests/time-series-tests.js

index 204b86f..0075379 100644 (file)
@@ -1,3 +1,41 @@
+2017-02-24  Ryosuke Niwa  <rniwa@webkit.org>
+
+        REGRESSION(r212853): Comparisons to baseline no longer shows up
+        https://bugs.webkit.org/show_bug.cgi?id=168863
+
+        Reviewed by Joseph Pecoraro.
+
+        The bug was caused by ChartStatusView's code not being updated to use TimeSeriesView's.
+        Updated the code to use TimeSeriesView's methods to fix the bug.
+
+        Also made InteractiveTimeSeriesChart's currentPoint to return a (TimeSeriesView, point, isLocked) tuple
+        to consolidate it with lockedIndicator() to work towards making the baseline data points selectable.
+
+        * browser-tests/time-series-chart-tests.js: Updated the test cases to use currentIndicator, and added
+        test cases for newly added lastPointInTimeRange.
+
+        * public/v3/components/chart-pane.js:
+        (ChartPane.prototype.serializeState): Updated to use currentIndicator.
+        (ChartPane.prototype._renderFilteringPopover): Ditto.
+
+        * public/v3/components/chart-status-view.js:
+        (ChartStatusView.prototype.updateStatusIfNeeded): Use currentIndicator for an interative time series.
+        Fixed the non-interactive chart's code path for TimeSeriesView.
+        (ChartStatusView.prototype._computeChartStatus): Modernized the code.
+        (ChartStatusView.prototype._findLastPointPriorToTime): Deleted. Replaced by TimeSeriesView's
+        lastPointInTimeRange.
+
+        * public/v3/components/interactive-time-series-chart.js:
+        (InteractiveTimeSeriesChart.prototype.currentIndicator):
+        (InteractiveTimeSeriesChart.prototype.moveLockedIndicatorWithNotification):
+        (InteractiveTimeSeriesChart.prototype._renderChartContent):
+        (InteractiveTimeSeriesChart):
+
+        * public/v3/models/time-series.js:
+        (TimeSeriesView.prototype.lastPointInTimeRange): Added.
+        (TimeSeriesView.prototype._reverse): Added. Traverses the view in the reverse order.
+        * unit-tests/time-series-tests.js:
+
 2017-02-23  Dewei Zhu  <dewei_zhu@apple.com>
 
         Rename 'commit_parent' in 'commits' table to 'commit_previous_commit'.
index a36ed4d..eb4f13e 100644 (file)
@@ -1,3 +1,5 @@
+(() => {
+
 const scripts = [
     '../shared/statistics.js',
     'instrumentation.js',
@@ -923,7 +925,7 @@ describe('TimeSeriesChart', () => {
 
 describe('InteractiveTimeSeriesChart', () => {
 
-    it('should change the indicator to the point closest to the last mouse move position', () => {
+    it('should change the unlocked indicator to the point closest to the last mouse move position', () => {
         const context = new BrowsingContext();
         return context.importScripts(scripts, 'ComponentBase', 'TimeSeriesChart', 'InteractiveTimeSeriesChart', 'MeasurementSet', 'MockRemoteAPI').then(() => {
             const chart = createChartWithSampleCluster(context, {}, {interactiveChart: true, interactive: true});
@@ -941,8 +943,7 @@ describe('InteractiveTimeSeriesChart', () => {
             let canvas;
             return waitForComponentsToRender(context).then(() => {
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(indicatorChangeCalls).to.be.eql([]);
 
                 canvas = chart.content().querySelector('canvas');
@@ -955,10 +956,13 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.not.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
-                const lastPoint = chart.sampledTimeSeriesData('current').lastPoint();
-                expect(chart.currentPoint()).to.be(lastPoint);
+                const indicator = chart.currentIndicator();
+                expect(indicator).to.not.be(null);
+                const currentView = chart.sampledTimeSeriesData('current');
+                const lastPoint = currentView.lastPoint();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(lastPoint);
+                expect(indicator.isLocked).to.be(false);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, false]]);
 
                 expect(selectionChangeCount).to.be(0);
@@ -984,8 +988,7 @@ describe('InteractiveTimeSeriesChart', () => {
             let canvas;
             return waitForComponentsToRender(context).then(() => {
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(indicatorChangeCalls).to.be.eql([]);
                 canvas = chart.content().querySelector('canvas');
                 const rect = canvas.getBoundingClientRect();
@@ -1003,10 +1006,13 @@ describe('InteractiveTimeSeriesChart', () => {
             }).then(() => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
-                const lastPoint = chart.sampledTimeSeriesData('current').lastPoint();
+                const currentView = chart.sampledTimeSeriesData('current');
+                const lastPoint = currentView.lastPoint();
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(lastPoint);
-                expect(chart.lockedIndicator()).to.be(lastPoint);
+                const indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(lastPoint);
+                expect(indicator.isLocked).to.be(true);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, false], [lastPoint.id, true]]);
 
                 expect(selectionChangeCount).to.be(0);
@@ -1034,8 +1040,7 @@ describe('InteractiveTimeSeriesChart', () => {
             let lastPoint;
             return waitForComponentsToRender(context).then(() => {
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(indicatorChangeCalls).to.be.eql([]);
 
                 canvas = chart.content().querySelector('canvas');
@@ -1047,10 +1052,13 @@ describe('InteractiveTimeSeriesChart', () => {
             }).then(() => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
-                lastPoint = chart.sampledTimeSeriesData('current').lastPoint();
+                const currentView = chart.sampledTimeSeriesData('current');
+                lastPoint = currentView.lastPoint();
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(lastPoint);
-                expect(chart.lockedIndicator()).to.be(null);
+                const indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(lastPoint);
+                expect(indicator.isLocked).to.be(false);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, false]]);
 
                 canvas.parentNode.dispatchEvent(new MouseEvent('mousemove', {target: canvas, clientX: rect.right + 50, clientY: rect.bottom + 50, composed: true, bubbles: true}));
@@ -1062,8 +1070,7 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, false], [null, false]]);
 
                 expect(selectionChangeCount).to.be(0);
@@ -1088,11 +1095,11 @@ describe('InteractiveTimeSeriesChart', () => {
 
             let canvas;
             let rect;
+            let currentView;
             let lastPoint;
             return waitForComponentsToRender(context).then(() => {
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(indicatorChangeCalls).to.be.eql([]);
 
                 canvas = chart.content().querySelector('canvas');
@@ -1104,10 +1111,13 @@ describe('InteractiveTimeSeriesChart', () => {
             }).then(() => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
-                lastPoint = chart.sampledTimeSeriesData('current').lastPoint();
+                currentView = chart.sampledTimeSeriesData('current');
+                lastPoint = currentView.lastPoint();
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(lastPoint);
-                expect(chart.lockedIndicator()).to.be(lastPoint);
+                const indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(lastPoint);
+                expect(indicator.isLocked).to.be(true);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, true]]);
 
                 canvas.parentNode.dispatchEvent(new MouseEvent('mousemove', {target: canvas, clientX: rect.right + 50, clientY: rect.bottom + 50, composed: true, bubbles: true}));
@@ -1119,8 +1129,10 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(false);
 
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(lastPoint);
-                expect(chart.lockedIndicator()).to.be(lastPoint);
+                const indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(lastPoint);
+                expect(indicator.isLocked).to.be(true);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, true]]);
 
                 expect(selectionChangeCount).to.be(0);
@@ -1146,11 +1158,11 @@ describe('InteractiveTimeSeriesChart', () => {
             let canvas;
             let rect;
             let y;
+            let currentView;
             let lastPoint;
             return waitForComponentsToRender(context).then(() => {
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(indicatorChangeCalls).to.be.eql([]);
 
                 canvas = chart.content().querySelector('canvas');
@@ -1163,10 +1175,13 @@ describe('InteractiveTimeSeriesChart', () => {
             }).then(() => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
-                lastPoint = chart.sampledTimeSeriesData('current').lastPoint();
+                currentView = chart.sampledTimeSeriesData('current');
+                lastPoint = currentView.lastPoint();
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(lastPoint);
-                expect(chart.lockedIndicator()).to.be(lastPoint);
+                const indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(lastPoint);
+                expect(indicator.isLocked).to.be(true);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, true]]);
 
                 canvas.dispatchEvent(new MouseEvent('click', {target: canvas, clientX: rect.left + 1, clientY: y, composed: true, bubbles: true}));
@@ -1177,9 +1192,11 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.be(null);
-                const firstPoint = chart.sampledTimeSeriesData('current').firstPoint();
-                expect(chart.currentPoint()).to.be(firstPoint);
-                expect(chart.lockedIndicator()).to.be(null);
+                const firstPoint = currentView.firstPoint();
+                const indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(firstPoint);
+                expect(indicator.isLocked).to.be(false);
                 expect(indicatorChangeCalls).to.be.eql([[lastPoint.id, true], [firstPoint.id, false]]);
 
                 expect(selectionChangeCount).to.be(0);
@@ -1209,13 +1226,13 @@ describe('InteractiveTimeSeriesChart', () => {
             let canvas;
             let rect;
             let y;
+            let currentView;
             let firstPoint;
             let oldRange;
             let newRange;
             return waitForComponentsToRender(context).then(() => {
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(selectionChangeCalls).to.be.eql([]);
 
                 canvas = chart.content().querySelector('canvas');
@@ -1228,10 +1245,13 @@ describe('InteractiveTimeSeriesChart', () => {
             }).then(() => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
-                firstPoint = chart.sampledTimeSeriesData('current').firstPoint();
+                currentView = chart.sampledTimeSeriesData('current');
+                firstPoint = currentView.firstPoint();
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(firstPoint);
-                expect(chart.lockedIndicator()).to.be(null);
+                let indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(firstPoint);
+                expect(indicator.isLocked).to.be(false);
                 expect(indicatorChangeCalls).to.be.eql([[firstPoint.id, false]]);
                 expect(zoomButton.offsetHeight).to.be(0);
 
@@ -1243,8 +1263,10 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(false);
 
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(firstPoint);
-                expect(chart.lockedIndicator()).to.be(null);
+                let indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(firstPoint);
+                expect(indicator.isLocked).to.be(false);
                 expect(selectionChangeCalls).to.be.eql([]);
                 expect(indicatorChangeCalls).to.be.eql([[firstPoint.id, false]]);
                 expect(zoomButton.offsetHeight).to.be(0);
@@ -1257,8 +1279,7 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.not.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(selectionChangeCalls.length).to.be(1);
                 oldRange = selectionChangeCalls[0][0];
                 expect(oldRange).to.be.eql(chart.currentSelection());
@@ -1274,8 +1295,7 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.not.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(selectionChangeCalls.length).to.be(2);
                 newRange = selectionChangeCalls[1][0];
                 expect(newRange).to.be.eql(chart.currentSelection());
@@ -1293,8 +1313,7 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.be.eql(newRange);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(selectionChangeCalls.length).to.be(3);
                 expect(selectionChangeCalls[2][0]).to.be.eql(newRange);
                 expect(selectionChangeCalls[2][1]).to.be(true);
@@ -1337,8 +1356,7 @@ describe('InteractiveTimeSeriesChart', () => {
 
                 selection = chart.currentSelection();
                 expect(selection).to.not.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
                 expect(zoomButton.offsetHeight).to.not.be(0);
                 expect(zoomCalls).to.be.eql([]);
                 zoomButton.click();
@@ -1383,8 +1401,7 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.not.be(null);
-                expect(chart.currentPoint()).to.be(null);
-                expect(chart.lockedIndicator()).to.be(null);
+                expect(chart.currentIndicator()).to.be(null);
 
                 canvas.dispatchEvent(new MouseEvent('click', {target: canvas, clientX: rect.left + 1, clientY: y + 5, composed: true, bubbles: true}));
 
@@ -1394,8 +1411,11 @@ describe('InteractiveTimeSeriesChart', () => {
                 expect(CanvasTest.hasCanvasBeenRedrawn(canvas)).to.be(true);
 
                 expect(chart.currentSelection()).to.be(null);
-                expect(chart.currentPoint()).to.be(chart.sampledTimeSeriesData('current').firstPoint());
-                expect(chart.lockedIndicator()).to.be(null);
+                const currentView = chart.sampledTimeSeriesData('current');
+                const indicator = chart.currentIndicator();
+                expect(indicator.view).to.be(currentView);
+                expect(indicator.point).to.be(currentView.firstPoint());
+                expect(indicator.isLocked).to.be(false);
             });
         });
     });
@@ -1460,3 +1480,5 @@ describe('InteractiveTimeSeriesChart', () => {
     });
 
 });
+
+})();
\ No newline at end of file
index 5752e17..1ebbe8a 100644 (file)
@@ -60,15 +60,18 @@ class ChartStatusView extends ComponentBase {
                     previousPoint = view.firstPoint();
                 }
             } else  {
-                currentPoint = this._chart.currentPoint();
-                previousPoint = this._chart.currentPoint(-1);
+                const indicator = this._chart.currentIndicator();
+                if (indicator) {
+                    currentPoint = indicator.point;
+                    previousPoint = indicator.view.previousPoint(currentPoint);
+                }
             }
         } else {
             var data = this._chart.sampledTimeSeriesData('current');
             if (!data)
                 return false;
-            if (data.length)
-                currentPoint = data[data.length - 1];
+            if (data.length())
+                currentPoint = data.lastPoint();
         }
 
         if (currentPoint == this._usedCurrentPoint && previousPoint == this._usedPreviousPoint)
@@ -100,77 +103,55 @@ class ChartStatusView extends ComponentBase {
 
     _computeChartStatus(metric, chart, currentPoint, previousPoint)
     {
-        var currentTimeSeriesData = chart.sampledTimeSeriesData('current');
-        var baselineTimeSeriesData = chart.sampledTimeSeriesData('baseline');
-        var targetTimeSeriesData = chart.sampledTimeSeriesData('target');
-        if (!currentTimeSeriesData)
-            return null;
-
-        var formatter = metric.makeFormatter(3);
-        var deltaFormatter = metric.makeFormatter(2, true);
-
-        if (!currentPoint)
-            currentPoint = currentTimeSeriesData[currentTimeSeriesData.length - 1];
-
-        var baselinePoint = this._findLastPointPriorToTime(currentPoint, baselineTimeSeriesData);
-        var diffFromBaseline = baselinePoint !== undefined ? (currentPoint.value - baselinePoint.value) / baselinePoint.value : undefined;
+        console.assert(currentPoint);
+        const baselineView = chart.sampledTimeSeriesData('baseline');
+        const targetView = chart.sampledTimeSeriesData('target');
+
+        const formatter = metric.makeFormatter(3);
+        const deltaFormatter = metric.makeFormatter(2, true);
+        const smallerIsBetter = metric.isSmallerBetter();
+
+        const labelForDiff = (diff, referencePoint, name, comparison) => {
+            const relativeDiff = Math.abs(diff * 100).toFixed(1);
+            const referenceValue = referencePoint ? ` (${formatter(referencePoint.value)})` : '';
+            return `${relativeDiff}% ${comparison} than ${name}${referenceValue}`;
+        };
 
-        var targetPoint = this._findLastPointPriorToTime(currentPoint, targetTimeSeriesData);
-        var diffFromTarget = targetPoint !== undefined ? (currentPoint.value - targetPoint.value) / targetPoint.value : undefined;
+        const baselinePoint = baselineView ? baselineView.lastPointInTimeRange(0, currentPoint.time) : null;
+        const targetPoint = targetView ? targetView.lastPointInTimeRange(0, currentPoint.time) : null;
 
-        var label = '';
-        var className = '';
+        const diffFromBaseline = baselinePoint ? (currentPoint.value - baselinePoint.value) / baselinePoint.value : undefined;
+        const diffFromTarget = targetPoint ? (currentPoint.value - targetPoint.value) / targetPoint.value : undefined;
 
-        function labelForDiff(diff, referencePoint, name, comparison)
-        {
-            var relativeDiff = Math.abs(diff * 100).toFixed(1);
-            var referenceValue = referencePoint ? ` (${formatter(referencePoint.value)})` : '';
-            return `${relativeDiff}% ${comparison} ${name}${referenceValue}`;
-        }
+        let label = null;
+        let comparison = null;
 
-        var smallerIsBetter = metric.isSmallerBetter();
         if (diffFromBaseline !== undefined && diffFromTarget !== undefined) {
             if (diffFromBaseline > 0 == smallerIsBetter) {
-                label = labelForDiff(diffFromBaseline, baselinePoint, 'baseline', 'worse than');
-                className = 'worse';
+                comparison = 'worse';
+                label = labelForDiff(diffFromBaseline, baselinePoint, 'baseline', comparison);
             } else if (diffFromTarget < 0 == smallerIsBetter) {
-                label = labelForDiff(diffFromTarget, targetPoint, 'target', 'better than');
-                className = 'better';
+                comparison = 'better';
+                label = labelForDiff(diffFromTarget, targetPoint, 'target', comparison);
             } else
                 label = labelForDiff(diffFromTarget, targetPoint, 'target', 'until');
         } else if (diffFromBaseline !== undefined) {
-            className = diffFromBaseline > 0 == smallerIsBetter ? 'worse' : 'better';
-            label = labelForDiff(diffFromBaseline, baselinePoint, 'baseline', className + ' than');
+            comparison = diffFromBaseline > 0 == smallerIsBetter ? 'worse' : 'better';
+            label = labelForDiff(diffFromBaseline, baselinePoint, 'baseline', comparison);
         } else if (diffFromTarget !== undefined) {
-            className = diffFromTarget < 0 == smallerIsBetter ? 'better' : 'worse';
-            label = labelForDiff(diffFromTarget, targetPoint, 'target', className + ' than');
+            comparison = diffFromTarget < 0 == smallerIsBetter ? 'better' : 'worse';
+            label = labelForDiff(diffFromTarget, targetPoint, 'target', comparison);
         }
 
-        var valueDelta = null;
-        var relativeDelta = null;
+        let valueDelta = null;
+        let relativeDelta = null;
         if (previousPoint) {
             valueDelta = deltaFormatter(currentPoint.value - previousPoint.value);
-            var relativeDelta = (currentPoint.value - previousPoint.value) / previousPoint.value;
+            relativeDelta = (currentPoint.value - previousPoint.value) / previousPoint.value;
             relativeDelta = (relativeDelta * 100).toFixed(0) + '%';
         }
-        return {
-            className: className,
-            label: label,
-            currentValue: formatter(currentPoint.value),
-            valueDelta: valueDelta,
-            relativeDelta: relativeDelta,
-        };
-    }
-
-    _findLastPointPriorToTime(currentPoint, timeSeriesData)
-    {
-        if (!currentPoint || !timeSeriesData || !timeSeriesData.length)
-            return undefined;
 
-        var i = 0;
-        while (i < timeSeriesData.length - 1 && timeSeriesData[i + 1].time < currentPoint.time)
-            i++;
-        return timeSeriesData[i];
+        return {className: comparison, label, currentValue: formatter(currentPoint.value), valueDelta, relativeDelta};
     }
 
     static htmlTemplate()
index 9a285dc..a5950d6 100644 (file)
@@ -16,22 +16,14 @@ class InteractiveTimeSeriesChart extends TimeSeriesChart {
         this._renderedAnnotation = null;
     }
 
-    currentPoint(diff)
+    currentIndicator()
     {
         var id = this._indicatorID;
         if (!id)
             return null;
 
-        if (!this._sampledTimeSeriesData) {
-            // FIXME: Why are we not using diff in this code path?
-            this._ensureFetchedTimeSeries();
-            for (var series of this._fetchedTimeSeries) {
-                var point = series.findById(id);
-                if (point)
-                    return point;
-            }
+        if (!this._sampledTimeSeriesData)
             return null;
-        }
 
         for (var view of this._sampledTimeSeriesData) {
             if (!view)
@@ -39,9 +31,7 @@ class InteractiveTimeSeriesChart extends TimeSeriesChart {
             let point = view.findById(id);
             if (!point)
                 continue;
-            if (!diff)
-                return point;
-            return (point && diff > 0 ? view.nextPoint(point) : view.previousPoint(point)) || point;
+            return {view, point, isLocked: this._indicatorIsLocked};
         }
         return null;
     }
@@ -62,8 +52,6 @@ class InteractiveTimeSeriesChart extends TimeSeriesChart {
         return selection && data ? data.firstPointInTimeRange(selection[0], selection[1]) : null;
     }
 
-    lockedIndicator() { return this._indicatorIsLocked ? this.currentPoint() : null; }
-
     setIndicator(id, shouldLock)
     {
         var selectionDidChange = !!this._sampledTimeSeriesData;
@@ -81,16 +69,16 @@ class InteractiveTimeSeriesChart extends TimeSeriesChart {
 
     moveLockedIndicatorWithNotification(forward)
     {
-        if (!this._indicatorID || !this._indicatorIsLocked)
+        const indicator = this.currentIndicator();
+        if (!indicator || !indicator.isLocked)
             return false;
-
         console.assert(!this._selectionTimeRange);
 
-        var point = this.currentPoint(forward ? 1 : -1);
-        if (!point || this._indicatorID == point.id)
+        const newPoint = forward ? indicator.view.nextPoint(indicator.point) : indicator.view.previousPoint(indicator.point);
+        if (!newPoint)
             return false;
 
-        this._indicatorID = point.id;
+        this._indicatorID = newPoint.id;
         this._lastMouseDownLocation = null;
         this._forceRender = true;
 
@@ -425,24 +413,22 @@ class InteractiveTimeSeriesChart extends TimeSeriesChart {
                 this._annotationLabel.style.display = 'none';
         }
 
-        var indicator = this._options.indicator;
-        if (this._indicatorID && indicator) {
-            context.fillStyle = indicator.lineStyle;
-            context.strokeStyle = indicator.lineStyle;
-            context.lineWidth = indicator.lineWidth;
+        const indicatorOptions = this._options.indicator;
+        const indicator = this.currentIndicator();
+        if (indicator && indicatorOptions) {
+            context.fillStyle = indicatorOptions.lineStyle;
+            context.strokeStyle = indicatorOptions.lineStyle;
+            context.lineWidth = indicatorOptions.lineWidth;
 
-            var point = this.currentPoint();
-            if (point) {
-                var x = metrics.timeToX(point.time);
-                var y = metrics.valueToY(point.value);
+            const x = metrics.timeToX(indicator.point.time);
+            const y = metrics.valueToY(indicator.point.value);
 
-                context.beginPath();
-                context.moveTo(x, metrics.chartY);
-                context.lineTo(x, metrics.chartY + metrics.chartHeight);
-                context.stroke();
+            context.beginPath();
+            context.moveTo(x, metrics.chartY);
+            context.lineTo(x, metrics.chartY + metrics.chartHeight);
+            context.stroke();
 
-                this._fillCircle(context, x, y, indicator.pointRadius);
-            }
+            this._fillCircle(context, x, y, indicatorOptions.pointRadius);
         }
 
         var selectionOptions = this._options.selection;
index 09bc340..0b55868 100644 (file)
@@ -205,6 +205,18 @@ class TimeSeriesView {
         return null;
     }
 
+    lastPointInTimeRange(startTime, endTime)
+    {
+        console.assert(startTime <= endTime);
+        for (let point of this._reverse()) {
+            if (point.time < startTime)
+                return null;
+            if (point.time <= endTime)
+                return point;
+        }
+        return null;
+    }
+
     [Symbol.iterator]()
     {
         const data = this._data;
@@ -216,6 +228,22 @@ class TimeSeriesView {
             }
         };
     }
+
+    _reverse()
+    {
+        return {
+            [Symbol.iterator]: () => {
+                const data = this._data;
+                const end = this._startingIndex;
+                let i = this._afterEndingIndex;
+                return {
+                    next() {
+                        return {done: i-- == end, value: data[i]};
+                    }
+                };
+            }
+        }
+    }
 }
 
 if (typeof module != 'undefined')
index ba2f9a8..2cb9bce 100644 (file)
@@ -92,11 +92,11 @@ class ChartPane extends ChartPaneBase {
         var state = [this._platformId, this._metricId];
         if (this._mainChart) {
             var selection = this._mainChart.currentSelection();
-            var currentPoint = this._mainChart.currentPoint();
+            const indicator = this._mainChart.currentIndicator();
             if (selection)
                 state[2] = selection;
-            else if (this._mainChartIndicatorWasLocked && currentPoint)
-                state[2] = currentPoint.id;
+            else if (indicator && indicator.isLocked)
+                state[2] = indicator.point.id;
         }
 
         var graphOptions = new Set;
@@ -387,7 +387,8 @@ class ChartPane extends ChartPaneBase {
         }
 
         var markAsOutlierButton = this.content().querySelector('.mark-as-outlier');
-        var firstSelectedPoint = this._mainChart.lockedIndicator();
+        const indicator = this._mainChart.currentIndicator();
+        let firstSelectedPoint = indicator && indicator.isLocked ? indicator.point : null;
         if (!firstSelectedPoint)
             firstSelectedPoint = this._mainChart.firstSelectedPoint('current');
         var alreayMarkedAsOutlier = firstSelectedPoint && firstSelectedPoint.markedOutlier;
index 2443a76..9cad7f9 100644 (file)
@@ -326,6 +326,14 @@ describe('TimeSeriesView', () => {
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
 
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[1].time), fivePoints[1]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[0].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time + 0.1, fivePoints[2].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
+
             assert.deepEqual([...view], [fivePoints[1], fivePoints[3]]);
         });
     });
@@ -370,6 +378,14 @@ describe('TimeSeriesView', () => {
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
 
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[1].time), fivePoints[1]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[0].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time + 0.1, fivePoints[2].time), fivePoints[2]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
+
             assert.deepEqual([...view], fivePoints.slice(1, 4));
         });
 
@@ -406,6 +422,14 @@ describe('TimeSeriesView', () => {
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
 
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[1].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[0].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time + 0.1, fivePoints[2].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
+
             assert.deepEqual([...view], [fivePoints[3]]);
         });
 
@@ -442,6 +466,14 @@ describe('TimeSeriesView', () => {
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
             assert.deepEqual(view.firstPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
 
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time, fivePoints[1].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[0].time, fivePoints[0].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[1].time + 0.1, fivePoints[2].time), null);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[3].time - 0.1, fivePoints[4].time), fivePoints[3]);
+            assert.deepEqual(view.lastPointInTimeRange(fivePoints[4].time, fivePoints[4].time), null);
+
             assert.deepEqual([...view], [fivePoints[3]]);
         });
     });