[Extensions Click-to-Script] Bring up dialog on 'specific sites' selection

When the user selects "On specific sites" from the dropdown in the
chrome://extensions page, automatically bring up the dialog to add an
allowed site. This makes it clear to the user that they need to specify
a site for this setting to work, and also fixes an issue where
selecting "On specific sites" and then refreshing the page (or having
the data model otherwise update) reset the value to "on click".

Bug: 884222

Change-Id: Ie89757f7fba2409774e5fcc2f4a8028aff31f7ba
Reviewed-on: https://chromium-review.googlesource.com/1226017Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#592118}

chrome/browser/resources/md_extensions/runtime_host_permissions.html

@@ -105,7 +105,9 @@
       <extensions-runtime-hosts-dialog
           delegate="[[delegate]]" item-id="[[itemId]]"
           current-site="[[hostDialogModel_]]"
-          on-close="onHostDialogClose_">
+          update-host-access="[[dialogShouldUpdateHostAccess_(oldHostAccess_)]]"
+          on-close="onHostDialogClose_"
+          on-cancel="onHostDialogCancel_">
       </extensions-runtime-hosts-dialog>
     </template>
   </template>

chrome/browser/resources/md_extensions/runtime_host_permissions.js

@@ -70,6 +70,17 @@ cr.define('extensions', function() {
         value: null,
       },
 
+      /**
+       * The old host access setting; used when we don't immediately commit the
+       * change to host access so that we can reset it if the user cancels.
+       * @type {?string}
+       * @private
+       */
+      oldHostAccess_: {
+        type: String,
+        value: null,
+      },
+
       /**
        * Proxying the enum to be used easily by the html template.
        * @private
@@ -88,12 +99,23 @@ cr.define('extensions', function() {
       const select = /** @type {!HTMLSelectElement} */ (event.target);
       const access =
           /** @type {chrome.developerPrivate.HostAccess} */ (select.value);
-      this.delegate.setItemHostAccess(this.itemId, access);
-      // Force the UI to update (in order to potentially hide or show the
-      // specific runtime hosts).
-      // TODO(devlin): Perhaps this should be handled by the backend updating
-      // and sending an onItemStateChanged event?
-      this.set('permissions.hostAccess', access);
+
+      if (access == chrome.developerPrivate.HostAccess.ON_SPECIFIC_SITES &&
+          (!this.permissions.runtimeHostPermissions ||
+           this.permissions.runtimeHostPermissions.length == 0)) {
+        // If the user is transitioning to the "on specific sites" option, show
+        // the "add host" dialog. This serves two purposes:
+        // - The user is prompted to add a host immediately, since otherwise
+        //   "on specific sites" is meaningless, and
+        // - The way the C++ code differentiates between "on click" and "on
+        //   specific sites" is by checking if there are any specific sites.
+        //   This ensures there will be at least one, so that the host access
+        //   is properly calculated.
+        this.oldHostAccess_ = assert(this.permissions.hostAccess);
+        this.doShowHostDialog_(select, null);
+      } else {
+        this.delegate.setItemHostAccess(this.itemId, access);
+      }
     },
 
     /**
@@ -137,6 +159,25 @@ cr.define('extensions', function() {
       this.hostDialogAnchorElement_ = null;
     },
 
+    /** @private */
+    onHostDialogCancel_: function() {
+      // The user canceled the dialog. Set host-access back to the old value,
+      // if the dialog was shown when just transitioning to a new state.
+      if (this.oldHostAccess_) {
+        assert(this.permissions.hostAccess == this.oldHostAccess_);
+        this.$['host-access'].value = this.oldHostAccess_;
+        this.oldHostAccess_ = null;
+      }
+    },
+
+    /**
+     * @return {boolean}
+     * @private
+     */
+    dialogShouldUpdateHostAccess_: function() {
+      return !!this.oldHostAccess_;
+    },
+
     /**
      * @param {!{
      *   model: !{item: string},

chrome/browser/resources/md_extensions/runtime_hosts_dialog.js

@@ -58,6 +58,15 @@ cr.define('extensions', function() {
         value: null,
       },
 
+      /**
+       * Whether the dialog should update the host access to be "on specific
+       * sites" before adding a new host permission.
+       */
+      updateHostAccess: {
+        type: Boolean,
+        value: false,
+      },
+
       /**
        * The site to add an exception for.
        * @private
@@ -83,6 +92,11 @@ cr.define('extensions', function() {
       this.$.dialog.showModal();
     },
 
+    /** @return {boolean} */
+    isOpen: function() {
+      return this.$.dialog.open;
+    },
+
     /**
      * Validates that the pattern entered is valid.
      * @private
@@ -138,24 +152,52 @@ cr.define('extensions', function() {
      * @private
      */
     onSubmitTap_: function() {
-      if (this.currentSite !== null) {
-        // No change in values, so no need to update the delegate.
-        if (this.currentSite == this.site_) {
-          this.$.dialog.close();
-          return;
-        }
-
-        // Changing the entry is done through a remove followed by an add.
-        this.delegate.removeRuntimeHostPermission(this.itemId, this.currentSite)
-            .then(() => {
-              this.addPermission_();
-            });
-        return;
+      if (this.currentSite !== null)
+        this.handleEdit_();
+      else
+        this.handleAdd_();
+    },
+
+    /**
+     * Handles adding a new site entry.
+     * @private
+     */
+    handleAdd_: function() {
+      assert(!this.currentSite);
+
+      if (this.updateHostAccess) {
+        this.delegate.setItemHostAccess(
+            this.itemId, chrome.developerPrivate.HostAccess.ON_SPECIFIC_SITES);
       }
 
       this.addPermission_();
     },
 
+    /**
+     * Handles editing an existing site entry.
+     * @private
+     */
+    handleEdit_: function() {
+      assert(this.currentSite);
+      assert(
+          !this.updateHostAccess,
+          'Editing host permissions should only be possible if the host ' +
+              'access is already set to specific sites.');
+
+      if (this.currentSite == this.site_) {
+        // No change in values, so no need to update anything.
+        this.$.dialog.close();
+        return;
+      }
+
+      // Editing an existing entry is done by removing the current site entry,
+      // and then adding the new one.
+      this.delegate.removeRuntimeHostPermission(this.itemId, this.currentSite)
+          .then(() => {
+            this.addPermission_();
+          });
+    },
+
     /**
      * Adds the runtime host permission through the delegate. If successful,
      * closes the dialog; otherwise displays the invalid input message.

chrome/test/data/webui/extensions/runtime_host_permissions_test.js

@@ -90,15 +90,84 @@ suite('RuntimeHostPermissions', function() {
     }
 
     // Check that selecting different values correctly notifies the delegate.
-    return expectDelegateCallOnAccessChange(HostAccess.ON_SPECIFIC_SITES)
-        .then(() => {
-          return expectDelegateCallOnAccessChange(HostAccess.ON_ALL_SITES);
-        })
+    return expectDelegateCallOnAccessChange(HostAccess.ON_ALL_SITES)
         .then(() => {
           return expectDelegateCallOnAccessChange(HostAccess.ON_CLICK);
         });
   });
 
+  test('on select sites cancel', function() {
+    const permissions = {
+      simplePermissions: ['permission 1', 'permission 2'],
+      hostAccess: HostAccess.ON_CLICK,
+      runtimeHostPermissions: [],
+    };
+
+    element.permissions = permissions;
+    Polymer.dom.flush();
+
+    const selectHostAccess = element.$$('#host-access');
+    assertTrue(!!selectHostAccess);
+
+    selectHostAccess.value = HostAccess.ON_SPECIFIC_SITES;
+    selectHostAccess.dispatchEvent(new CustomEvent('change'));
+
+    Polymer.dom.flush();
+    const dialog = element.$$('extensions-runtime-hosts-dialog');
+    assertTrue(!!dialog);
+
+    expectTrue(dialog.updateHostAccess);
+
+    // Canceling the dialog should reset the selectHostAccess value to ON_CLICK,
+    // since no host was added.
+    assertTrue(dialog.isOpen());
+    let whenClosed = test_util.eventToPromise('close', dialog);
+    dialog.$$('.cancel-button').click();
+    return whenClosed.then(() => {
+      Polymer.dom.flush();
+      expectEquals(HostAccess.ON_CLICK, selectHostAccess.value);
+    });
+  });
+
+  test('on select sites accept', function() {
+    const permissions = {
+      simplePermissions: ['permission 1', 'permission 2'],
+      hostAccess: HostAccess.ON_CLICK,
+      runtimeHostPermissions: [],
+    };
+
+    element.set('permissions', permissions);
+    Polymer.dom.flush();
+
+    const selectHostAccess = element.$$('#host-access');
+    assertTrue(!!selectHostAccess);
+
+    selectHostAccess.value = HostAccess.ON_SPECIFIC_SITES;
+    selectHostAccess.dispatchEvent(
+        new CustomEvent('change', {target: selectHostAccess}));
+
+    Polymer.dom.flush();
+    const dialog = element.$$('extensions-runtime-hosts-dialog');
+    assertTrue(!!dialog);
+
+    expectTrue(dialog.updateHostAccess);
+
+    // Make the add button clickable by entering valid input.
+    const input = dialog.$$('cr-input');
+    input.value = 'https://example.com';
+    input.fire('input');
+
+    // Closing the dialog (as opposed to canceling) should keep the
+    // selectHostAccess value at ON_SPECIFIC_SITES.
+    assertTrue(dialog.isOpen());
+    let whenClosed = test_util.eventToPromise('close', dialog);
+    dialog.$$('.action-button').click();
+    return whenClosed.then(() => {
+      Polymer.dom.flush();
+      expectEquals(HostAccess.ON_SPECIFIC_SITES, selectHostAccess.value);
+    });
+  });
+
   test('clicking add host triggers dialog', function() {
     const permissions = {
       simplePermissions: [],
@@ -119,6 +188,7 @@ suite('RuntimeHostPermissions', function() {
     assertTrue(!!dialog);
     expectTrue(dialog.$.dialog.open);
     expectEquals(null, dialog.currentSite);
+    expectFalse(dialog.updateHostAccess);
   });
 
   test('removing runtime host permissions', function() {
@@ -169,6 +239,7 @@ suite('RuntimeHostPermissions', function() {
     const dialog = element.$$('extensions-runtime-hosts-dialog');
     assertTrue(!!dialog);
     expectTrue(dialog.$.dialog.open);
+    expectFalse(dialog.updateHostAccess);
     expectEquals('https://example.com', dialog.currentSite);
   });
 });

chrome/test/data/webui/extensions/runtime_hosts_dialog_test.js

@@ -122,4 +122,24 @@ suite('RuntimeHostsDialog', function() {
         'https://example.com:80/*',
         extensions.getPatternFromSite('https://example.com:80/*'));
   });
+
+  test('update site access', function() {
+    dialog.updateHostAccess = true;
+    const input = dialog.$$('cr-input');
+    const site = 'http://www.example.com';
+    input.value = site;
+    input.fire('input');
+    assertFalse(input.invalid);
+
+    const submit = dialog.$.submit;
+    assertFalse(submit.disabled);
+    submit.click();
+    return delegate.whenCalled('setItemHostAccess').then((args) => {
+      let id = args[0];
+      let access = args[1];
+      assertEquals(ITEM_ID, id);
+      assertEquals(
+          chrome.developerPrivate.HostAccess.ON_SPECIFIC_SITES, access);
+    });
+  });
 });