Open embedded extension options from the extension action context menu

chrome/browser/resources/extensions/extension_list.js

 // Copyright (c) 2012 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.
 
 <include src="extension_error.js">
 
 cr.define('options', function() {
   'use strict';
 
   /**
@@ skipping 15 matching lines @@
    * @type {Object.<string, string>} A map from extension id to last reloaded
    *     timestamp. The timestamp is recorded when the user click the 'Reload'
    *     link. It is used to refresh the icon of an unpacked extension.
    *     This persists between calls to decorate.
    */
   var extensionReloadedTimestamp = {};
 
   ExtensionsList.prototype = {
     __proto__: HTMLDivElement.prototype,
 
+    /**
+     * Indicates whether an embedded options page that was navigated to through
+     * the '?options=' URL query has been shown to the user. This is necessary
+     * to prevent showExtensionNodes_ from opening the options more than once.
+     * @type {boolean}
+     * @private
+     */
+    optionsShown_: false,
+
     /** @override */
     decorate: function() {
       this.textContent = '';
 
       this.showExtensionNodes_();
     },
 
     getIdQueryParam_: function() {
       return parseQueryParams(document.location)['id'];
     },
 
+    getOptionsQueryParam_: function() {
+      return parseQueryParams(document.location)['options'];
+    },
+
     /**
      * Creates all extension items from scratch.
      * @private
      */
     showExtensionNodes_: function() {
       // Iterate over the extension data and add each item to the list.
       this.data_.extensions.forEach(this.createNode_, this);
 
       var idToHighlight = this.getIdQueryParam_();
-      if (idToHighlight && $(idToHighlight)) {
-        // Scroll offset should be calculated slightly higher than the actual
-        // offset of the element being scrolled to, so that it ends up not all
-        // the way at the top. That way it is clear that there are more elements
-        // above the element being scrolled to.
-        var scrollFudge = 1.2;
-        var scrollTop = $(idToHighlight).offsetTop - scrollFudge *
-            $(idToHighlight).clientHeight;
-        setScrollTopForDocument(document, scrollTop);
+      if (idToHighlight && $(idToHighlight))
+        this.scrollToNode_(idToHighlight);

[not at google - send to devlin, 2014/08/25 18:11:07]

I'm seeing some weird behaviour when you specify an
id=gighmmpiobklfepjocnamgkkbiglidom and an
options=gighmmpiobklfepjocnamgkkbiglidom. It removes the ID but still highlights
the extension, then when you dismiss the dialogue it continues to highlight the
extension.

Basically: I don't think it should hide the ID. You shouldn't be messing with
the parameters so much. Just add/remove the options parameter, that's all.

+
+      var idToOpenOptions = this.getOptionsQueryParam_();
+      if (idToOpenOptions && $(idToOpenOptions) && !this.optionsShown_) {
+        this.scrollToNode_(idToOpenOptions);
+        $(idToOpenOptions).querySelector('.options-link').click();

[not at google - send to devlin, 2014/08/25 18:11:07]

Why clicking on this link? Seems kinda roundabout. Could you pull the click
handling logic into a member function and call that from here and the event
handler? You could also do the "!this.optionsShown_" check, and setting it, in
that function.

[ericzeng, 2014/08/25 21:42:34]

Done.

+        this.optionsShown_ = true;
       }
 
       if (this.data_.extensions.length == 0)
         this.classList.add('empty-extension-list');
       else
         this.classList.remove('empty-extension-list');
     },
 
     /**
+     * Scrolls the page down to the extension node with the given id.
+     * @param {string} extensionId The id of the extension to scroll to.
+     * @private
+     */
+    scrollToNode_: function(extensionId) {
+      // Scroll offset should be calculated slightly higher than the actual
+      // offset of the element being scrolled to, so that it ends up not all
+      // the way at the top. That way it is clear that there are more elements
+      // above the element being scrolled to.
+      var scrollFudge = 1.2;
+      var scrollTop = $(extensionId).offsetTop - scrollFudge *
+          $(extensionId).clientHeight;
+      setScrollTopForDocument(document, scrollTop);
+    },
+
+    /**
      * Synthesizes and initializes an HTML element for the extension metadata
      * given in |extension|.
      * @param {Object} extension A dictionary of extension metadata.
      * @private
      */
     createNode_: function(extension) {
       var template = $('template-collection').querySelector(
           '.extension-list-item-wrapper');
       var node = template.cloneNode(true);
       node.id = extension.id;
@@ skipping 103 matching lines @@
         });
         fileAccess.querySelector('input').checked = extension.allowFileAccess;
         fileAccess.hidden = false;
       }
 
       // The 'Options' link.
       if (extension.enabled && extension.optionsUrl) {
         var options = node.querySelector('.options-link');
         options.addEventListener('click', function(e) {
           if (this.data_.enableEmbeddedExtensionOptions) {
+            // Add the options query string.

[not at google - send to devlin, 2014/08/25 18:11:07]

Mention why you need to postMessage here.

[ericzeng, 2014/08/25 21:42:34]

Done.

+            window.parent.postMessage({
+              method: 'updateHistory',
+              params: {
+                state: {},
+                path: '?options=' + extension.id,
+                replace: false

[not at google - send to devlin, 2014/08/25 18:11:07]

I think that clicking on an icon should replace the state. Clicking on a link
which opens a dialogue doesn't seem like it should be a new navigation.

I.e. if you:
1. go to google.com
2. go to chrome://extensions
3. click on an options link
4. press back

Should you go to chrome://extensions or google.com? I think google.com. It's
weird if not.

Site note: when I try this with your patch in, pressing back doesn't dismiss the
options dialogue. It should (but replacing state will bypass that issue).
HOWEVER when I press back a second time to go to google.com it's 100%
reproducible that the renderer crashes. Unfortunately I don't get a stack trace.
It might not be your patch, but, seems... plausible.

[ericzeng, 2014/08/25 21:42:34]

Replace state is done.

It looks like the crash is actually a regression caused by the window-closing
CL; it has something to do with how my guest view is implementing
WebContentsDelegate::CloseContents. It happens whenever you try to navigate away
and an <extensionoptions> is in the DOM. I'll see if I can fix that in separate
CL.

+              }
+            }, 'chrome://chrome');
             extensions.ExtensionOptionsOverlay.getInstance().
                 setExtensionAndShowOverlay(extension.id,
                                            extension.name,
                                            extension.icon);
           } else {
             chrome.send('extensionSettingsOptions', [extension.id]);
           }
           e.preventDefault();
         }.bind(this));
         options.hidden = false;
@@ skipping 209 matching lines @@
           topScroll -= pad / 2;
         setScrollTopForDocument(document, topScroll);
       }
     },
   };
 
   return {
     ExtensionsList: ExtensionsList
   };
 });