Changeset 176710 in webkit

Timestamp:

Dec 2, 2014, 8:17:32 PM (12 years ago)

Author:

rniwa@webkit.org

Message:

New perf dashboard's chart UI is buggy
​https://bugs.webkit.org/show_bug.cgi?id=139214

Reviewed by Chris Dumez.

The bugginess was caused by weird interactions between charts and panes. Rewrote the code to fix it.

Superfluous selectionChanged and domainChanged "event" actions were removed from the interactive chart
component. This is not how Ember.js components should interact to begin with. The component now exposes
selectedPoints and always updates selection instead of sharedSelection.

• public/v2/app.js:

(App.ChartsController.present): Added. We can't call Date.now() in various points in our code as that
would lead to infinite mutual recursions since X-axis domain values wouldn't match up.
(App.ChartsController.updateSharedDomain): This function was completely useless. The overview's start
and end time should be completely determined by "since" and the present time.
(App.ChartsController._startTimeChanged): Ditto.
(App.ChartsController._scheduleQueryStringUpdate):
(App.ChartsController._updateQueryString): Set "zoom" only if it's different from the shared domain.

(App.domainsAreEqual): Moved from InteractiveChartComponent._xDomainsAreSame.

(App.PaneController.actions.createAnalysisTask): Use selectedPoints property set by the chart.
(App.PaneController.actions.overviewDomainChanged): Removed; only needed to call updateSharedDomain.
(App.PaneController.actions.rangeChanged): Removed. _showDetails (renamed to _updateDetails) directly
observes the changes to selectedPoints property as it gets updated by the main chart.
(App.PaneController._overviewSelectionChanged): This was previously a dead code. Now it's used again
with a bug fix. When the overview selection is cleared, we use the same domain in the main chart and
the overview chart.
(App.PaneController._sharedDomainChanged): Fixed a but that it erroneously updates the overview domain
when domain arrays aren't identical. This was causing a subtle race with other logic.
(App.PaneController._sharedZoomChanged): Ditto. Also don't set mainPlotDomain here as any changes to
overviewSelection will automatically propagate to the main plot's domain as they're aliased.
(App.PaneController._currentItemChanged): Merged into _updateDetails (renamed from _showDetails).
(App.PaneController._updateDetails): Previously, this function took points and inspected _hasRange to
see if those two points correspond to a range or a single data point. Rewrote all that logic by
directly observing selectedPoints and currentItem properties instead of taking points and relying on
an instance variable, which was a terrible API.
(App.PaneController._updateCanAnalyze): Use selectedPoints property. Since this property is only set
when the main plot has a selected range, we don't have to check this._hasRange anymore.

(App.InteractiveChartComponent._updateDomain): No longer sends domainChanged "event" action.
(App.InteractiveChartComponent._sharedSelectionChanged): Removed. This is a dead code.
(App.InteractiveChartComponent._updateSelection):
(App.InteractiveChartComponent._xDomainsAreSame): Moved to App.domainsAreEqual.
(App.InteractiveChartComponent._setCurrentSelection): Update the selection only if needed. Also set
selectedPoints property.

(App.AnalysisTaskController._fetchedRuns):
(App.AnalysisTaskController._rootChangedForTestSet):

• public/v2/index.html:

Removed non-functional sharedSelection and superfluous selectionChanged and domainChanged actions.

Location:

trunk/Websites/perf.webkit.org

Files:

• ChangeLog (modified) (1 diff)
• public/v2/app.js (modified) (21 diffs)
• public/v2/index.html (modified) (2 diffs)

trunk/Websites/perf.webkit.org/ChangeLog

@@ +1 @@
+2014-12-02  Ryosuke Niwa  <rniwa@webkit.org>
+
+        New perf dashboard's chart UI is buggy
+        https://bugs.webkit.org/show_bug.cgi?id=139214
+
+        Reviewed by Chris Dumez.
+
+        The bugginess was caused by weird interactions between charts and panes. Rewrote the code to fix it.
+
+        Superfluous selectionChanged and domainChanged "event" actions were removed from the interactive chart
+        component. This is not how Ember.js components should interact to begin with. The component now exposes
+        selectedPoints and always updates selection instead of sharedSelection.
+
+        * public/v2/app.js:
+        (App.ChartsController.present): Added. We can't call Date.now() in various points in our code as that
+        would lead to infinite mutual recursions since X-axis domain values wouldn't match up.
+        (App.ChartsController.updateSharedDomain): This function was completely useless. The overview's start
+        and end time should be completely determined by "since" and the present time.
+        (App.ChartsController._startTimeChanged): Ditto.
+        (App.ChartsController._scheduleQueryStringUpdate):
+        (App.ChartsController._updateQueryString): Set "zoom" only if it's different from the shared domain.
+
+        (App.domainsAreEqual): Moved from InteractiveChartComponent._xDomainsAreSame.
+
+        (App.PaneController.actions.createAnalysisTask): Use selectedPoints property set by the chart.
+        (App.PaneController.actions.overviewDomainChanged): Removed; only needed to call updateSharedDomain.
+        (App.PaneController.actions.rangeChanged): Removed. _showDetails (renamed to _updateDetails) directly
+        observes the changes to selectedPoints property as it gets updated by the main chart.
+        (App.PaneController._overviewSelectionChanged): This was previously a dead code. Now it's used again
+        with a bug fix. When the overview selection is cleared, we use the same domain in the main chart and
+        the overview chart.
+        (App.PaneController._sharedDomainChanged): Fixed a but that it erroneously updates the overview domain
+        when domain arrays aren't identical. This was causing a subtle race with other logic.
+        (App.PaneController._sharedZoomChanged): Ditto. Also don't set mainPlotDomain here as any changes to
+        overviewSelection will automatically propagate to the main plot's domain as they're aliased.
+        (App.PaneController._currentItemChanged): Merged into _updateDetails (renamed from _showDetails).
+        (App.PaneController._updateDetails): Previously, this function took points and inspected _hasRange to
+        see if those two points correspond to a range or a single data point. Rewrote all that logic by
+        directly observing selectedPoints and currentItem properties instead of taking points and relying on
+        an instance variable, which was a terrible API.
+        (App.PaneController._updateCanAnalyze): Use selectedPoints property. Since this property is only set
+        when the main plot has a selected range, we don't have to check this._hasRange anymore.
+
+        (App.InteractiveChartComponent._updateDomain): No longer sends domainChanged "event" action.
+        (App.InteractiveChartComponent._sharedSelectionChanged): Removed. This is a dead code.
+        (App.InteractiveChartComponent._updateSelection):
+        (App.InteractiveChartComponent._xDomainsAreSame): Moved to App.domainsAreEqual.
+        (App.InteractiveChartComponent._setCurrentSelection): Update the selection only if needed. Also set
+        selectedPoints property.
+
+        (App.AnalysisTaskController._fetchedRuns):
+        (App.AnalysisTaskController._rootChangedForTestSet):
+
+        * public/v2/index.html:
+        Removed non-functional sharedSelection and superfluous selectionChanged and domainChanged actions.
+
 2014-11-21  Ryosuke Niwa  <rniwa@webkit.org>
 

trunk/Websites/perf.webkit.org/public/v2/app.js

@@ -400 +400 @@
     sharedZoom: null,
     startTime: null,
+    present: Date.now(),
     defaultSince: Date.now() - 7 * 24 * 3600 * 1000,
 
@@ -451 +452 @@
     }.observes('zoom').on('init'),
 
-    updateSharedDomain: function ()
-    {
-        var panes = this.get('panes');
-        if (!panes.length)
-            return;
-
-        var union = [undefined, undefined];
-        for (var i = 0; i < panes.length; i++) {
-            var domain = panes[i].intrinsicDomain;
-            if (!domain)
-                continue;
-            if (!union[0] || domain[0] < union[0])
-                union[0] = domain[0];
-            if (!union[1] || domain[1] > union[1])
-                union[1] = domain[1];
-        }
-        if (union[0] === undefined)
-            return;
-
-        var startTime = this.get('startTime');
-        var zoom = this.get('sharedZoom');
-        if (startTime)
-            union[0] = zoom ? Math.min(zoom[0], startTime) : startTime;
-
-        this.set('sharedDomain', union);
-    }.observes('panes.@each'),
-
     _startTimeChanged: function () {
-        this.updateSharedDomain();
+        this.set('sharedDomain', [this.get('startTime'), this.get('present')]);
         this._scheduleQueryStringUpdate();
     }.observes('startTime'),
@@ -537 +511 @@
     _scheduleQueryStringUpdate: function ()
     {
-        Ember.run.debounce(this, '_updateQueryString', 500);
-    }.observes('sharedZoom')
-        .observes('panes.@each.platform', 'panes.@each.metric', 'panes.@each.selectedItem',
+        Ember.run.debounce(this, '_updateQueryString', 1000);
+    }.observes('sharedZoom', 'panes.@each.platform', 'panes.@each.metric', 'panes.@each.selectedItem',
         'panes.@each.timeRange', 'panes.@each.timeRangeIsLocked'),
 
@@ -548 +521 @@
 
         var zoom = undefined;
-        var selection = this.get('sharedZoom');
-        if (selection)
-            zoom = (selection[0] - 0) + '-' + (selection[1] - 0);
+        var sharedZoom = this.get('sharedZoom');
+        if (sharedZoom && !App.domainsAreEqual(sharedZoom, this.get('sharedDomain')))
+            zoom = +sharedZoom[0] + '-' + +sharedZoom[1];
         this.set('zoom', zoom);
 
@@ -635 +608 @@
     }.property('childTests', 'metrics'),
 });
+
+App.domainsAreEqual = function (domain1, domain2) {
+    return (!domain1 && !domain2) || (domain1 && domain2 && !(domain1[0] - domain2[0]) && !(domain1[1] - domain2[1]));
+}
 
 App.PaneController = Ember.ObjectController.extend({
@@ -658 +635 @@
         {
             var name = this.get('newAnalysisTaskName');
-            var points = this._selectedPoints;
+            var points = this.get('selectedPoints');
             Ember.assert('The analysis name should not be empty', name);
             Ember.assert('There should be at least two points in the range', points && points.length >= 2);
@@ -696 +673 @@
             Ember.run.debounce(this, 'propagateZoom', 100);
         },
-        overviewDomainChanged: function (domain, intrinsicDomain)
-        {
-            this.set('overviewDomain', domain);
-            this.set('intrinsicDomain', intrinsicDomain);
-            this.get('parentController').updateSharedDomain();
-        },
-        rangeChanged: function (extent, points)
-        {
-            if (!points) {
-                this._hasRange = false;
-                this.set('details', null);
-                this.set('timeRange', null);
-                return;
-            }
-            this._hasRange = true;
-            this._showDetails(points);
-            this.set('timeRange', extent);
-        },
     },
     _detailsChanged: function ()
@@ -722 +681 @@
     {
         var overviewSelection = this.get('overviewSelection');
-        this.set('mainPlotDomain', overviewSelection);
+        this.set('mainPlotDomain', overviewSelection || this.get('overviewDomain'));
         Ember.run.debounce(this, 'propagateZoom', 100);
     }.observes('overviewSelection'),
@@ -728 +687 @@
     {
         var newDomain = this.get('parentController').get('sharedDomain');
-        if (newDomain == this.get('overviewDomain'))
+        if (App.domainsAreEqual(newDomain, this.get('overviewDomain')))
             return;
         this.set('overviewDomain', newDomain);
@@ -741 +700 @@
     {
         var newSelection = this.get('parentController').get('sharedZoom');
-        if (newSelection == this.get('mainPlotDomain'))
+        if (App.domainsAreEqual(newSelection, this.get('mainPlotDomain')))
             return;
         this.set('overviewSelection', newSelection);
-        this.set('mainPlotDomain', newSelection);
     }.observes('parentController.sharedZoom').on('init'),
-    _currentItemChanged: function ()
-    {
-        if (this._hasRange)
-            return;
-        var point = this.get('currentItem');
-        if (!point || !point.measurement)
+    _updateDetails: function ()
+    {
+        var selectedPoints = this.get('selectedPoints');
+        var currentPoint = this.get('currentItem');
+        if (!selectedPoints && !currentPoint) {
             this.set('details', null);
-        else {
-            var previousPoint = point.series.previousPoint(point);
-            this._showDetails(previousPoint ? [previousPoint, point] : [point]);
-        }
-    }.observes('currentItem'),
-    _showDetails: function (points)
-    {
-        var isShowingEndPoint = !this._hasRange;
-        var currentMeasurement = points[points.length - 1].measurement;
-        var oldMeasurement = points[0].measurement;
+            return;
+        }
+
+        var currentMeasurement;
+        var oldMeasurement;
+        if (currentPoint) {
+            currentMeasurement = currentPoint.measurement;
+            var previousPoint = currentPoint.series.previousPoint(currentPoint);
+            oldMeasurement = previousPoint ? previousPoint.measurement : null;
+        } else {
+            currentMeasurement = selectedPoints[selectedPoints.length - 1].measurement;
+            oldMeasurement = selectedPoints[0].measurement;            
+        }
+
         var formattedRevisions = currentMeasurement.formattedRevisions(oldMeasurement);
         var revisions = App.Manifest.get('repositories')
@@ -779 +740 @@
         var buildNumber = null;
         var buildURL = null;
-        if (isShowingEndPoint) {
+        if (currentPoint) {
             buildNumber = currentMeasurement.buildNumber();
             var builder = App.Manifest.builder(currentMeasurement.builderId());
@@ -786 +747 @@
         }
 
-        this._selectedPoints = points;
         this.set('details', Ember.Object.create({
             currentValue: currentMeasurement.mean().toFixed(2),
-            oldValue: oldMeasurement && !isShowingEndPoint ? oldMeasurement.mean().toFixed(2) : null,
+            oldValue: oldMeasurement && selectedPoints ? oldMeasurement.mean().toFixed(2) : null,
             buildNumber: buildNumber,
             buildURL: buildURL,
@@ -796 +756 @@
         }));
         this._updateCanAnalyze();
-    },
+    }.observes('currentItem', 'selectedPoints'),
     _updateCanAnalyze: function ()
     {
-        var points = this._selectedPoints;
-        this.set('cannotAnalyze', !this.get('newAnalysisTaskName') || !this._hasRange || !points || points.length < 2);
+        var points = this.get('selectedPoints');
+        this.set('cannotAnalyze', !this.get('newAnalysisTaskName') || !points || points.length < 2);
     }.observes('newAnalysisTaskName'),
 });
@@ -995 +955 @@
             xDomain = intrinsicXDomain;
         var currentDomain = this._x.domain();
-        if (currentDomain && this._xDomainsAreSame(currentDomain, xDomain))
+        if (currentDomain && App.domainsAreEqual(currentDomain, xDomain))
             return currentDomain;
 
@@ -1001 +961 @@
         this._x.domain(xDomain);
         this._y.domain(yDomain);
-        this.sendAction('domainChanged', xDomain, intrinsicXDomain);
         return xDomain;
     },
@@ -1181 +1140 @@
         this._updateSelection(this.get('selection'));
     }.observes('selection'),
-    _sharedSelectionChanged: function ()
-    {
-        if (this.get('selectionIsLocked'))
-            return;
-        this._updateSelection(this.get('sharedSelection'));
-    }.observes('sharedSelection'),
     _updateSelection: function (newSelection)
     {
@@ -1193 +1146 @@
 
         var currentSelection = this._currentSelection();
-        if (newSelection && currentSelection && this._xDomainsAreSame(newSelection, currentSelection))
+        if (newSelection && currentSelection && App.domainsAreEqual(newSelection, currentSelection))
             return;
 
         var domain = this._x.domain();
-        if (!newSelection || this._xDomainsAreSame(domain, newSelection))
+        if (!newSelection || App.domainsAreEqual(domain, newSelection))
             this._brush.clear();
         else
@@ -1204 +1157 @@
 
         this._setCurrentSelection(newSelection);
-    },
-    _xDomainsAreSame: function (domain1, domain2)
-    {
-        return !(domain1[0] - domain2[0]) && !(domain1[1] - domain2[1]);
     },
     _brushChanged: function ()
@@ -1558 +1507 @@
         this._updateSelectionToolbar();
 
-        this.set('sharedSelection', newSelection);
-        this.sendAction('selectionChanged', newSelection, points);
+        if (!App.domainsAreEqual(this.get('selection'), newSelection))
+            this.set('selection', newSelection);
+        this.set('selectedPoints', points);
     },
     _updateSelectionToolbar: function ()
@@ -1676 +1626 @@
         }));
     },
-    _fetchedRuns: function (data) {
+    _fetchedRuns: function (data)
+    {
         var runs = data.runs;
 
@@ -1719 +1670 @@
         ];
     }.property('analysisPoints'),
-    _rootChangedForTestSet: function () {
+    _rootChangedForTestSet: function ()
+    {
         var sets = this.get('testSets');
         var roots = this.get('roots');

trunk/Websites/perf.webkit.org/public/v2/index.html

@@ -157 +157 @@
                             rangeRoute="analysisTask"
                             selection=timeRange
-                            sharedSelection=sharedSelection
-                            selectionChanged="rangeChanged"
+                            selectedPoints=selectedPoints
                             selectionIsLocked=timeRangeIsLocked
                             markedPoints=markedPoints
@@ -177 +176 @@
                                 showYAxis=false
                                 domain=overviewDomain
-                                domainChanged="overviewDomainChanged"
-                                selection=mainPlotDomain
-                                selectionChanged="zoomed"}}
+                                selection=overviewSelection}}
                         {{/if}}
                         </div>