[Extensions] Make chrome://extensions use developerPrivate for packing crxs

chrome/browser/resources/extensions/pack_extension_overlay.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.
 
 cr.define('extensions', function() {
   /**
    * PackExtensionOverlay class
    * Encapsulated handling of the 'Pack Extension' overlay page.
    * @constructor
    */
@@ skipping 30 matching lines @@
       extensions.ExtensionSettings.showOverlay(null);
     },
 
     /**
      * Handles a click on the pack button.
      * @param {Event} e The click event.
      */
     handleCommit_: function(e) {
       var extensionPath = $('extension-root-dir').value;
       var privateKeyPath = $('extension-private-key').value;
-      chrome.send('pack', [extensionPath, privateKeyPath, 0]);
+      chrome.developerPrivate.packDirectory(
+          extensionPath, privateKeyPath, 0, this.onPackResponse_.bind(this));
     },
 
     /**
      * Utility function which asks the C++ to show a platform-specific file
-     * select dialog, and fire |callback| with the |filePath| that resulted.
-     * |selectType| can be either 'file' or 'folder'. |operation| can be 'load'
-     * or 'pem' which are signals to the C++ to do some operation-specific
-     * configuration.
+     * select dialog, and set the value property of |node| to the selected path.
+     * @param {SelectType} selectType The type of selection to use.
+     * @param {FileType} fileType The type of file to select.
+     * @param {HTMLInputElement} node The node to set the value of.
      * @private
      */
-    showFileDialog_: function(selectType, operation, callback) {
-      window.handleFilePathSelected = function(filePath) {
-        callback(filePath);
-        window.handleFilePathSelected = function() {};
-      };
-
-      chrome.send('packExtensionSelectFilePath', [selectType, operation]);
+    showFileDialog_: function(selectType, fileType, node) {
+      chrome.developerPrivate.choosePath(selectType, fileType, function(path) {
+        if (!chrome.runtime.lastError && path)

[not at google - send to devlin, 2015/02/27 17:58:11]

Add note, that this will happen if the file chooser is cancelled.

[Dan Beam, 2015/02/27 19:57:51]

i find it a little odd that we rely on a global here instead of passing back an
error to the callback.

if something else goes wrong before this dialog is shown, will
chrome.runtime.lastError still be set?  is it only set while inside this
function?  if this function is async and the global operates similarly to any
other global: this sounds super racey.

[Devlin, 2015/02/27 21:37:02]

Done.

[Devlin, 2015/02/27 21:37:02]

TL;DR: Part of the extensions system.  Should be okay.

Kalman can confirm this, but I believe that runtime.lastError is actually only
set for the lifespan of the callback.  That is, before we call the callback in
extension bindings, we set lastError, and then we unset it afterwards.  It's
done this way for every extension api, so I don't think it's really in the scope
of this CL to change it. :)

[Dan Beam, 2015/02/27 22:57:17]

lame-ish

[not at google - send to devlin, 2015/02/27 22:58:44]

Indeed. It's weighing down my TODO list to fix it, but it's an epic project.

+          node.value = path;
+      });
     },
 
     /**
      * Handles the showing of the extension directory browser.
      * @param {Event} e Change event.
      * @private
      */
     handleBrowseExtensionDir_: function(e) {
-      this.showFileDialog_('folder', 'load', function(filePath) {
-        $('extension-root-dir').value = filePath;
-      });
+      this.showFileDialog_(
+          'FOLDER',
+          'LOAD',
+          /** @type {HTMLInputElement} */ ($('extension-root-dir')));
     },
 
     /**
      * Handles the showing of the extension private key file.
      * @param {Event} e Change event.
      * @private
      */
     handleBrowsePrivateKey_: function(e) {
-      this.showFileDialog_('file', 'pem', function(filePath) {
-        $('extension-private-key').value = filePath;
-      });
+      this.showFileDialog_(
+          'FILE',
+          'PEM',
+          /** @type {HTMLInputElement} */ ($('extension-private-key')));
+    },
+
+    /**
+     * Handles a response from a packDirectory call.
+     * @param {PackDirectoryResponse} response The response of the pack call.
+     * @private
+     */
+    onPackResponse_: function(response) {

[not at google - send to devlin, 2015/02/27 17:58:11]

I will give this a spin when I get to work.

[Devlin, 2015/02/27 21:37:02]

Spun.

+      console.log('On pack response');
+      console.log(JSON.stringify(response));

[not at google - send to devlin, 2015/02/27 17:58:11]

Remove logging?

[Dan Beam, 2015/02/27 19:57:51]

+1

[Devlin, 2015/02/27 21:37:02]

Done.

+      /** @type {string} */
+      var alertTitle;
+      /** @type {string} */
+      var alertOk;
+      /** @type {string} */
+      var alertCancel;
+      /** @type {function()} */
+      var alertOkCallback;
+      /** @type {function()} */
+      var alertCancelCallback;
+
+      var closeAlert = function() {
+        extensions.ExtensionSettings.showOverlay(null);
+      };
+
+      if (response.status == 'SUCCESS') {

[not at google - send to devlin, 2015/02/27 19:23:58]

Can you switch?

[Dan Beam, 2015/02/27 19:57:51]

yes, switch (stringValue) {...} works in js, and if we could create an @enum
{string} for the potential responses that'd be cool as well (I think it'd
integrate well with the closure compiler)

[Devlin, 2015/02/27 21:37:02]

switch done.

Dan, what would it look like to use the closure compiler syntax for an enum
here?  (Bearing in mind that this "enum" is actually just a string, not a real
enum, and is part of the extension system).  Would we create a separate enum,
and assign response.status into it?  Or...?

[Dan Beam, 2015/02/27 22:57:17]

http://stackoverflow.com/questions/18186309/is-there-a-way-to-use-an-enum-as-...

/** @enum {string} */
var PackDirectoryResponse = {
  SUCCESS: 'SUCCESS',
  WARNING: 'WARNING',
  ERROR: 'ERROR',
};

... later on  ...
@typedef PackDirectoryResponse {{
  ...
  status: PackDirecotyResponse,
  ...
}}

[Devlin, 2015/02/27 23:38:37]

I was more curious if there was a way of doing it without adjusting the closure
compiler extern definition since, technically, even though we say these are
enums in the idl, they're really just strings we pass.  What's more, we
*shouldn't* have the practice of declaring enums as such in the extern in
general, because then we can't call the function.  For instance, if we use an
enum for the "enum" of SelectType (FILE or FOLDER), then closure compiler breaks
when we try to call it with 'FOLDER', since it's a string. And, of course, we
don't (as part of the extension api) expose enum definitions on the api
anywhere, so we can't pass a real enum FileType.  Because of this, I don't
really think declaring PackStatus to be an @enum in the externs is really the
right thing to do... WDYT?

[Dan Beam, 2015/02/28 00:17:57]

JavaScript has no enums natively, so using some existing type is necessary if we
want similar functionality now.  And even in C++, enums are backed by integers.
why can't you call the function with SelectedType.FOLDER instead of 'FOLDER',
even in tests?  declare the @enum at runtime instead of in @externs.
why don't you?  the web does this sometimes:
https://developer.mozilla.org/en-US/docs/Web/API/Node/nodeType

+        alertTitle = loadTimeData.getString('packExtensionOverlay');
+        alertOk = loadTimeData.getString('ok');
+        alertOkCallback = closeAlert;
+        // No 'Cancel' option.
+      } else if (response.status == 'WARNING') {
+        alertTitle = loadTimeData.getString('packExtensionWarningTitle'),
+        alertOk = loadTimeData.getString('packExtensionProceedAnyway'),
+        alertCancel = loadTimeData.getString('cancel'),
+        alertOkCallback = function() {
+          chrome.developerPrivate.packDirectory(
+              response.item_path,
+              response.pem_path,
+              response.override_flags,
+              this.onPackResponse_.bind(this));
+          closeAlert();
+        }.bind(this);
+        alertCancelCallback = closeAlert;
+      } else {
+        assert(response.status == 'ERROR');

[not at google - send to devlin, 2015/02/27 19:23:58]

I'd rather cover this in a switch case then in a default case assert(false).

[Devlin, 2015/02/27 21:37:02]

Done.

+        alertTitle = loadTimeData.getString('packExtensionErrorTitle'),
+        alertOk = loadTimeData.getString('ok'),
+        alertOkCallback = function() {
+          extensions.ExtensionSettings.showOverlay($('pack-extension-overlay'));
+        };
+        // No 'Cancel' option.
+      }
+
+      alertOverlay.setValues(alertTitle,

[Dan Beam, 2015/02/27 19:57:50]

where does this variable come from?  is alertOverlay == $('alertOverlay')?

[Devlin, 2015/02/27 21:37:03]

Took me forever to find it, to, when I was trying to figure out what these calls
were doing.  It's an object defined here:
https://code.google.com/p/chromium/codesearch#chromium/src/ui/webui/resources...
(and is separate, but related, to $('alertOverlay'))

+                             response.message,
+                             alertOk,
+                             alertCancel,
+                             alertOkCallback,
+                             alertCancelCallback);
+      extensions.ExtensionSettings.showOverlay($('alertOverlay'));
     },
   };
 
-  /**
-   * Wrap up the pack process by showing the success |message| and closing
-   * the overlay.
-   * @param {string} message The message to show to the user.
-   */
-  PackExtensionOverlay.showSuccessMessage = function(message) {
-    alertOverlay.setValues(
-        loadTimeData.getString('packExtensionOverlay'),
-        message,
-        loadTimeData.getString('ok'),
-        '',
-        function() {
-          extensions.ExtensionSettings.showOverlay(null);
-        });
-    extensions.ExtensionSettings.showOverlay($('alertOverlay'));
-  };
-
-  /**
-   * Post an alert overlay showing |message|, and upon acknowledgement, close
-   * the alert overlay and return to showing the PackExtensionOverlay.
-   * @param {string} message The error message.
-   */
-  PackExtensionOverlay.showError = function(message) {
-    alertOverlay.setValues(
-        loadTimeData.getString('packExtensionErrorTitle'),
-        message,
-        loadTimeData.getString('ok'),
-        '',
-        function() {
-          extensions.ExtensionSettings.showOverlay($('pack-extension-overlay'));
-        });
-    extensions.ExtensionSettings.showOverlay($('alertOverlay'));
-  };
-
   // Export
   return {
     PackExtensionOverlay: PackExtensionOverlay
   };
 });