Merge changes Ibf6d63ab,I0986d539

* changes:
  Fix plugin loading in browsers that use polyfill for HTML imports
  Wait for plugins to be loaded before showing change actions

polygerrit-ui/app/elements/admin/gr-admin-view/gr-admin-view_test.html

@@ -37,7 +37,7 @@ limitations under the License.
     let element;
     let sandbox;
 
-    setup(() => {
+    setup(done => {
       sandbox = sinon.sandbox.create();
       element = fixture('basic');
       stub('gr-rest-api-interface', {
@@ -45,7 +45,9 @@ limitations under the License.
           return Promise.resolve({});
         },
       });
-      sandbox.stub(Gerrit, 'awaitPluginsLoaded').returns(Promise.resolve());
+      const pluginsLoaded = Promise.resolve();
+      sandbox.stub(Gerrit, 'awaitPluginsLoaded').returns(pluginsLoaded);
+      pluginsLoaded.then(() => flush(done));
     });
 
     teardown(() => {
@@ -557,6 +559,7 @@ limitations under the License.
               }));
           sandbox.stub(element.$.restAPI, 'getIsGroupOwner')
               .returns(Promise.resolve(true));
+          return element.reload();
         });
 
         test('group list', () => {

polygerrit-ui/app/elements/change/gr-change-actions/gr-change-actions.js

@@ -385,7 +385,7 @@
 
     ready() {
       this.$.jsAPI.addElement(this.$.jsAPI.Element.CHANGE_ACTIONS, this);
-      this._loading = false;
+      this._handleLoadingComplete();
     },
 
     reload() {
@@ -398,7 +398,7 @@
         if (!revisionActions) { return; }
 
         this.revisionActions = revisionActions;
-        this._loading = false;
+        this._handleLoadingComplete();
       }).catch(err => {
         this.fire('show-alert', {message: ERR_REVISION_ACTIONS});
         this._loading = false;
@@ -406,6 +406,10 @@
       });
     },
 
+    _handleLoadingComplete() {
+      Gerrit.awaitPluginsLoaded().then(() => this._loading = false);
+    },
+
     _changeChanged() {
       this.reload();
     },

polygerrit-ui/app/elements/change/gr-change-metadata/gr-change-metadata-it_test.html

@@ -89,8 +89,6 @@ limitations under the License.
     });
 
     teardown(() => {
-      Gerrit._pluginsPending = -1;
-      Gerrit._allPluginsPromise = undefined;
       sandbox.restore();
     });
 
@@ -109,6 +107,7 @@ limitations under the License.
 
     suite('with plugin style', () => {
       setup(done => {
+        Gerrit._resetPlugins();
         const pluginHost = fixture('plugin-host');
         pluginHost.config = {
           plugin: {
@@ -144,7 +143,7 @@ limitations under the License.
             new URL('test/plugin.html?' + Math.random(),
                     window.location.href).toString());
         sandbox.stub(Gerrit, '_arePluginsLoaded').returns(true);
-        Gerrit._resolveAllPluginsLoaded();
+        Gerrit._setPluginsPending([]);
         element = createElement();
         sandbox.stub(element, '_computeCanDeleteVote').returns(true);
 

polygerrit-ui/app/elements/change/gr-reply-dialog/gr-reply-dialog-it_test.html

@@ -110,8 +110,6 @@ limitations under the License.
     });
 
     teardown(() => {
-      Gerrit._pluginsPending = -1;
-      Gerrit._allPluginsPromise = undefined;
       sandbox.restore();
     });
 
@@ -132,12 +130,14 @@ limitations under the License.
     });
 
     test('lgtm plugin', done => {
+      Gerrit._resetPlugins();
       const pluginHost = fixture('plugin-host');
       pluginHost.config = {
         plugin: {
           js_resource_paths: [],
           html_resource_paths: [
-            new URL('test/plugin.html', window.location.href).toString(),
+            new URL('test/plugin.html?' + Math.random(),
+                window.location.href).toString(),
           ],
         },
       };

polygerrit-ui/app/elements/change/gr-reply-dialog/test/plugin.html

@@ -28,6 +28,6 @@ limitations under the License.
           replyApi.setLabelValue(label, '+1');
         }
       });
-    }, '0.1', 'http://test.com/plugins/testplugin/static/test.js');
+    });
   </script>
 </dom-module>

polygerrit-ui/app/elements/plugins/gr-plugin-host/gr-plugin-host.js

@@ -34,11 +34,13 @@
     _configChanged(config) {
       const plugins = config.plugin;
       const htmlPlugins = plugins.html_resource_paths || [];
-      const jsPlugins = this._handleMigrations(plugins.js_resource_paths || [],
+      const jsPlugins =
-          htmlPlugins);
+          this._handleMigrations(plugins.js_resource_paths || [], htmlPlugins);
       const defaultTheme = config.default_theme;
-      Gerrit._setPluginsCount(
+      const pluginsPending =
-          jsPlugins.length + htmlPlugins.length + (defaultTheme ? 1 : 0));
+          [].concat(jsPlugins, htmlPlugins, defaultTheme || []).map(
+              p => this._urlFor(p));
+      Gerrit._setPluginsPending(pluginsPending);
       if (defaultTheme) {
         // Make theme first to be first to load.
         // Load sync to work around rare theme loading race condition.
@@ -72,7 +74,9 @@
         // onload (second param) needs to be a function. When null or undefined
         // were passed, plugins were not loaded correctly.
         this.importHref(
-            this._urlFor(url), () => {}, Gerrit._pluginInstalled, async);
+            this._urlFor(url), () => {},
+            Gerrit._pluginInstallError.bind(null, `${url} import error`),
+            async);
       }
     },
 
@@ -86,18 +90,20 @@
       const el = document.createElement('script');
       el.defer = true;
       el.src = url;
-      el.onerror = Gerrit._pluginInstalled;
+      el.onerror = Gerrit._pluginInstallError.bind(null, `${url} load error`);
       return document.body.appendChild(el);
     },
 
     _urlFor(pathOrUrl) {
       if (pathOrUrl.startsWith('http')) {
+        // Plugins are loaded from another domain.
         return pathOrUrl;
       }
       if (!pathOrUrl.startsWith('/')) {
         pathOrUrl = '/' + pathOrUrl;
       }
-      return this.getBaseUrl() + pathOrUrl;
+      const {href, pathname} = window.location;
+      return href.split(pathname)[0] + this.getBaseUrl() + pathOrUrl;
     },
   });
 })();

polygerrit-ui/app/elements/plugins/gr-plugin-host/gr-plugin-host_test.html

@@ -36,12 +36,14 @@ limitations under the License.
   suite('gr-diff tests', () => {
     let element;
     let sandbox;
+    let url;
 
     setup(() => {
       element = fixture('basic');
       sandbox = sinon.sandbox.create();
       sandbox.stub(document.body, 'appendChild');
       sandbox.stub(element, 'importHref');
+      url = window.location.href.split(window.location.pathname)[0];
     });
 
     teardown(() => {
@@ -60,36 +62,43 @@ limitations under the License.
     });
 
     test('imports relative html plugins from config', () => {
+      sandbox.stub(Gerrit, '_pluginInstallError');
       element.config = {
         plugin: {html_resource_paths: ['foo/bar', 'baz']},
       };
-      assert.equal(element.importHref.firstCall.args[0], '/foo/bar');
+      assert.equal(element.importHref.firstCall.args[0], url + '/foo/bar');
-      assert.equal(element.importHref.firstCall.args[2],
-          Gerrit._pluginInstalled);
       assert.isTrue(element.importHref.firstCall.args[3]);
 
-      assert.equal(element.importHref.secondCall.args[0], '/baz');
+      assert.equal(element.importHref.secondCall.args[0], url + '/baz');
-      assert.equal(element.importHref.secondCall.args[2],
-          Gerrit._pluginInstalled);
       assert.isTrue(element.importHref.secondCall.args[3]);
+
+      assert.equal(Gerrit._pluginInstallError.callCount, 0);
+      element.importHref.firstCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 1);
+      element.importHref.secondCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 2);
     });
 
     test('imports relative html plugins from config with a base url', () => {
+      sandbox.stub(Gerrit, '_pluginInstallError');
       sandbox.stub(element, 'getBaseUrl').returns('/the-base');
       element.config = {
         plugin: {html_resource_paths: ['foo/bar', 'baz']}};
-      assert.equal(element.importHref.firstCall.args[0], '/the-base/foo/bar');
+      assert.equal(element.importHref.firstCall.args[0],
-      assert.equal(element.importHref.firstCall.args[2],
+          url + '/the-base/foo/bar');
-          Gerrit._pluginInstalled);
       assert.isTrue(element.importHref.firstCall.args[3]);
 
-      assert.equal(element.importHref.secondCall.args[0], '/the-base/baz');
+      assert.equal(element.importHref.secondCall.args[0],
-      assert.equal(element.importHref.secondCall.args[2],
+          url + '/the-base/baz');
-          Gerrit._pluginInstalled);
       assert.isTrue(element.importHref.secondCall.args[3]);
+      assert.equal(Gerrit._pluginInstallError.callCount, 0);
+      element.importHref.firstCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 1);
+      element.importHref.secondCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 2);
     });
 
-    test('inportHref is not called with null callback functions', () => {
+    test('importHref is not called with null callback functions', () => {
       const plugins = ['path/to/plugin'];
       element._importHtmlPlugins(plugins);
       assert.isTrue(element.importHref.calledOnce);
@@ -98,6 +107,7 @@ limitations under the License.
     });
 
     test('imports absolute html plugins from config', () => {
+      sandbox.stub(Gerrit, '_pluginInstallError');
       element.config = {
         plugin: {
           html_resource_paths: [
@@ -108,15 +118,16 @@ limitations under the License.
       };
       assert.equal(element.importHref.firstCall.args[0],
           'http://example.com/foo/bar');
-      assert.equal(element.importHref.firstCall.args[2],
-          Gerrit._pluginInstalled);
       assert.isTrue(element.importHref.firstCall.args[3]);
 
       assert.equal(element.importHref.secondCall.args[0],
           'https://example.com/baz');
-      assert.equal(element.importHref.secondCall.args[2],
-          Gerrit._pluginInstalled);
       assert.isTrue(element.importHref.secondCall.args[3]);
+      assert.equal(Gerrit._pluginInstallError.callCount, 0);
+      element.importHref.firstCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 1);
+      element.importHref.secondCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 2);
     });
 
     test('adds js plugins from config to the body', () => {
@@ -127,16 +138,18 @@ limitations under the License.
     test('imports relative js plugins from config', () => {
       sandbox.stub(element, '_createScriptTag');
       element.config = {plugin: {js_resource_paths: ['foo/bar', 'baz']}};
-      assert.isTrue(element._createScriptTag.calledWith('/foo/bar'));
+      assert.isTrue(element._createScriptTag.calledWith(url + '/foo/bar'));
-      assert.isTrue(element._createScriptTag.calledWith('/baz'));
+      assert.isTrue(element._createScriptTag.calledWith(url + '/baz'));
     });
 
     test('imports relative html plugins from config with a base url', () => {
       sandbox.stub(element, '_createScriptTag');
       sandbox.stub(element, 'getBaseUrl').returns('/the-base');
       element.config = {plugin: {js_resource_paths: ['foo/bar', 'baz']}};
-      assert.isTrue(element._createScriptTag.calledWith('/the-base/foo/bar'));
+      assert.isTrue(element._createScriptTag.calledWith(
-      assert.isTrue(element._createScriptTag.calledWith('/the-base/baz'));
+          url + '/the-base/foo/bar'));
+      assert.isTrue(element._createScriptTag.calledWith(
+          url + '/the-base/baz'));
     });
 
     test('imports absolute html plugins from config', () => {
@@ -156,21 +169,23 @@ limitations under the License.
     });
 
     test('default theme is loaded with html plugins', () => {
+      sandbox.stub(Gerrit, '_pluginInstallError');
       element.config = {
         default_theme: '/oof',
         plugin: {
           html_resource_paths: ['some'],
         },
       };
-      assert.equal(element.importHref.firstCall.args[0], '/oof');
+      assert.equal(element.importHref.firstCall.args[0], url + '/oof');
-      assert.equal(element.importHref.firstCall.args[2],
-          Gerrit._pluginInstalled);
       assert.isFalse(element.importHref.firstCall.args[3]);
 
-      assert.equal(element.importHref.secondCall.args[0], '/some');
+      assert.equal(element.importHref.secondCall.args[0], url + '/some');
-      assert.equal(element.importHref.secondCall.args[2],
-          Gerrit._pluginInstalled);
       assert.isTrue(element.importHref.secondCall.args[3]);
+      assert.equal(Gerrit._pluginInstallError.callCount, 0);
+      element.importHref.firstCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 1);
+      element.importHref.secondCall.args[2]();
+      assert.equal(Gerrit._pluginInstallError.callCount, 2);
     });
   });
 </script>

polygerrit-ui/app/elements/plugins/gr-theme-api/gr-theme-api_test.html

@@ -65,7 +65,7 @@ limitations under the License.
         stub('gr-custom-plugin-header', {
           ready() { customHeader = this; },
         });
-        Gerrit._resolveAllPluginsLoaded();
+        Gerrit._setPluginsPending([]);
       });
 
       test('sets logo and title', done => {

polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-change-reply-js-api.js

@@ -82,6 +82,7 @@
         if (!el.content) { return; }
 
         el.content.addEventListener('labels-changed', e => {
+          console.log('labels-changed', e.detail);
           handler(e.detail);
         });
       });

polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface_test.html

@@ -60,9 +60,9 @@ limitations under the License.
       });
       element = fixture('basic');
       errorStub = sandbox.stub(console, 'error');
-      Gerrit._setPluginsCount(1);
       Gerrit.install(p => { plugin = p; }, '0.1',
           'http://test.com/plugins/testplugin/static/test.js');
+      Gerrit._setPluginsPending([]);
     });
 
     teardown(() => {
@@ -306,7 +306,6 @@ limitations under the License.
     test('_setPluginsCount', done => {
       stub('gr-reporting', {
         pluginsLoaded() {
-          assert.equal(Gerrit._pluginsPending, 0);
           done();
         },
       });
@@ -324,13 +323,11 @@ limitations under the License.
     test('_pluginInstalled', done => {
       stub('gr-reporting', {
         pluginsLoaded() {
-          assert.equal(Gerrit._pluginsPending, 0);
           done();
         },
       });
       Gerrit._setPluginsCount(2);
       Gerrit._pluginInstalled();
-      assert.equal(Gerrit._pluginsPending, 1);
       Gerrit._pluginInstalled();
     });
 
@@ -348,10 +345,10 @@ limitations under the License.
       assert.isTrue(Gerrit._pluginInstalled.calledOnce);
     });
 
-    test('install calls _pluginInstalled on error', () => {
+    test('plugin install errors mark plugins as loaded', () => {
-      sandbox.stub(Gerrit, '_pluginInstalled');
+      Gerrit._setPluginsCount(1);
       Gerrit.install(() => {}, '0.0pre-alpha');
-      assert.isTrue(Gerrit._pluginInstalled.calledOnce);
+      return Gerrit.awaitPluginsLoaded();
     });
 
     test('installGwt calls _pluginInstalled', () => {

polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-public-js-api.js

@@ -20,13 +20,22 @@
   /**
    * Hash of loaded and installed plugins, name to Plugin object.
    */
-  const plugins = {};
+  const _plugins = {};
+
+  /**
+   * Array of plugin URLs to be loaded, name to url.
+   */
+  let _pluginsPending = {};
+
+  let _pluginsPendingCount = -1;
 
   const PANEL_ENDPOINTS_MAPPING = {
     CHANGE_SCREEN_BELOW_COMMIT_INFO_BLOCK: 'change-view-integration',
     CHANGE_SCREEN_BELOW_CHANGE_INFO_BLOCK: 'change-metadata-item',
   };
 
+  const PLUGIN_LOADING_TIMEOUT_MS = 10000;
+
   let _restAPI;
   const getRestAPI = () => {
     if (!_restAPI) {
@@ -80,6 +89,14 @@
   window.$wnd = window;
 
   function getPluginNameFromUrl(url) {
+    if (!(url instanceof URL)) {
+      try {
+        url = new URL(url);
+      } catch (e) {
+        console.warn(e);
+        return null;
+      }
+    }
     const base = Gerrit.BaseUrlBehavior.getBaseUrl();
     const pathname = url.pathname.replace(base, '');
     // Site theme is server from predefined path.
@@ -393,22 +410,26 @@
 
   const Gerrit = window.Gerrit || {};
 
+  let _resolveAllPluginsLoaded = null;
+  let _allPluginsPromise = null;
+
+  Gerrit._endpoints = new GrPluginEndpoints();
+
   // Provide reset plugins function to clear installed plugins between tests.
   const app = document.querySelector('#app');
   if (!app) {
     // No gr-app found (running tests)
     Gerrit._resetPlugins = () => {
-      for (const k of Object.keys(plugins)) {
+      _resolveAllPluginsLoaded = null;
-        delete plugins[k];
+      _allPluginsPromise = null;
+      Gerrit._setPluginsPending([]);
+      Gerrit._endpoints = new GrPluginEndpoints();
+      for (const k of Object.keys(_plugins)) {
+        delete _plugins[k];
       }
     };
   }
 
-  // Number of plugins to initialize, -1 means 'not yet known'.
-  Gerrit._pluginsPending = -1;
-
-  Gerrit._endpoints = new GrPluginEndpoints();
-
   Gerrit.getPluginName = function() {
     console.warn('Gerrit.getPluginName is not supported in PolyGerrit.',
         'Please use plugin.getPluginName() instead.');
@@ -428,32 +449,31 @@
   };
 
   Gerrit.install = function(callback, opt_version, opt_src) {
+    // HTML import polyfill adds __importElement pointing to the import tag.
+    const script = document.currentScript &&
+        (document.currentScript.__importElement || document.currentScript);
+    const src = opt_src || (script && (script.src || script.baseURI));
+    const name = getPluginNameFromUrl(src);
+
     if (opt_version && opt_version !== API_VERSION) {
-      console.warn('Only version ' + API_VERSION +
+      Gerrit._pluginInstallError(`Plugin ${name} install error: only version ` +
-          ' is supported in PolyGerrit. ' + opt_version + ' was given.');
+          API_VERSION + ' is supported in PolyGerrit. ' + opt_version +
-      Gerrit._pluginInstalled();
+          ' was given.');
       return;
     }
 
-    const src = opt_src || (document.currentScript &&
+    const existingPlugin = _plugins[name];
-         (document.currentScript.src || document.currentScript.baseURI));
-    const name = getPluginNameFromUrl(new URL(src));
-    const existingPlugin = plugins[name];
     const plugin = existingPlugin || new Plugin(src);
     try {
       callback(plugin);
-      plugins[name] = plugin;
+      if (name) {
-    } catch (e) {
+        _plugins[name] = plugin;
-      console.warn(`${name} install failed: ${e.name}: ${e.message}`);
       }
-    // Don't double count plugins that may have an html and js install.
+      if (!existingPlugin) {
-    // TODO(beckysiegel) remove name check once name issue is resolved.
+        Gerrit._pluginInstalled(src);
-    // If there isn't a name, it's due to an issue with the polyfill for
+      }
-    // html imports in Safari/Firefox. In this case, other plugin related
+    } catch (e) {
-    // features may still be broken, but still make sure to call.
+      Gerrit._pluginInstallError(`${e.name}: ${e.message}`);
-    // _pluginInstalled.
-    if (!name || !existingPlugin) {
-      Gerrit._pluginInstalled();
     }
   };
 
@@ -502,50 +522,85 @@
    * @deprecated best effort support, will be removed with GWT UI.
    */
   Gerrit.installGwt = function(url) {
-    Gerrit._pluginInstalled();
+    const name = getPluginNameFromUrl(url);
-    const name = getPluginNameFromUrl(new URL(url));
     let plugin;
     try {
-      plugin = plugins[name] || new Plugin(url);
+      plugin = _plugins[name] || new Plugin(url);
       plugin.deprecated.install();
+      Gerrit._pluginInstalled(url);
     } catch (e) {
-      console.warn(`${name} install failed: ${e.name}: ${e.message}`);
+      Gerrit._pluginInstallError(`${e.name}: ${e.message}`);
     }
     return plugin;
   };
 
-  Gerrit._allPluginsPromise = null;
-  Gerrit._resolveAllPluginsLoaded = null;
-
   Gerrit.awaitPluginsLoaded = function() {
-    if (!Gerrit._allPluginsPromise) {
+    if (!_allPluginsPromise) {
       if (Gerrit._arePluginsLoaded()) {
-        Gerrit._allPluginsPromise = Promise.resolve();
+        _allPluginsPromise = Promise.resolve();
       } else {
-        Gerrit._allPluginsPromise = new Promise(resolve => {
+        let timeoutId;
-          Gerrit._resolveAllPluginsLoaded = resolve;
+        _allPluginsPromise =
-        });
+          Promise.race([
+            new Promise(resolve => _resolveAllPluginsLoaded = resolve),
+            new Promise(resolve => timeoutId = setTimeout(
+                Gerrit._pluginLoadingTimeout, PLUGIN_LOADING_TIMEOUT_MS)),
+          ]).then(() => clearTimeout(timeoutId));
       }
     }
-    return Gerrit._allPluginsPromise;
+    return _allPluginsPromise;
+  };
+
+  Gerrit._pluginLoadingTimeout = function() {
+    document.dispatchEvent(new CustomEvent('show-alert', {
+      detail: {
+        message: 'Plugins loading timeout. Check the console for errors.',
+      },
+    }));
+    console.error(`Failed to load plugins: ${Object.keys(_pluginsPending)}`);
+    Gerrit._setPluginsPending([]);
+  };
+
+  Gerrit._setPluginsPending = function(plugins) {
+    _pluginsPending = plugins.reduce((o, url) => {
+      o[getPluginNameFromUrl(url)] = url;
+      return o;
+    }, {});
+    Gerrit._setPluginsCount(plugins.length);
   };
 
   Gerrit._setPluginsCount = function(count) {
-    Gerrit._pluginsPending = count;
+    _pluginsPendingCount = count;
     if (Gerrit._arePluginsLoaded()) {
       document.createElement('gr-reporting').pluginsLoaded();
-      if (Gerrit._resolveAllPluginsLoaded) {
+      if (_resolveAllPluginsLoaded) {
-        Gerrit._resolveAllPluginsLoaded();
+        _resolveAllPluginsLoaded();
       }
     }
   };
 
-  Gerrit._pluginInstalled = function() {
+  Gerrit._pluginInstallError = function(message) {
-    Gerrit._setPluginsCount(Gerrit._pluginsPending - 1);
+    console.log(`Plugin install error: ${message}`);
+    Gerrit._setPluginsCount(_pluginsPendingCount - 1);
+  };
+
+  Gerrit._pluginInstalled = function(url) {
+    const name = getPluginNameFromUrl(url);
+    if (name && !_pluginsPending[name]) {
+      console.warn(`Unexpected plugin from ${url}!`);
+    } else {
+      if (name) {
+        delete _pluginsPending[name];
+        console.log(`Plugin ${name} installed`);
+      } else {
+        console.log(`Plugin installed from ${url}`);
+      }
+      Gerrit._setPluginsCount(_pluginsPendingCount - 1);
+    }
   };
 
   Gerrit._arePluginsLoaded = function() {
-    return Gerrit._pluginsPending === 0;
+    return _pluginsPendingCount === 0;
   };
 
   Gerrit._getPluginScreenName = function(pluginName, screenName) {

polygerrit-ui/app/test/common-test-setup.html

@@ -49,14 +49,9 @@ limitations under the License.
   (function() {
     setup(() => {
       if (!window.Gerrit) { return; }
-      Gerrit._pluginsPending = -1;
-      Gerrit._allPluginsPromise = undefined;
       if (Gerrit._resetPlugins) {
         Gerrit._resetPlugins();
       }
-      if (Gerrit._endpoints) {
-        Gerrit._endpoints = new GrPluginEndpoints();
-      }
     });
   })();
 </script>