Charts page show an inconsistent list of revisions for Git and Subversion
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Mar 2017 01:19:39 +0000 (01:19 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 21 Mar 2017 01:19:39 +0000 (01:19 +0000)
https://bugs.webkit.org/show_bug.cgi?id=169888

Reviewed by Andreas Kling.

With Git, CommitLogViewer was showing the list of revisions including the starting hash,
which was the last data point's revision instead of all revisions after the last data point.

Fixed the bug by always specifying the revision at the last data point in both Subversion
and Git and then making /api/commits/<repository>/?from=X&to=Y exclude the first revision.
For clarity, "from" and "to" query parameters have been renamed to "precedingRevision" and
"lastRevision" respectively.

We also no longer adds 1 to the starting revision of Subversion-like starting revisions. e.g.
when the last data point was at r1234, new data point is at r1250, the label is now "r1234-r1250"
instead of "r1235-r1250".

* browser-tests/chart-revision-range-tests.js: Fixed the tests since revisionList no longer
specifies from/to revisions.
* browser-tests/commit-log-viewer-tests.js: Added. Added tests for CommitLogViewer.
* browser-tests/index.html: Include the new test. Also use a local copy of mocha.js/css.

* public/api/commits.php:
(main): Renamed "from" and "to" query parameters.

* public/include/commit-log-fetcher.php:
(CommitLogFetcher::fetch_between): Added a check that commit time should either be specified
in both rows or not specified in either. Also reject when before_first_revision is identical
or after last_revision instead of re-ordering them since it no longer makes sense to do so with
new query parameter names.

* public/v3/components/base.js:
(ComponentBase._addContentToElement): Use Array.isArray instead of instanceof. It's resilient
againt realm (global object) differences.

* public/v3/components/chart-pane-base.js:
(ChartPaneBase.prototype._updateCommitLogViewer): No longer calls enqueueToRender on this since
CommitLogViewer does that on its own now.
(ChartPaneBase.prototype.render): Juse use this._openRepository instead of relying on CommitLogViewer
to remember which repository is current. This was the only use of currentRepository.

* public/v3/components/commit-log-viewer.js:
(CommitLogViewer):
(CommitLogViewer.prototype.currentRepository): Deleted.
(CommitLogViewer.prototype.view):
(CommitLogViewer.prototype._fetchCommitLogs): Modernized and extracted from view to make it lazy.
Call fetchForSingleRevision when precedingRevision is not specified or it's identical to lastRevision
since the generic JSON API no longer supports being called with the identical revisions.
(CommitLogViewer.prototype.render): Modernized & simplified the code.
(CommitLogViewer.prototype._renderCommitList): Extracted from render to make it lazy.
(CommitLogViewer.htmlTemplate): Add ID on caption & tbody so that they're more easily addressable.
(CommitLogViewer.cssTemplate):

* public/v3/models/commit-log.js:
(CommitLog.prototype.diff): No longer includes from/to revisions in the result. Also avoid adding
1 to a Subversion-like starting revision for creating the label. See above. But we still do this
for forming URLs due to the way tools like Trac work with Subversion revisions.
(CommitLog.fetchBetweenRevisions): Rewritten using DataModel.prototype.cachedFetch with FIXME for
what this function is supposed to be doing.
(CommitLog._cachedCommitLogs): Deleted.
(CommitLog.fetchForSingleRevision): Added.
(CommitLog._constructFromRawData): Added.

* public/v3/models/data-model.js:
(DataModelObject.cachedFetch): Don't parse query values as an integer. Just URL-escape them.

* public/v3/remote.js:
(BrowserRemoteAPI.prototype.sendHttpRequest): Fixed a typo.

* server-tests/api-commits-tests.js: Renamed from api-commits.js. Updated the existing tests to
use new query parameters and added more test cases.

* unit-tests/commit-log-tests.js: Updated the test cases now that CommitLog.prototype.diff no longer
includes from/to values. They're computed in ChartRevisionRange instead.

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

14 files changed:
Websites/perf.webkit.org/ChangeLog
Websites/perf.webkit.org/browser-tests/chart-revision-range-tests.js
Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js [new file with mode: 0644]
Websites/perf.webkit.org/browser-tests/index.html
Websites/perf.webkit.org/public/api/commits.php
Websites/perf.webkit.org/public/include/commit-log-fetcher.php
Websites/perf.webkit.org/public/v3/components/base.js
Websites/perf.webkit.org/public/v3/components/chart-pane-base.js
Websites/perf.webkit.org/public/v3/components/commit-log-viewer.js
Websites/perf.webkit.org/public/v3/models/commit-log.js
Websites/perf.webkit.org/public/v3/models/data-model.js
Websites/perf.webkit.org/public/v3/remote.js
Websites/perf.webkit.org/server-tests/api-commits-tests.js [moved from Websites/perf.webkit.org/server-tests/api-commits.js with 76% similarity]
Websites/perf.webkit.org/unit-tests/commit-log-tests.js

index fc12eb1..1379f25 100644 (file)
@@ -1,3 +1,80 @@
+2017-03-19  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Charts page show an inconsistent list of revisions for Git and Subversion
+        https://bugs.webkit.org/show_bug.cgi?id=169888
+
+        Reviewed by Andreas Kling.
+
+        With Git, CommitLogViewer was showing the list of revisions including the starting hash,
+        which was the last data point's revision instead of all revisions after the last data point.
+
+        Fixed the bug by always specifying the revision at the last data point in both Subversion
+        and Git and then making /api/commits/<repository>/?from=X&to=Y exclude the first revision.
+        For clarity, "from" and "to" query parameters have been renamed to "precedingRevision" and
+        "lastRevision" respectively.
+
+        We also no longer adds 1 to the starting revision of Subversion-like starting revisions. e.g.
+        when the last data point was at r1234, new data point is at r1250, the label is now "r1234-r1250"
+        instead of "r1235-r1250".
+
+        * browser-tests/chart-revision-range-tests.js: Fixed the tests since revisionList no longer
+        specifies from/to revisions.
+        * browser-tests/commit-log-viewer-tests.js: Added. Added tests for CommitLogViewer.
+        * browser-tests/index.html: Include the new test. Also use a local copy of mocha.js/css.
+
+        * public/api/commits.php:
+        (main): Renamed "from" and "to" query parameters.
+
+        * public/include/commit-log-fetcher.php:
+        (CommitLogFetcher::fetch_between): Added a check that commit time should either be specified
+        in both rows or not specified in either. Also reject when before_first_revision is identical
+        or after last_revision instead of re-ordering them since it no longer makes sense to do so with
+        new query parameter names.
+
+        * public/v3/components/base.js:
+        (ComponentBase._addContentToElement): Use Array.isArray instead of instanceof. It's resilient
+        againt realm (global object) differences.
+
+        * public/v3/components/chart-pane-base.js:
+        (ChartPaneBase.prototype._updateCommitLogViewer): No longer calls enqueueToRender on this since
+        CommitLogViewer does that on its own now.
+        (ChartPaneBase.prototype.render): Juse use this._openRepository instead of relying on CommitLogViewer
+        to remember which repository is current. This was the only use of currentRepository.
+
+        * public/v3/components/commit-log-viewer.js:
+        (CommitLogViewer):
+        (CommitLogViewer.prototype.currentRepository): Deleted.
+        (CommitLogViewer.prototype.view):
+        (CommitLogViewer.prototype._fetchCommitLogs): Modernized and extracted from view to make it lazy.
+        Call fetchForSingleRevision when precedingRevision is not specified or it's identical to lastRevision
+        since the generic JSON API no longer supports being called with the identical revisions.
+        (CommitLogViewer.prototype.render): Modernized & simplified the code.
+        (CommitLogViewer.prototype._renderCommitList): Extracted from render to make it lazy.
+        (CommitLogViewer.htmlTemplate): Add ID on caption & tbody so that they're more easily addressable.
+        (CommitLogViewer.cssTemplate):
+
+        * public/v3/models/commit-log.js:
+        (CommitLog.prototype.diff): No longer includes from/to revisions in the result. Also avoid adding
+        1 to a Subversion-like starting revision for creating the label. See above. But we still do this
+        for forming URLs due to the way tools like Trac work with Subversion revisions.
+        (CommitLog.fetchBetweenRevisions): Rewritten using DataModel.prototype.cachedFetch with FIXME for
+        what this function is supposed to be doing.
+        (CommitLog._cachedCommitLogs): Deleted.
+        (CommitLog.fetchForSingleRevision): Added.
+        (CommitLog._constructFromRawData): Added.
+
+        * public/v3/models/data-model.js:
+        (DataModelObject.cachedFetch): Don't parse query values as an integer. Just URL-escape them.
+
+        * public/v3/remote.js:
+        (BrowserRemoteAPI.prototype.sendHttpRequest): Fixed a typo.
+
+        * server-tests/api-commits-tests.js: Renamed from api-commits.js. Updated the existing tests to
+        use new query parameters and added more test cases.
+
+        * unit-tests/commit-log-tests.js: Updated the test cases now that CommitLog.prototype.diff no longer
+        includes from/to values. They're computed in ChartRevisionRange instead.
+
 2017-03-20  Ryosuke Niwa  <rniwa@webkit.org>
 
         Fix os-build-fetcher.js and subprocess.js to make them work
index 3e70323..b1b7066 100644 (file)
@@ -33,13 +33,9 @@ describe('ChartRevisionRange', () => {
 
                 expect(revisionList[0].repository.label()).to.be('SomeApp');
                 expect(revisionList[0].label).to.be('r4006');
-                expect(revisionList[0].from).to.be(null);
-                expect(revisionList[0].to).to.be('4006');
 
                 expect(revisionList[1].repository.label()).to.be('macOS');
                 expect(revisionList[1].label).to.be('15C50');
-                expect(revisionList[1].from).to.be(null);
-                expect(revisionList[1].to).to.be('15C50');
             })
         });
     });
@@ -96,14 +92,10 @@ describe('ChartRevisionRange', () => {
                 expect(revisionList.length).to.be(2);
 
                 expect(revisionList[0].repository.label()).to.be('SomeApp');
-                expect(revisionList[0].label).to.be('r4005-r4006');
-                expect(revisionList[0].from).to.be('4005');
-                expect(revisionList[0].to).to.be('4006');
+                expect(revisionList[0].label).to.be('r4004-r4006');
 
                 expect(revisionList[1].repository.label()).to.be('macOS');
                 expect(revisionList[1].label).to.be('15C50');
-                expect(revisionList[1].from).to.be(null);
-                expect(revisionList[1].to).to.be('15C50');
 
                 chart.setIndicator(1004, true); // Across macOS change.
 
@@ -111,14 +103,10 @@ describe('ChartRevisionRange', () => {
                 expect(revisionList.length).to.be(2);
 
                 expect(revisionList[0].repository.label()).to.be('SomeApp');
-                expect(revisionList[0].label).to.be('r4004-r4004');
-                expect(revisionList[0].from).to.be('4004');
-                expect(revisionList[0].to).to.be('4004');
+                expect(revisionList[0].label).to.be('r4003-r4004');
 
                 expect(revisionList[1].repository.label()).to.be('macOS');
                 expect(revisionList[1].label).to.be('15B42 - 15C50');
-                expect(revisionList[1].from).to.be('15B42');
-                expect(revisionList[1].to).to.be('15C50');
             });
         });
 
@@ -150,14 +138,10 @@ describe('ChartRevisionRange', () => {
                 expect(revisionList.length).to.be(2);
 
                 expect(revisionList[0].repository.label()).to.be('SomeApp');
-                expect(revisionList[0].label).to.be('r4003-r4004'); // 4002 and 4005 are outliers and skipped.
-                expect(revisionList[0].from).to.be('4003');
-                expect(revisionList[0].to).to.be('4004');
+                expect(revisionList[0].label).to.be('r4002-r4004'); // 4001 and 4005 are outliers and skipped.
 
                 expect(revisionList[1].repository.label()).to.be('macOS');
                 expect(revisionList[1].label).to.be('15B42 - 15C50');
-                expect(revisionList[1].from).to.be('15B42');
-                expect(revisionList[1].to).to.be('15C50');
             });
         });
     });
diff --git a/Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js b/Websites/perf.webkit.org/browser-tests/commit-log-viewer-tests.js
new file mode 100644 (file)
index 0000000..7f52dc1
--- /dev/null
@@ -0,0 +1,123 @@
+describe('CommitLogViewer', () => {
+
+    function importCommitLogViewer(context)
+    {
+        const scripts = [
+            '../shared/statistics.js',
+            'instrumentation.js',
+            'lazily-evaluated-function.js',
+            'models/data-model.js',
+            'models/repository.js',
+            'models/commit-set.js',
+            'models/commit-log.js',
+            'components/base.js',
+            'components/spinner-icon.js',
+            'components/commit-log-viewer.js'];
+        return context.importScripts(scripts, 'ComponentBase', 'CommitLogViewer', 'Repository', 'CommitLog', 'RemoteAPI').then(() => {
+            return context.symbols.CommitLogViewer;
+        });
+    }
+
+    const webkitCommit210949 = {
+        "id": "185334",
+        "revision": "210949",
+        "previousCommit": null,
+        "time": +new Date("2017-01-20T03:23:50.645Z"),
+        "authorName": "Chris Dumez",
+        "authorEmail": "cdumez@apple.com",
+        "message": "some message",
+    };
+
+    const webkitCommit210950 = {
+        "id": "185338",
+        "revision": "210950",
+        "previousCommit": null,
+        "time": +new Date("2017-01-20T03:49:37.887Z"),
+        "authorName": "Commit Queue",
+        "authorEmail": "commit-queue@webkit.org",
+        "message": "another message",
+    };
+
+    it('should initially be empty with no spinner', () => {
+        const context = new BrowsingContext();
+        return importCommitLogViewer(context).then((CommitLogViewer) => {
+            const viewer = new CommitLogViewer;
+            context.document.body.appendChild(viewer.element());
+            viewer.enqueueToRender();
+            return waitForComponentsToRender(context).then(() => {
+                expect(viewer.content('spinner-container').offsetHeight).to.be(0);
+                expect(viewer.content('commits-list').matches(':empty')).to.be(true);
+                expect(viewer.content('repository-name').matches(':empty')).to.be(true);
+            });
+        });
+    });
+
+    it('should show the repository name and the spinner once it started fetching the list of commits ', () => {
+        const context = new BrowsingContext();
+        return importCommitLogViewer(context).then((CommitLogViewer) => {
+            const Repository = context.symbols.Repository;
+            const SpinnerIcon = context.symbols.SpinnerIcon;
+            const ComponentBase = context.symbols.ComponentBase;
+            const RemoteAPI = context.symbols.RemoteAPI;
+            const viewer = new CommitLogViewer;
+            const webkit = new Repository(1, {name: 'WebKit'});
+            context.document.body.appendChild(viewer.element());
+            viewer.enqueueToRender();
+            return waitForComponentsToRender(context).then(() => {
+                expect(viewer.content('spinner-container').offsetHeight).to.be(0);
+                expect(viewer.content('commits-list').matches(':empty')).to.be(true);
+                expect(viewer.content('repository-name').matches(':empty')).to.be(true);
+                expect(RemoteAPI.requests.length, 0);
+                viewer.view(webkit, '210948', '210950');
+                expect(RemoteAPI.requests.length, 1);
+                return waitForComponentsToRender(context);
+            }).then(() => {
+                expect(viewer.content('spinner-container').offsetHeight).to.not.be(0);
+                expect(viewer.content('commits-list').matches(':empty')).to.be(true);
+                expect(viewer.content('repository-name').matches(':empty')).to.be(false);
+                expect(viewer.content('repository-name').textContent).to.contain('WebKit');
+            });
+        });
+    });
+
+    it('should show the repository name, the list of commits, and hide the spinner once the list of commits are fetched', () => {
+        const context = new BrowsingContext();
+        return importCommitLogViewer(context).then((CommitLogViewer) => {
+            const Repository = context.symbols.Repository;
+            const SpinnerIcon = context.symbols.SpinnerIcon;
+            const ComponentBase = context.symbols.ComponentBase;
+            const requests = context.symbols.RemoteAPI.requests;
+            const viewer = new CommitLogViewer;
+            const webkit = new Repository(1, {name: 'WebKit'});
+            context.document.body.appendChild(viewer.element());
+            viewer.enqueueToRender();
+            return waitForComponentsToRender(context).then(() => {
+                viewer.view(webkit, '210948', '210950');
+                return waitForComponentsToRender(context);
+            }).then(() => {
+                expect(viewer.content('spinner-container').offsetHeight).to.not.be(0);
+                expect(viewer.content('commits-list').matches(':empty')).to.be(true);
+                expect(viewer.content('repository-name').matches(':empty')).to.be(false);
+                expect(viewer.content('repository-name').textContent).to.contain('WebKit');
+                expect(requests.length).to.be(1);
+                expect(requests[0].url).to.be('/api/commits/1/?precedingRevision=210948&lastRevision=210950');
+                requests[0].resolve({
+                    "status": "OK",
+                    "commits": [webkitCommit210949, webkitCommit210950],
+                });
+                return waitForComponentsToRender(context);
+            }).then(() => {
+                expect(viewer.content('spinner-container').offsetHeight).to.be(0);
+                expect(viewer.content('commits-list').matches(':empty')).to.be(false);
+                expect(viewer.content('commits-list').textContent).to.contain('r210949');
+                expect(viewer.content('commits-list').textContent).to.contain('Chris Dumez');
+                expect(viewer.content('commits-list').textContent).to.contain('r210950');
+                expect(viewer.content('commits-list').textContent).to.contain('Commit Queue');
+                expect(viewer.content('repository-name').matches(':empty')).to.be(false);
+                expect(viewer.content('repository-name').textContent).to.contain('WebKit');
+                expect(viewer.content('commits-list').querySelector('a')).to.be(null);
+            });
+        });
+    });
+
+});
index 252a39b..6330452 100644 (file)
@@ -1,9 +1,10 @@
 <!DOCTYPE html>
 <html>
 <head>
+<meta charset="utf-8">
 <title>In-Browser Tests for Performance Dashboard</title>
-<link rel="stylesheet" href="https://cdn.rawgit.com/mochajs/mocha/2.2.5/mocha.css">
-<script src="https://cdnjs.cloudflare.com/ajax/libs/mocha/2.2.5/mocha.js"></script>
+<link rel="stylesheet" href="../node_modules/mocha/mocha.css">
+<script src="../node_modules/mocha/mocha.js"></script>
 <script src="https://cdnjs.cloudflare.com/ajax/libs/expect.js/0.2.0/expect.min.js"></script>
 <script>
 
@@ -21,6 +22,7 @@ mocha.setup('bdd');
 <script src="interactive-time-series-chart-tests.js"></script>
 <script src="chart-status-evaluator-tests.js"></script>
 <script src="chart-revision-range-tests.js"></script>
+<script src="commit-log-viewer-tests.js"></script>
 <script>
 
 afterEach(() => {
index a6a4db8..c11dd6b 100644 (file)
@@ -24,9 +24,9 @@ function main($paths) {
     $commits = array();
     if (!$filter) {
         $keyword = array_get($_GET, 'keyword'); // V2 UI compatibility.
-        $from = array_get($_GET, 'from');
-        $to = array_get($_GET, 'to');
-        $commits = $fetcher->fetch_between($repository_id, $from, $to, $keyword);
+        $preceding_revision = array_get($_GET, 'precedingRevision');
+        $last_revision = array_get($_GET, 'lastRevision');
+        $commits = $fetcher->fetch_between($repository_id, $preceding_revision, $last_revision, $keyword);
     } else if ($filter == 'oldest') {
         $commits = $fetcher->fetch_oldest($repository_id);
     } else if ($filter == 'latest') {
index 05d0a01..7c15660 100644 (file)
@@ -33,7 +33,7 @@ class CommitLogFetcher {
         return $repository_row['repository_id'];
     }
 
-    function fetch_between($repository_id, $first, $second, $keyword = NULL)
+    function fetch_between($repository_id, $before_first_revision, $last_revision, $keyword = NULL)
     {
         $statements = 'SELECT commit_id as "id",
             commit_revision as "revision",
@@ -46,22 +46,29 @@ class CommitLogFetcher {
             WHERE commit_repository = $1 AND commit_reported = true';
         $values = array($repository_id);
 
-        if ($first && $second) {
-            $first_commit = $this->commit_for_revision($repository_id, $first);
-            $second_commit = $this->commit_for_revision($repository_id, $second);
-            $first = $first_commit['commit_time'];
-            $second = $second_commit['commit_time'];
-            $column_name = 'commit_time';
-            if (!$first || !$second) {
-                $first = $first_commit['commit_order'];
-                $second = $second_commit['commit_order'];
+        if ($before_first_revision && $last_revision) {
+            $preceding_commit = $this->commit_for_revision($repository_id, $before_first_revision);
+            $last_commit = $this->commit_for_revision($repository_id, $last_revision);
+
+            $preceding = $preceding_commit['commit_time'];
+            $last = $last_commit['commit_time'];
+            if (!$preceding != !$last)
+                exit_with_error('InconsistentCommits');
+            if ($preceding)
+                $column_name = 'commit_time';
+            else {
+                $preceding = $preceding_commit['commit_order'];
+                $last = $last_commit['commit_order'];
                 $column_name = 'commit_order';
+                if (!$preceding || !$last)
+                    return array();
             }
+            if ($preceding >= $last)
+                exit_with_error('InvalidCommitRange');
 
-            $in_order = $first < $second;
-            array_push($values, $in_order ? $first : $second);
-            $statements .= ' AND ' . $column_name . ' >= $' . count($values);
-            array_push($values, $in_order ? $second : $first);
+            array_push($values, $preceding);
+            $statements .= ' AND ' . $column_name . ' > $' . count($values);
+            array_push($values, $last);
             $statements .= ' AND ' . $column_name . ' <= $' . count($values);
         }
 
index 9f75cd0..365363d 100644 (file)
@@ -238,7 +238,7 @@ class ComponentBase {
 
     static _addContentToElement(element, content)
     {
-        if (content instanceof Array) {
+        if (Array.isArray(content)) {
             for (var nestedChild of content)
                 this._addContentToElement(element, nestedChild);
         } else if (content instanceof Node)
index 5743bab..ae1f1ea 100644 (file)
@@ -163,9 +163,8 @@ class ChartPaneBase extends ComponentBase {
     _updateCommitLogViewer()
     {
         const range = this._revisionRange.rangeForRepository(this._openRepository);
-        const updateRendering = () => { this.enqueueToRender(); };
-        this._commitLogViewer.view(this._openRepository, range.from, range.to).then(updateRendering);
-        updateRendering();
+        this._commitLogViewer.view(this._openRepository, range.from, range.to);
+        this.enqueueToRender();
     }
 
     _openAnalysisTask(annotation)
@@ -257,7 +256,7 @@ class ChartPaneBase extends ComponentBase {
             this._mainChartStatus.enqueueToRender();
 
         var body = this.content().querySelector('.chart-pane-body');
-        if (this._commitLogViewer && this._commitLogViewer.currentRepository()) {
+        if (this._openRepository) {
             body.classList.add('has-second-sidebar');
             this._commitLogViewer.enqueueToRender();
         } else
index 4ecc326..9a5d174 100644 (file)
@@ -4,92 +4,87 @@ class CommitLogViewer extends ComponentBase {
     constructor()
     {
         super('commit-log-viewer');
+        this._lazilyFetchCommitLogs = new LazilyEvaluatedFunction(this._fetchCommitLogs.bind(this));
         this._repository = null;
         this._fetchingPromise = null;
         this._commits = null;
-        this._from = null;
-        this._to = null;
+        this._renderCommitListLazily = new LazilyEvaluatedFunction(this._renderCommitList.bind(this));
     }
 
-    currentRepository() { return this._repository; }
-
-    view(repository, from, to)
+    view(repository, precedingRevision, lastRevision)
     {
-        if (this._repository == repository && this._from == from && this._to == to)
-            return Promise.resolve(null);
+        this._lazilyFetchCommitLogs.evaluate(repository, precedingRevision, lastRevision);
+    }
 
+    _fetchCommitLogs(repository, precedingRevision, lastRevision)
+    {
+        this._fetchingPromise = null;
         this._commits = null;
-        this._repository = repository;
-        this._from = from;
-        this._to = to;
 
-        if (!repository) {
-            this._fetchingPromise = null;
+        if (!repository || !lastRevision) {
             this._repository = null;
-            return Promise.resolve(null);
-        }
-
-        if (!to) {
-            this._fetchingPromise = null;
             this.enqueueToRender();
-            return Promise.resolve(null);
+            return;
         }
 
-        var promise = CommitLog.fetchBetweenRevisions(repository, from || to, to);
+        let promise;
+        if (!precedingRevision || precedingRevision == lastRevision)
+            promise = CommitLog.fetchForSingleRevision(repository, lastRevision);
+        else
+            promise = CommitLog.fetchBetweenRevisions(repository, precedingRevision, lastRevision);
 
+        this._repository = repository;
         this._fetchingPromise = promise;
 
-        var self = this;
-        var spinnerTimer = setTimeout(function () {
-            self.enqueueToRender();
-        }, 300);
-
-        this._fetchingPromise.then(function (commits) {
-            clearTimeout(spinnerTimer);
-            if (self._fetchingPromise != promise)
+        this._fetchingPromise.then((commits) => {
+            if (this._fetchingPromise != promise)
                 return;
-            self._fetchingPromise = null;
-            self._commits = commits;
-        }, function (error) {
-            if (self._fetchingPromise != promise)
+            this._fetchingPromise = null;
+            this._commits = commits;
+            this.enqueueToRender();
+        }, (error) => {
+            if (this._fetchingPromise != promise)
                 return;
-            self._fetchingPromise = null;
-            self._commits = null;
+            this._fetchingPromise = null;
+            this._commits = null;
+            this.enqueueToRender();
         });
 
-        return this._fetchingPromise;
+        this.enqueueToRender();
     }
 
     render()
     {
-        var caption = '';
-        if (this._repository && (this._commits || this._fetchingPromise))
-            caption = this._repository.name();
-        this.content().querySelector('caption').textContent = caption;
+        this.part('spinner').enqueueToRender();
+
+        const shouldShowRepositoryName = this._repository && (this._commits || this._fetchingPromise);
+        this.content('repository-name').textContent = shouldShowRepositoryName ? this._repository.name() : '';
+        this.content('spinner-container').style.display = this._fetchingPromise ? null : 'none';
+        this._renderCommitListLazily.evaluate(this._commits);
+    }
 
-        var element = ComponentBase.createElement;
-        var link = ComponentBase.createLink;
+    _renderCommitList(commits)
+    {
+        const element = ComponentBase.createElement;
+        const link = ComponentBase.createLink;
 
-        this.renderReplace(this.content().querySelector('tbody'), (this._commits || []).map(function (commit) {
-            var label = commit.label();
-            var url = commit.url();
+        this.renderReplace(this.content('commits-list'), (commits || []).map((commit) => {
+            const label = commit.label();
+            const url = commit.url();
             return element('tr', [
                 element('th', [element('h4', {class: 'revision'}, url ? link(label, commit.title(), url) : label), commit.author() || '']),
                 element('td', commit.message() ? commit.message().substring(0, 80) : '')]);
         }));
-
-        this.content().querySelector('.commits-viewer-spinner').style.display = this._fetchingPromise ? null : 'none';
     }
 
     static htmlTemplate()
     {
         return `
             <div class="commits-viewer-container">
-                <div class="commits-viewer-spinner"><spinner-icon></spinner-icon></div>
-                <table class="commits-viewer-table">
-                    <caption></caption>
-                    <tbody>
-                    </tbody>
+                <div id="spinner-container"><spinner-icon id="spinner"></spinner-icon></div>
+                <table id="commits-viewer-table">
+                    <caption id="repository-name"></caption>
+                    <tbody id="commits-list"></tbody>
                 </table>
             </div>
 `;
@@ -104,31 +99,27 @@ class CommitLogViewer extends ComponentBase {
                 overflow-y: scroll;
             }
             
-            .commits-viewer-table {
+            #commits-viewer-table {
                 width: 100%;
+                border-collapse: collapse;
+                border-bottom: solid 1px #ccc;
             }
 
-            .commits-viewer-table caption {
+            caption {
                 font-weight: inherit;
                 font-size: 1rem;
                 text-align: center;
                 padding: 0.2rem;
             }
 
-            .commits-viewer-table {
-                border-collapse: collapse;
-                border-bottom: solid 1px #ccc;
-            }
-
-            .commits-viewer-table .revision {
+            .revision {
                 white-space: nowrap;
                 font-weight: normal;
                 margin: 0;
                 padding: 0;
             }
 
-            .commits-viewer-table td,
-            .commits-viewer-table th {
+            td, th {
                 word-break: break-word;
                 border-top: solid 1px #ccc;
                 padding: 0.2rem;
@@ -137,7 +128,7 @@ class CommitLogViewer extends ComponentBase {
                 font-weight: normal;
             }
 
-            .commits-viewer-spinner {
+            #spinner-container {
                 margin-top: 2rem;
                 text-align: center;
             }
index 4eba7e1..5624af5 100644 (file)
@@ -49,70 +49,46 @@ class CommitLog extends DataModelObject {
         if (this == previousCommit)
             previousCommit = null;
 
-        var repository = this._repository;
+        const repository = this._repository;
         if (!previousCommit)
-            return {from: null, to: this.revision(), repository: repository, label: this.label(), url: this.url()};
-
-        var to = this.revision();
-        var from = previousCommit.revision();
-        var label = null;
-        if (parseInt(to) == to) { // e.g. r12345.
-            from = (parseInt(from) + 1).toString();
+            return {repository: repository, label: this.label(), url: this.url()};
+
+        const to = this.revision();
+        const from = previousCommit.revision();
+        let fromRevisionForURL = from;
+        let label = null;
+        if (parseInt(from) == from) { // e.g. r12345.
+            fromRevisionForURL = (parseInt(from) + 1).toString;
             label = `r${from}-r${this.revision()}`;
-        } else if (to.length == 40) // e.g. git hash
+        } else if (to.length == 40) // e.g. git hash
             label = `${from.substring(0, 8)}..${to.substring(0, 8)}`;
-        else
+        else
             label = `${from} - ${to}`;
 
-        return {from: from, to: to, repository: repository, label: label, url: repository.urlForRevisionRange(from, to)};
+        return {repository: repository, label: label, url: repository.urlForRevisionRange(from, to)};
     }
 
-    static fetchBetweenRevisions(repository, from, to)
+    static fetchBetweenRevisions(repository, precedingRevision, lastRevision)
     {
-        var params = [];
-        if (from && to) {
-            params.push(['from', from]);
-            params.push(['to', to]);
-        }
-
-        var url = '../api/commits/' + repository.id() + '/?' + params.map(function (keyValue) {
-            return encodeURIComponent(keyValue[0]) + '=' + encodeURIComponent(keyValue[1]);
-        }).join('&');
-
-
-        var cachedLogs = this._cachedCommitLogs(repository, from, to);
-        if (cachedLogs)
-            return new Promise(function (resolve) { resolve(cachedLogs); });
-
-        var self = this;
-        return RemoteAPI.getJSONWithStatus(url).then(function (data) {
-            var commits = data['commits'].map(function (rawData) {
-                rawData.repository = repository;
-                return CommitLog.ensureSingleton(rawData.id, rawData);
-            });
-            self._cacheCommitLogs(repository, from, to, commits);
-            return commits;
-        });
+        // FIXME: The cache should be smarter about fetching a range within an already fetched range, etc...
+        // FIXME: We should evict some entires from the cache in cachedFetch.
+        return this.cachedFetch(`/api/commits/${repository.id()}/`, {precedingRevision, lastRevision})
+            .then((data) => this._constructFromRawData(repository, data));
     }
 
-    static _cachedCommitLogs(repository, from, to)
+    static fetchForSingleRevision(repository, revision)
     {
-        if (!this._caches)
-            return null;
-        var cache = this._caches[repository.id()];
-        if (!cache)
-            return null;
-        // FIXME: Make each commit know of its parent, then we can do a better caching. 
-        return cache[from + '|' + to];
+        return this.cachedFetch(`/api/commits/${repository.id()}/${revision}`).then((data) => {
+            return this._constructFromRawData(repository, data);
+        });
     }
 
-    static _cacheCommitLogs(repository, from, to, logs)
+    static _constructFromRawData(repository, data)
     {
-        if (!this._caches)
-            this._caches = {};
-        if (!this._caches[repository.id()])
-            this._caches[repository.id()] = {};
-        this._caches[repository.id()][from + '|' + to] = logs;
+        return data['commits'].map((rawData) => {
+            rawData.repository = repository;
+            return CommitLog.ensureSingleton(rawData.id, rawData);
+        });
     }
 }
 
index f805c56..63170f7 100644 (file)
@@ -62,10 +62,10 @@ class DataModelObject {
 
     static cachedFetch(path, params, noCache)
     {
-        var query = [];
+        const query = [];
         if (params) {
-            for (var key in params)
-                query.push(key + '=' + parseInt(params[key]));
+            for (let key in params)
+                query.push(key + '=' + escape(params[key]));
         }
         if (query.length)
             path += '?' + query.join('&');
index 0eee833..8d9203b 100644 (file)
@@ -12,7 +12,7 @@ class BrowserRemoteAPI extends CommonRemoteAPI {
             function onload() {
                 Instrumentation.endMeasuringTime('Remote', 'sendHttpRequest');
                 if (xhr.status != 200)
-                    return resject(xhr.status);
+                    return reject(xhr.status);
                 resolve({statusCode: xhr.status, responseText: xhr.responseText});
             };
 
@@ -396,7 +396,7 @@ describe("/api/commits/", function () {
 
     });
 
-    describe('/api/commits/<repository>/?from=<commit-1>&to=<commit-2>', () => {
+    describe('/api/commits/<repository>/?precedingRevision=<commit-1>&lastRevision=<commit-2>', () => {
         it("should return RepositoryNotFound when there are no matching repository", () => {
             return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210900&to=211000').then((response) => {
                 assert.equal(response['status'], 'RepositoryNotFound');
@@ -409,20 +409,106 @@ describe("/api/commits/", function () {
                 db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
                 db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'})
             ]).then(() => {
-                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210900&to=211000');
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?precedingRevision=210900&lastRevision=211000');
             }).then((response) => {
                 assert.equal(response['status'], 'UnknownCommit');
             });
         });
 
-        it("should return an empty result when commits in the specified range have not reported", () => {
+        it("should return an empty result when commits in the specified range have not been reported", () => {
             const db = TestServer.database();
             return Promise.all([
                 db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
                 db.insert('commits', {'repository': 1, 'revision': '210949', 'time': '2017-01-20T03:23:50.645Z'}),
                 db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'}),
             ]).then(() => {
-                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210949&to=210950');
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?precedingRevision=210949&lastRevision=210950');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return InvalidCommitRange when the specified range is backwards", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210949', 'time': '2017-01-20T03:23:50.645Z'}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z'}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?precedingRevision=210950&lastRevision=210949');
+            }).then((response) => {
+                assert.equal(response['status'], 'InvalidCommitRange');
+            });
+        });
+
+        it("should return use the commit order when time is not specified", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'macOS'}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16A323', order: 1, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2555', order: 2, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2657', order: 3, 'reported': true}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/macOS/?precedingRevision=10.12%2016A323&lastRevision=10.12%2016B2657');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'].map((commit) => commit['revision']), ['10.12 16B2555', '10.12 16B2657']);
+            });
+        });
+
+        it("should return InconsistentCommits when precedingRevision specifies a time but lastRevision does not", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'macOS'}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16A323', time: '2017-01-20T03:23:50.645Z', order: 1, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2555', order: 2, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2657', order: 3, 'reported': true}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/macOS/?precedingRevision=10.12%2016A323&lastRevision=10.12%2016B2657');
+            }).then((response) => {
+                assert.equal(response['status'], 'InconsistentCommits');
+            });
+        });
+
+        it("should return InconsistentCommits when precedingRevision does not specify a time has a time but lastRevision does", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'macOS'}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16A323', order: 1, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2555', order: 2, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2657', time: '2017-01-20T03:23:50.645Z', order: 3, 'reported': true}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/macOS/?precedingRevision=10.12%2016A323&lastRevision=10.12%2016B2657');
+            }).then((response) => {
+                assert.equal(response['status'], 'InconsistentCommits');
+            });
+        });
+
+        it("should return empty results when precedingRevision does not specify a time or an order has a time but lastRevision does", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'macOS'}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16A323', 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2555', order: 2, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2657', order: 3, 'reported': true}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/macOS/?precedingRevision=10.12%2016A323&lastRevision=10.12%2016B2657');
+            }).then((response) => {
+                assert.equal(response['status'], 'OK');
+                assert.deepEqual(response['commits'], []);
+            });
+        });
+
+        it("should return empty results when precedingRevision an order has a time but lastRevision does not", () => {
+            const db = TestServer.database();
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'macOS'}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16A323', order: 1, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2555', order: 2, 'reported': true}),
+                db.insert('commits', {'repository': 1, 'revision': '10.12 16B2657', 'reported': true}),
+            ]).then(() => {
+                return TestServer.remoteAPI().getJSON('/api/commits/macOS/?precedingRevision=10.12%2016A323&lastRevision=10.12%2016B2657');
             }).then((response) => {
                 assert.equal(response['status'], 'OK');
                 assert.deepEqual(response['commits'], []);
@@ -433,10 +519,11 @@ describe("/api/commits/", function () {
             const db = TestServer.database();
             return Promise.all([
                 db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210948', 'time': '2017-01-20T02:52:34.577Z', 'reported': true}),
                 db.insert('commits', {'repository': 1, 'revision': '210949', 'time': '2017-01-20T03:23:50.645Z', 'reported': true}),
                 db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z', 'reported': true}),
             ]).then(() => {
-                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?from=210949&to=210950');
+                return TestServer.remoteAPI().getJSON('/api/commits/WebKit/?precedingRevision=210948&lastRevision=210950');
             }).then((result) => {
                 assert.equal(result['status'], 'OK');
                 assert.deepEqual(result['commits'].length, 2);
@@ -458,11 +545,20 @@ describe("/api/commits/", function () {
         });
 
         it("should not include a revision not within the specified range", () => {
+            const db = TestServer.database();
             const remote = TestServer.remoteAPI();
-            return addSlaveForReport(subversionCommits).then(() => {
+            return Promise.all([
+                db.insert('repositories', {'id': 1, 'name': 'WebKit'}),
+                db.insert('commits', {'repository': 1, 'revision': '210947', 'time': '2017-01-20T02:38:45.485Z', 'reported': false}),
+                db.insert('commits', {'repository': 1, 'revision': '210948', 'time': '2017-01-20T02:52:34.577Z', 'reported': false}),
+                db.insert('commits', {'repository': 1, 'revision': '210949', 'time': '2017-01-20T03:23:50.645Z', 'reported': false}),
+                db.insert('commits', {'repository': 1, 'revision': '210950', 'time': '2017-01-20T03:49:37.887Z', 'reported': false}),
+            ]).then(() => {
+                return addSlaveForReport(subversionCommits);
+            }).then(() => {
                 return remote.postJSONWithStatus('/api/report-commits/', subversionCommits);
             }).then(() => {
-                return remote.getJSON('/api/commits/WebKit/?from=210948&to=210949');
+                return remote.getJSON('/api/commits/WebKit/?precedingRevision=210947&lastRevision=210949');
             }).then((result) => {
                 assert.equal(result['status'], 'OK');
                 assert.deepEqual(result['commits'].length, 2);
index 8425ab0..46210dd 100644 (file)
@@ -93,24 +93,18 @@ describe('CommitLog', function () {
     describe('diff', function () {
         it('should use label() as the label the previous commit is missing', function () {
             assert.deepEqual(webkitCommit().diff(), {
-                from: null,
-                to: '200805',
                 label: 'r200805',
                 url: 'http://trac.webkit.org/changeset/200805',
                 repository: MockModels.webkit
             });
 
             assert.deepEqual(gitWebKitCommit().diff(), {
-                from: null,
-                to: '6f8b0dbbda95a440503b88db1dd03dad3a7b07fb',
                 label: '6f8b0dbb',
                 url: 'http://trac.webkit.org/changeset/6f8b0dbbda95a440503b88db1dd03dad3a7b07fb',
                 repository: MockModels.webkit,
             });
 
             assert.deepEqual(osxCommit().diff(), {
-                from: null,
-                to: '10.11.4 15E65',
                 label: '10.11.4 15E65',
                 url: '',
                 repository: MockModels.osx,
@@ -119,9 +113,7 @@ describe('CommitLog', function () {
 
         it('should use increment the old SVN revision by 1', function () {
             assert.deepEqual(webkitCommit().diff(oldWebKitCommit()), {
-                from: '200575',
-                to: '200805',
-                label: 'r200575-r200805',
+                label: 'r200574-r200805',
                 url: '',
                 repository: MockModels.webkit
             });
@@ -129,8 +121,6 @@ describe('CommitLog', function () {
 
         it('should truncate a Git hash at 8th character', function () {
             assert.deepEqual(gitWebKitCommit().diff(oldGitWebKitCommit()), {
-                from: 'ffda14e6db0746d10d0f050907e4a7325851e502',
-                to: '6f8b0dbbda95a440503b88db1dd03dad3a7b07fb',
                 label: 'ffda14e6..6f8b0dbb',
                 url: '',
                 repository: MockModels.webkit
@@ -139,8 +129,6 @@ describe('CommitLog', function () {
 
         it('should surround "-" with spaces', function () {
             assert.deepEqual(osxCommit().diff(oldOSXCommit()), {
-                from: '10.11.3 15D21',
-                to: '10.11.4 15E65',
                 label: '10.11.3 15D21 - 10.11.4 15E65',
                 url: '',
                 repository: MockModels.osx