Display extension options in a WebUI overlay instead of in a new tab

chrome/browser/resources/extensions/extension_options_overlay.js

Index: chrome/browser/resources/extensions/extension_options_overlay.js
diff --git a/chrome/browser/resources/extensions/extension_options_overlay.js b/chrome/browser/resources/extensions/extension_options_overlay.js
new file mode 100644
index 0000000000000000000000000000000000000000..92ab71810cbe0dd5ca5932e7d945a627c39f38a5
--- /dev/null
+++ b/chrome/browser/resources/extensions/extension_options_overlay.js
@@ -0,0 +1,83 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+cr.define('extensions', function() {
+  'use strict';
+
+  function ExtensionOptionsOverlay() {}

[not at google - send to devlin, 2014/08/14 15:33:20]

/**
 * Comment.
 * @constructor
 */

[Devlin, 2014/08/14 15:37:51]

docs

[ericzeng, 2014/08/14 18:11:54]

Done.

+
+  cr.addSingletonGetter(ExtensionOptionsOverlay);
+
+  ExtensionOptionsOverlay.prototype = {
+    /**
+     * The extension whose options are being displayed
+     * @type {??}

[not at google - send to devlin, 2014/08/14 15:33:20]

@type {string}

[Devlin, 2014/08/14 15:37:52]

type {??} is probably bad :P

[ericzeng, 2014/08/14 18:11:54]

Done.

+     * @private
+     */
+    extensionId_: undefined,

[not at google - send to devlin, 2014/08/14 15:33:20]

Weak nit: I prefer null over undefined when you're initializing class variables
(initializing a variable seems like defining it to me, so undefined looks
strange). However, I'm not up to speed on the idioms of this. Perhaps a WebUI
owner can comment.

[Devlin, 2014/08/14 15:40:36]

dbeam will be able to confirm, but I think I've been told undefined lately.

+
+    /**
+     * Initialize the page.
+     * @param {function(HTMLDivElement)} showOverlay The function to show or
+     *     hide the ExtensionOptionsOverlay; this should take a single parameter
+     *     which is either the overlay Div if the overlay should be displayed,
+     *     or null if the overlay should be hidden.
+     */
+    initializePage: function(showOverlay) {
+      console.log('initializePage');

[not at google - send to devlin, 2014/08/14 15:33:20]

Remove logging .: logging.

[Devlin, 2014/08/14 15:37:51]

leftover from testing?

[ericzeng, 2014/08/14 18:11:54]

Yeeeuuup

+      var overlay = $('overlay');
+      cr.ui.overlay.setupOverlay(overlay);
+      cr.ui.overlay.globalInitialization();
+      overlay.addEventListener('cancelOverlay', this.handleDismiss_.bind(this));
+
+      /**
+        * The element of the full overlay.
+        * @type {HTMLDivElement}
+        * @private
+        */
+      this.overlayDiv_ = $('extension-options-overlay');

[not at google - send to devlin, 2014/08/14 15:33:20]

Define this as a class property under |extensionId_|? Mainly so that you can put
the jsdoc on that, rather than here. I'm not sure the closure compiler can do as
much with it here.

[Devlin, 2014/08/14 15:40:36]

again, dbeam to confirm, but he's told me in the past to just initialize and doc
here.  (This may or may not have changed since we're actually gonna be using the
CC)

[ericzeng, 2014/08/14 18:11:54]

The extension error overlay initializes it here so I'm just going to leave it
until we get a definitive answer.

+
+      this.setVisible = function(isVisible) {

[Devlin, 2014/08/14 15:37:51]

docs

[ericzeng, 2014/08/14 18:11:54]

Done.

[Dan Beam, 2014/08/14 19:01:27]

move to prototype, make @private e.g.

  /**
   * @param {boolean} Whether something or other should be visible.
   * @private
   */
  setVisible_: function(isVisible) {
    showOverlay(isVisible ? this.overlayDiv_ : null);
  },

[ericzeng, 2014/08/14 22:34:13]

Done.

+        console.log('setVisible');

[Devlin, 2014/08/14 15:37:52]

leftover?

[ericzeng, 2014/08/14 18:11:54]

leftover.

+        showOverlay(isVisible ? this.overlayDiv_ : null);
+      };
+    },
+
+    handleDismiss_: function(event) {

[Devlin, 2014/08/14 15:37:52]

docs

[ericzeng, 2014/08/14 18:11:54]

Done.

+      this.setVisible(false);
+      if (!this.extensionId_)
+        return;
+      var extensionoptions = document.querySelector('extensionoptions');
+      this.overlayDiv_.removeChild(extensionoptions);
+      this.extensionId_ = undefined;
+    },
+
+    setExtensionAndShowOverlay: function(extensionId, extensionName) {

[Devlin, 2014/08/14 15:37:52]

docs

[ericzeng, 2014/08/14 18:11:54]

Done.

+      console.log('setExtensionAndShowOverlay');

[Devlin, 2014/08/14 15:37:51]

leftover?

[ericzeng, 2014/08/14 18:11:54]

affirmative

+      this.extensionId_ = extensionId;
+      var extensionoptions = new ExtensionOptions();

[Dan Beam, 2014/08/14 19:01:27]

nit: extensionOptions

+      extensionoptions.extension = this.extensionId_;
+      extensionoptions.autosize = 'on';
+
+      extensionoptions.onsizechanged = function(evt) {

[not at google - send to devlin, 2014/08/14 15:33:20]

TODO(ericzeng): Resize in a non-jarring way.

I don't think this is possible without some non-trivial creative CSS or JS, so
not worth it for now (the complication being that you need to resize to an
arbitrary size, not a pre-determined one).

[ericzeng, 2014/08/14 18:11:54]

Done (but not the todo).

[Dan Beam, 2014/08/14 19:01:27]

just keep the current JS and just add:

  transition: height 200ms, width 200ms;

or something in the CSS

[ericzeng, 2014/08/14 22:34:13]

Unfortunately that doesn't really help with the pop-in. It's an unfortunate
limitation with <extensionoptions> and the underlying browser plugin. The
embedded webview needs to be attached to the DOM and be visible for it to begin
autosizing. 

[not at google - send to devlin, 2014/08/15 15:51:06]

I think you could get some kind of decent approximation which solves the
jarring-ness - use webanimations to fade out the guestview, resize, then fade it
back in again - for example. If it's a really fast animation I reckon it'd look
neat (like 80s special effects neat). Maybe later though.

+        console.log(evt.width + 'x' + evt.height);

[Devlin, 2014/08/14 15:37:52]

leftover?

[ericzeng, 2014/08/14 18:11:54]

yes

+        this.overlayDiv_.style.width = evt.width;
+        this.overlayDiv_.style.height = evt.height;
+      }.bind(this);
+
+      this.overlayDiv_.appendChild(extensionoptions);
+
+      document.querySelector(
+          '#extension-options-overlay .extension-options-overlay-title')

[Devlin, 2014/08/14 15:37:51]

do we need the "#extension-options-overlay"?  How many
extension-options-overlay-titles are there gonna be? :P

[not at google - send to devlin, 2014/08/14 15:40:53]

The extension-options-overlay ID is nice, but Devlin has a point, perhaps you
can do away with the .extension-options-overlay-title class and use
'#extensions-options-overlay h1" here. If you want.

[Dan Beam, 2014/08/14 19:01:27]

if there's only ever going to be 1, use an #id.  if there's going to be more,
use a .class.

[ericzeng, 2014/08/14 22:34:13]

Done.

+          .textContent = extensionName;
+
+      this.setVisible(true);
+    }
+  };
+
+  // Export
+  return {
+    ExtensionOptionsOverlay: ExtensionOptionsOverlay
+  };
+});