From 61bcdbc3db34b413ac38a000d192c3b27cd08e63 Mon Sep 17 00:00:00 2001
From: Andrew Bonventre <andybons@chromium.org>
Date: Tue, 3 May 2016 18:11:00 -0400
Subject: [PATCH] gr-ajax cleanup (gr-change-list-view)

Bug: Issue 3988
Change-Id: I1f312eba0e561f0c04296f42e8f8da5779077402
---
 .../gr-change-list-view.html                  | 32 +++---------
 .../gr-change-list-view.js                    | 41 +++++++--------
 .../diff/gr-diff-view/gr-diff-view.js         | 24 +--------
 polygerrit-ui/app/elements/gr-app.html        |  1 -
 polygerrit-ui/app/elements/gr-app.js          | 21 ++------
 .../gr-rest-api-interface.js                  | 50 ++++++++++++++++++-
 6 files changed, 78 insertions(+), 91 deletions(-)

diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
index f71d0f7d4a..1d3968f0a8 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.html
@@ -15,8 +15,7 @@ limitations under the License.
 -->
 
 <link rel="import" href="../../../bower_components/polymer/polymer.html">
-<link rel="import" href="../../../behaviors/rest-client-behavior.html">
-<link rel="import" href="../../shared/gr-ajax/gr-ajax.html">
+<link rel="import" href="../../shared/gr-rest-api-interface/gr-rest-api-interface.html">
 <link rel="import" href="../gr-change-list/gr-change-list.html">
 
 <dom-module id="gr-change-list-view">
@@ -26,15 +25,9 @@ limitations under the License.
         background-color: var(--view-background-color);
         display: block;
       }
-      .loading,
-      .error {
-        padding: 1em var(--default-horizontal-margin);
-      }
       .loading {
         color: #666;
-      }
-      .error {
-        color: #D32F2F;
+        padding: 1em var(--default-horizontal-margin);
       }
       gr-change-list {
         width: 100%;
@@ -56,30 +49,21 @@ limitations under the License.
         }
       }
     </style>
-    <gr-ajax
-        auto
-        url="/changes/"
-        params="[[_computeQueryParams(_query, _offset, changesPerPage)]]"
-        last-response="{{_changes}}"
-        last-error="{{_lastError}}"
-        loading="{{_loading}}"></gr-ajax>
-    <div class="loading" hidden$="[[!_loading]]" hidden>Loading...</div>
-    <div class="error" hidden$="[[_computeErrorHidden(_loading, _lastError)]]" hidden>
-      [[_lastError.request.xhr.responseText]]
-    </div>
-    <div hidden$="[[_computeListHidden(_loading, _lastError)]]" hidden>
+    <div class="loading" hidden$="[[!_loading]]">Loading...</div>
+    <div hidden$="[[_loading]]">
       <gr-change-list
           changes="{{_changes}}"
           selected-index="{{viewState.selectedChangeIndex}}"
           show-star="[[loggedIn]]"></gr-change-list>
       <nav>
-        <a href$="[[_computeNavLink(_query, _offset, -1, changesPerPage)]]"
+        <a href$="[[_computeNavLink(_query, _offset, -1, _changesPerPage)]]"
            hidden$="[[_hidePrevArrow(_offset)]]">&larr; Prev</a>
-        <a href$="[[_computeNavLink(_query, _offset, 1, changesPerPage)]]"
-           hidden$="[[_hideNextArrow(_changes.length, changesPerPage)]]">
+        <a href$="[[_computeNavLink(_query, _offset, 1, _changesPerPage)]]"
+           hidden$="[[_hideNextArrow(_changes.length, _changesPerPage)]]">
           Next &rarr;</a>
       </nav>
     </div>
+    <gr-rest-api-interface id="restAPI"></gr-rest-api-interface>
   </template>
   <script src="gr-change-list-view.js"></script>
 </dom-module>
diff --git a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
index a694fc9256..79531ac7af 100644
--- a/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
+++ b/polygerrit-ui/app/elements/change-list/gr-change-list-view/gr-change-list-view.js
@@ -49,7 +49,7 @@
         value: function() { return {}; },
       },
 
-      changesPerPage: Number,
+      _changesPerPage: Number,
 
       /**
        * Currently active query.
@@ -66,11 +66,6 @@
        */
       _changes: Array,
 
-      /**
-       * Contains error of last request (in case of change loading error).
-       */
-      _lastError: Object,
-
       /**
        * For showing a "loading..." string during ajax requests.
        */
@@ -80,10 +75,6 @@
       },
     },
 
-    behaviors: [
-      Gerrit.RESTClientBehavior,
-    ],
-
     attached: function() {
       this.fire('title-change', {title: this._query});
     },
@@ -91,6 +82,7 @@
     _paramsChanged: function(value) {
       if (value.view != this.tagName.toLowerCase()) { return; }
 
+      this._loading = true;
       this._query = value.query;
       this._offset = value.offset || 0;
       if (this.viewState.query != this._query ||
@@ -101,22 +93,23 @@
       }
 
       this.fire('title-change', {title: this._query});
+
+      this._getPreferences().then(function(prefs) {
+        this._changesPerPage = prefs.changes_per_page;
+        return this._getChanges();
+      }.bind(this)).then(function(changes) {
+        this._changes = changes;
+        this._loading = false;
+      }.bind(this));
     },
 
-    _computeQueryParams: function(query, offset, changesPerPage) {
-      var options = this.listChangesOptionsToHex(
-          this.ListChangesOption.LABELS,
-          this.ListChangesOption.DETAILED_ACCOUNTS
-      );
-      var obj = {
-        n: changesPerPage,
-        O: options,
-        S: offset || 0,
-      };
-      if (query && query.length > 0) {
-        obj.q = query;
-      }
-      return obj;
+    _getChanges: function() {
+      return this.$.restAPI.getChanges(this._changesPerPage, this._query,
+          this._offset);
+    },
+
+    _getPreferences: function() {
+      return this.$.restAPI.getPreferences();
     },
 
     _computeNavLink: function(query, offset, direction, changesPerPage) {
diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
index e18d329f4e..85371f46b7 100644
--- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
+++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.js
@@ -121,29 +121,7 @@
     },
 
     _getDiffPreferences: function() {
-      return this._getLoggedIn().then(function(loggedIn) {
-        if (!loggedIn) {
-          // These defaults should match the defaults in
-          // gerrit-extension-api/src/main/jcg/gerrit/extensions/client/DiffPreferencesInfo.java
-          // NOTE: There are some settings that don't apply to PolyGerrit
-          // (Render mode being at least one of them).
-          return Promise.resolve({
-            auto_hide_diff_table_header: true,
-            context: 10,
-            cursor_blink_rate: 0,
-            ignore_whitespace: 'IGNORE_NONE',
-            intraline_difference: true,
-            line_length: 100,
-            show_line_endings: true,
-            show_tabs: true,
-            show_whitespace_errors: true,
-            syntax_highlighting: true,
-            tab_size: 8,
-            theme: 'DEFAULT',
-          });
-        }
-        return this.$.restAPI.getDiffPreferences();
-      }.bind(this));
+      return this.$.restAPI.getDiffPreferences();
     },
 
     _handleReviewedChange: function(e) {
diff --git a/polygerrit-ui/app/elements/gr-app.html b/polygerrit-ui/app/elements/gr-app.html
index 1272a45704..95b8ae60c1 100644
--- a/polygerrit-ui/app/elements/gr-app.html
+++ b/polygerrit-ui/app/elements/gr-app.html
@@ -89,7 +89,6 @@ limitations under the License.
         <gr-change-list-view
             params="[[params]]"
             view-state="{{_viewState.changeListView}}"
-            changes-per-page="[[_preferences.changes_per_page]]"
             logged-in="[[_computeLoggedIn(_account)]]"></gr-change-list-view>
       </template>
       <template is="dom-if" if="[[_showDashboardView]]" restamp="true">
diff --git a/polygerrit-ui/app/elements/gr-app.js b/polygerrit-ui/app/elements/gr-app.js
index 3495218110..be6142839b 100644
--- a/polygerrit-ui/app/elements/gr-app.js
+++ b/polygerrit-ui/app/elements/gr-app.js
@@ -30,7 +30,6 @@
       },
       _serverConfig: Object,
       _version: String,
-      _preferences: Object,
       _showChangeListView: Boolean,
       _showDashboardView: Boolean,
       _showChangeView: Boolean,
@@ -52,10 +51,6 @@
       Gerrit.KeyboardShortcutBehavior,
     ],
 
-    get loggedIn() {
-      return !!(this._account && Object.keys(this._account).length > 0);
-    },
-
     attached: function() {
       this.$.restAPI.getAccount().then(function(account) {
         this._account = account;
@@ -88,17 +83,9 @@
     },
 
     _accountChanged: function(account) {
-      if (this.loggedIn) {
-        this.$.restAPI.getPreferences().then(function(preferences) {
-          this._preferences = preferences;
-        }.bind(this));
-        // Diff preferences are cached; warm it before a diff is rendered.
-        this.$.restAPI.getDiffPreferences();
-      } else {
-        this._preferences = {
-          changes_per_page: 25,
-        };
-      }
+      // Preferences are cached when a user is logged in; warm them.
+      this.$.restAPI.getPreferences();
+      this.$.restAPI.getDiffPreferences();
     },
 
     _viewChanged: function(view) {
@@ -117,7 +104,7 @@
 
     // Argument used for binding update only.
     _computeLoggedIn: function(account) {
-      return this.loggedIn;
+      return !!(account && Object.keys(account).length > 0);
     },
 
     _handlePageError: function(e) {
diff --git a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
index 2f93a7cac5..7d2d0ac10b 100644
--- a/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
+++ b/polygerrit-ui/app/elements/shared/gr-rest-api-interface/gr-rest-api-interface.js
@@ -153,7 +153,29 @@
     },
 
     getDiffPreferences: function() {
-      return this._fetchSharedCacheURL('/accounts/self/preferences.diff');
+      return this.getLoggedIn().then(function(loggedIn) {
+        if (loggedIn) {
+          return this._fetchSharedCacheURL('/accounts/self/preferences.diff');
+        }
+        // These defaults should match the defaults in
+        // gerrit-extension-api/src/main/jcg/gerrit/extensions/client/DiffPreferencesInfo.java
+        // NOTE: There are some settings that don't apply to PolyGerrit
+        // (Render mode being at least one of them).
+        return Promise.resolve({
+          auto_hide_diff_table_header: true,
+          context: 10,
+          cursor_blink_rate: 0,
+          ignore_whitespace: 'IGNORE_NONE',
+          intraline_difference: true,
+          line_length: 100,
+          show_line_endings: true,
+          show_tabs: true,
+          show_whitespace_errors: true,
+          syntax_highlighting: true,
+          tab_size: 8,
+          theme: 'DEFAULT',
+        });
+      }.bind(this));
     },
 
     saveDiffPreferences: function(prefs, opt_errFn, opt_ctx) {
@@ -172,7 +194,15 @@
     },
 
     getPreferences: function() {
-      return this._fetchSharedCacheURL('/accounts/self/preferences');
+      return this.getLoggedIn().then(function(loggedIn) {
+        if (loggedIn) {
+          return this._fetchSharedCacheURL('/accounts/self/preferences');
+        }
+
+        return Promise.resolve({
+          changes_per_page: 25,
+        });
+      }.bind(this));
     },
 
     _fetchSharedCacheURL: function(url) {
@@ -197,6 +227,22 @@
       return this._sharedFetchPromises[url];
     },
 
+    getChanges: function(changesPerPage, opt_query, opt_offset) {
+      var options = this._listChangesOptionsToHex(
+          ListChangesOption.LABELS,
+          ListChangesOption.DETAILED_ACCOUNTS
+      );
+      var params = {
+        n: changesPerPage,
+        O: options,
+        S: opt_offset || 0,
+      };
+      if (opt_query && opt_query.length > 0) {
+        params.q = opt_query;
+      }
+      return this.fetchJSON('/changes/', null, null, params);
+    },
+
     getChangeActionURL: function(changeNum, opt_patchNum, endpoint) {
       return this._changeBaseURL(changeNum, opt_patchNum) + endpoint;
     },