Fix UI glitches with a custom analysis test group with a patch
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 30 May 2017 05:03:43 +0000 (05:03 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 30 May 2017 05:03:43 +0000 (05:03 +0000)
https://bugs.webkit.org/show_bug.cgi?id=172694

Reviewed by Sam Weinig.

Fix the following UI glitches with perf try bots:
 - Retrying an A/B testing with a patch fails.
 - A patch specified in an test group does not get specified in the configurator.
 - Drag & dropping a patch doesn't work.
 - Results for custom analysis tasks don't get shown.

* public/api/test-groups.php:
(main): Fix a bug that test group's platform does not match that of the request'ed platform. Since each test
group is associated with platform, just use that instead of querying test_configurations. This resulted in
the configurator not being able to find a triggerable in some cases.

* public/v3/components/custom-analysis-task-configurator.js:
(CustomAnalysisTaskConfigurator):
(CustomAnalysisTaskConfigurator.prototype.setCommitSets): Add patches in the commit set.
(CustomAnalysisTaskConfigurator.prototype._setUploadedFilesToUploader): Now clears the exiting uploaded files
Also renamed from _setUploadedFilesIfEmpty.
(CustomAnalysisTaskConfigurator.prototype._setPatchFiles): Added.
(CustomAnalysisTaskConfigurator.prototype.didConstructShadowTree): We no longer update the list of roots
for the comparsion when a new root is added to the baseline.
(CustomAnalysisTaskConfigurator.prototype._configureComparison): Copy over the list of patches and roots when
starting to configure the comparsion.

* public/v3/components/instant-file-uploader.js:
(InstantFileUploader.prototype.clear): Added.
(InstantFileUploader.prototype.didConstructShadowTree): Added event handlers for dragover & drop events to
allow specifying a patch and root using drag & drop. Unfortunately, this still doesn't work in WebKit due to
a bug in our shadow DOM implementation.
(InstantFileUploader.prototype._didFileInputChange):
(InstantFileUploader.prototype._uploadFiles): Extracted from _didFileInputChange.

* public/v3/pages/analysis-task-page.js:
(AnalysisTaskTestGroupPane.prototype.setAnalysisResults): No longer takes metric.
(AnalysisTaskTestGroupPane.cssTemplate): Removed unused rules. Also disallow flexing on the list of test groups
to avoid the name of a test froup from overflowing on top of the results pane.
(AnalysisTaskPage.prototype._assignTestResultsIfPossible): Set setAnalysisResults even when metric is not set
as is the case for a custom analysis task.
(AnalysisTaskPage.prototype._retryCurrentTestGroup): Use createWithCustomConfiguration to allow retrying of
an A/B testing with a patch in a custom analysis task.
(AnalysisTaskPage.prototype._createTestGroupAfterVerifyingCommitSetList):

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

Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/public/api/test-groups.php
Websites/perf.webkit.org/public/v3/components/custom-analysis-task-configurator.js
Websites/perf.webkit.org/public/v3/components/instant-file-uploader.js
Websites/perf.webkit.org/public/v3/pages/analysis-task-page.js

index 74f07cf..4424993 100644 (file)
@@ -1,3 +1,50 @@
+2017-05-29  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Fix UI glitches with a custom analysis test group with a patch
+        https://bugs.webkit.org/show_bug.cgi?id=172694
+
+        Reviewed by Sam Weinig.
+
+        Fix the following UI glitches with perf try bots:
+         - Retrying an A/B testing with a patch fails.
+         - A patch specified in an test group does not get specified in the configurator.
+         - Drag & dropping a patch doesn't work.
+         - Results for custom analysis tasks don't get shown.
+
+        * public/api/test-groups.php:
+        (main): Fix a bug that test group's platform does not match that of the request'ed platform. Since each test
+        group is associated with platform, just use that instead of querying test_configurations. This resulted in
+        the configurator not being able to find a triggerable in some cases.
+
+        * public/v3/components/custom-analysis-task-configurator.js:
+        (CustomAnalysisTaskConfigurator):
+        (CustomAnalysisTaskConfigurator.prototype.setCommitSets): Add patches in the commit set.
+        (CustomAnalysisTaskConfigurator.prototype._setUploadedFilesToUploader): Now clears the exiting uploaded files
+        Also renamed from _setUploadedFilesIfEmpty.
+        (CustomAnalysisTaskConfigurator.prototype._setPatchFiles): Added.
+        (CustomAnalysisTaskConfigurator.prototype.didConstructShadowTree): We no longer update the list of roots
+        for the comparsion when a new root is added to the baseline.
+        (CustomAnalysisTaskConfigurator.prototype._configureComparison): Copy over the list of patches and roots when
+        starting to configure the comparsion.
+
+        * public/v3/components/instant-file-uploader.js:
+        (InstantFileUploader.prototype.clear): Added.
+        (InstantFileUploader.prototype.didConstructShadowTree): Added event handlers for dragover & drop events to
+        allow specifying a patch and root using drag & drop. Unfortunately, this still doesn't work in WebKit due to
+        a bug in our shadow DOM implementation.
+        (InstantFileUploader.prototype._didFileInputChange):
+        (InstantFileUploader.prototype._uploadFiles): Extracted from _didFileInputChange.
+
+        * public/v3/pages/analysis-task-page.js:
+        (AnalysisTaskTestGroupPane.prototype.setAnalysisResults): No longer takes metric.
+        (AnalysisTaskTestGroupPane.cssTemplate): Removed unused rules. Also disallow flexing on the list of test groups
+        to avoid the name of a test froup from overflowing on top of the results pane.
+        (AnalysisTaskPage.prototype._assignTestResultsIfPossible): Set setAnalysisResults even when metric is not set
+        as is the case for a custom analysis task.
+        (AnalysisTaskPage.prototype._retryCurrentTestGroup): Use createWithCustomConfiguration to allow retrying of
+        an A/B testing with a patch in a custom analysis task.
+        (AnalysisTaskPage.prototype._createTestGroupAfterVerifyingCommitSetList):
+
 2017-05-26  Ryosuke Niwa  <rniwa@webkit.org>
 
         Show patches applied in each A/B testing build requests
index 9110c7e..42ddd30 100644 (file)
@@ -38,16 +38,9 @@ function main($path) {
     foreach ($test_groups as &$group) {
         $group_id = $group['id'];
         $group_by_id[$group_id] = &$group;
-        $platforms = $db->query_and_fetch_all('SELECT DISTINCT(config_platform)
-            FROM test_configurations, test_runs, build_requests
-            WHERE run_config = config_id AND run_build = request_build AND request_group = $1', array($group_id));
-        if ($platforms)
-            $group['platform'] = $platforms[0]['config_platform'];
-        else {
-            $first_request = $db->select_first_row('build_requests', 'request', array('group' => $group_id), 'order');
-            if ($first_request)
-                $group['platform'] = $first_request['request_platform'];
-        }
+        $first_request = $db->select_first_row('build_requests', 'request', array('group' => $group_id), 'order');
+        if ($first_request)
+            $group['platform'] = $first_request['request_platform'];
     }
 
     $build_requests = $build_requests_fetcher->results();
index 06b4e7f..b70f204 100644 (file)
@@ -12,6 +12,7 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
         this._commitSetMap = {};
         this._specifiedRevisions = {'Baseline': new Map, 'Comparison': new Map};
         this._patchUploaders = {'Baseline': new Map, 'Comparison': new Map};
+        this._customRootUploaders = {'Baseline': null, 'Comparison': null};
         this._fetchedRevisions = {'Baseline': new Map, 'Comparison': new Map};
         this._repositoryGroupByConfiguration = {'Baseline': null, 'Comparison': null};
         this._updateTriggerableLazily = new LazilyEvaluatedFunction(this._updateTriggerable.bind(this));
@@ -19,8 +20,6 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
         this._renderTriggerableTestsLazily = new LazilyEvaluatedFunction(this._renderTriggerableTests.bind(this));
         this._renderTriggerablePlatformsLazily = new LazilyEvaluatedFunction(this._renderTriggerablePlatforms.bind(this));
         this._renderRepositoryPanesLazily = new LazilyEvaluatedFunction(this._renderRepositoryPanes.bind(this));
-
-        this._customRootUploaders = {};
     }
 
     tests() { return this._selectedTests; }
@@ -65,29 +64,41 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
         const baselineRepositoryGroup = triggerable.repositoryGroups().find((repositoryGroup) => repositoryGroup.accepts(baselineCommitSet));
         if (baselineRepositoryGroup) {
             this._repositoryGroupByConfiguration['Baseline'] = baselineRepositoryGroup;
-            this._setUploadedFilesIfEmpty(this._customRootUploaders['Baseline'], baselineCommitSet);
+            this._setUploadedFilesToUploader(this._customRootUploaders['Baseline'], baselineCommitSet.customRoots());
             this._specifiedRevisions['Baseline'] = this._revisionMapFromCommitSet(baselineCommitSet);
+            this._setPatchFiles('Baseline', baselineCommitSet);
         }
 
         const comparisonRepositoryGroup = triggerable.repositoryGroups().find((repositoryGroup) => repositoryGroup.accepts(baselineCommitSet));
         if (comparisonRepositoryGroup) {
             this._repositoryGroupByConfiguration['Comparison'] = comparisonRepositoryGroup;
-            this._setUploadedFilesIfEmpty(this._customRootUploaders['Comparison'], comparisonCommitSet);
+            this._setUploadedFilesToUploader(this._customRootUploaders['Comparison'], comparisonCommitSet.customRoots());
             this._specifiedRevisions['Comparison'] = this._revisionMapFromCommitSet(comparisonCommitSet);
+            this._setPatchFiles('Comparison', comparisonCommitSet);
         }
 
         this._showComparison = true;
         this._updateCommitSetMap();
     }
 
-    _setUploadedFilesIfEmpty(uploader, commitSet)
+    _setUploadedFilesToUploader(uploader, files)
     {
         if (!uploader || uploader.hasFileToUpload() || uploader.uploadedFiles().length)
             return;
-        for (const uploadedFile of commitSet.customRoots())
+        uploader.clearUploads();
+        for (const uploadedFile of files)
             uploader.addUploadedFile(uploadedFile);
     }
 
+    _setPatchFiles(configurationName, commitSet)
+    {
+        for (const repository of commitSet.repositories()) {
+            const patch = commitSet.patchForRepository(repository);
+            if (patch)
+                this._setUploadedFilesToUploader(this._ensurePatchUploader(configurationName, repository), [patch]);
+        }
+    }
+
     _revisionMapFromCommitSet(commitSet)
     {
         const revisionMap = new Map;
@@ -109,11 +120,7 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
         }
 
         const baselineRootsUploader = createRootUploader();
-        baselineRootsUploader.listenToAction('uploadedFile', (uploadedFile) => {
-            comparisonRootsUploader.addUploadedFile(uploadedFile);
-            this._updateCommitSetMap();
-        });
-
+        baselineRootsUploader.listenToAction('uploadedFile', (uploadedFile) => this._updateCommitSetMap());
         this._customRootUploaders['Baseline'] = baselineRootsUploader;
 
         const comparisonRootsUploader = createRootUploader();
@@ -148,6 +155,19 @@ class CustomAnalysisTaskConfigurator extends ComponentBase {
             specifiedComparisonRevisions.set(key, specifiedBaselineRevisions.get(key));
         this._specifiedRevisions['Comparison'] = specifiedComparisonRevisions;
 
+        for (const [repository, patchUploader] of this._patchUploaders['Baseline']) {
+            const files = patchUploader.uploadedFiles();
+            if (!files.length)
+                continue;
+            const comparisonPatchUploader = this._ensurePatchUploader('Comparison', repository);
+            for (const uploadedFile of files)
+                comparisonPatchUploader.addUploadedFile(uploadedFile);
+        }
+
+        const comparisonRootUploader = this._customRootUploaders['Comparison'];
+        for (const uploadedFile of this._customRootUploaders['Baseline'].uploadedFiles())
+            comparisonRootUploader.addUploadedFile(uploadedFile);
+
         this.enqueueToRender();
     }
 
index daef4a5..339099a 100644 (file)
@@ -13,6 +13,12 @@ class InstantFileUploader extends ComponentBase {
         this._renderPreuploadFilesLazily = new LazilyEvaluatedFunction(this._renderPreuploadFiles.bind(this));
     }
 
+    clearUploads()
+    {
+        this._uploadedFiles = [];
+        this._preuploadFiles = [];
+        this._uploadProgress = new WeakMap;
+    }
     hasFileToUpload() { return !!this._preuploadFiles.length; }
     uploadedFiles() { return this._uploadedFiles; }
 
@@ -33,9 +39,23 @@ class InstantFileUploader extends ComponentBase {
 
     didConstructShadowTree()
     {
-        this.content('file-adder').onclick = () => {
+        const addButton = this.content('file-adder');
+        addButton.onclick = () => {
             inputElement.click();
         }
+        addButton.addEventListener('dragover', (event) => {
+            event.dataTransfer.dropEffect = 'copy';
+            event.preventDefault();
+        });
+        addButton.addEventListener('drop', (event) => {
+            event.preventDefault();
+            let files = event.dataTransfer.files;
+            if (!files.length)
+                return;
+            if (files.length > 1 && !this._allowMultipleFiles)
+                files = [files[0]];
+            this._uploadFiles(files);
+        });
         const inputElement = document.createElement('input');
         inputElement.type = 'file';
         inputElement.onchange = () => this._didFileInputChange(inputElement);
@@ -125,13 +145,23 @@ class InstantFileUploader extends ComponentBase {
     {
         if (!input.files.length)
             return;
+        this._uploadFiles(input.files);
+        input.value = null;
+        this.enqueueToRender();
+    }
+
+    _uploadFiles(files)
+    {
         const limit = UploadedFile.fileUploadSizeLimit;
-        for (let file of input.files) {
+        for (let file of files) {
             if (file.size > limit) {
                 alert(`The specified file "${file.name}" is too big (${this._fileSizeFormatter(file.size)}). It must be smaller than ${this._fileSizeFormatter(limit)}`);
-                input.value = null;
                 return;
             }
+        }
+
+        const uploadProgress = this._uploadProgress;
+        for (let file of files) {
             UploadedFile.fetchUnloadedFileWithIdenticalHash(file).then((uploadedFile) => {
                 if (uploadedFile) {
                     this._didUploadFile(file, uploadedFile);
@@ -139,20 +169,20 @@ class InstantFileUploader extends ComponentBase {
                 }
 
                 UploadedFile.uploadFile(file, (progress) => {
-                    this._uploadProgress.set(file, progress);
-                    this.enqueueToRender();
+                    uploadProgress.set(file, progress);
+                    if (this._uploadProgress == uploadProgress)
+                        this.enqueueToRender();
                 }).then((uploadedFile) => {
-                    this._didUploadFile(file, uploadedFile);
+                    if (this._uploadProgress == uploadProgress)
+                        this._didUploadFile(file, uploadedFile);
                 }, (error) => {
-                    this._uploadProgress.set(file, {error: error === 0 ? 'UnknownError' : error});
-                    this.enqueueToRender();
+                    uploadProgress.set(file, {error: error === 0 ? 'UnknownError' : error});
+                    if (this._uploadedProgress == uploadProgress)
+                        this.enqueueToRender();
                 });
             });
         }
-        this._preuploadFiles = Array.from(input.files);
-        input.value = null;
-
-        this.enqueueToRender();
+        this._preuploadFiles = Array.from(files);
     }
 
     _removeUploadedFile(uploadedFile)
index d580d53..3dc4358 100644 (file)
@@ -280,10 +280,10 @@ class AnalysisTaskTestGroupPane extends ComponentBase {
         this.enqueueToRender();
     }
 
-    setAnalysisResults(analysisResults, metric)
+    setAnalysisResults(analysisResults)
     {
         this.part('revision-table').setAnalysisResults(analysisResults);
-        this.part('results-viewer').setAnalysisResults(analysisResults, metric);
+        this.part('results-viewer').setAnalysisResults(analysisResults, null);
         this.enqueueToRender();
     }
 
@@ -369,15 +369,8 @@ class AnalysisTaskTestGroupPane extends ComponentBase {
                 font-size: 0.9rem;
             }
 
-            #new-container {
-                display: flex;
-            }
-
-            #new-container test-group-revision-table {
-                margin-left: 2rem;
-            }
-
             #test-group-list {
+                flex: none;
                 margin: 0;
                 padding: 0.2rem 0;
                 list-style: none;
@@ -617,12 +610,15 @@ class AnalysisTaskPage extends PageWithHeading {
 
     _assignTestResultsIfPossible()
     {
-        if (!this._task || !this._metric || !this._testGroups || !this._analysisResults)
+        if (!this._task || !this._testGroups || !this._analysisResults)
             return false;
 
-        const view = this._analysisResults.viewForMetric(this._metric);
-        this.part('group-pane').setAnalysisResults(this._analysisResults, this._metric);
-        this.part('results-pane').setAnalysisResultsView(view);
+        this.part('group-pane').setAnalysisResults(this._analysisResults);
+        let metric = this._metric;
+        if (metric) {
+            const view = this._analysisResults.viewForMetric(metric);
+            this.part('results-pane').setAnalysisResultsView(view);
+        }
 
         return true;
     }
@@ -791,11 +787,11 @@ class AnalysisTaskPage extends PageWithHeading {
     {
         const newName = this._createRetryNameForTestGroup(testGroup.name());
         const commitSetList = testGroup.requestedCommitSets();
-
-        const commitSetMap = {};
-        for (let commitSet of commitSetList)
-            commitSetMap[testGroup.labelForCommitSet(commitSet)] = commitSet;
-
+        const platform = this._task.platform() || testGroup.platform();
+        return TestGroup.createWithCustomConfiguration(this._task, platform, testGroup.test(), newName, repetitionCount, commitSetList)
+            .then(this._didFetchTestGroups.bind(this), function (error) {
+            alert('Failed to create a new test group: ' + error);
+        });
         return this._createTestGroupAfterVerifyingCommitSetList(newName, repetitionCount, commitSetMap);
     }
 
@@ -825,7 +821,7 @@ class AnalysisTaskPage extends PageWithHeading {
         for (let label in commitSetMap)
             commitSets.push(commitSetMap[label]);
 
-        TestGroup.createAndRefetchTestGroups(this._task, testGroupName, repetitionCount, commitSets)
+        return TestGroup.createAndRefetchTestGroups(this._task, testGroupName, repetitionCount, commitSets)
             .then(this._didFetchTestGroups.bind(this), function (error) {
             alert('Failed to create a new test group: ' + error);
         });