Dave Townsend [:mossop] Assignee
	Description • 15 years ago

The migration code to convert to chrome.manifest has been around since Firefox 1.5, probably time to retire it now.

	Dave Townsend [:mossop] Assignee
	Comment 1 • 15 years ago

Chrome registry part of the patch

	Dave Townsend [:mossop] Assignee
	Comment 2 • 15 years ago

Extension manager part of the patch.

After I was done removing the parts directly relating to the chrome registry migration I found that there was very little difference between the theme and extension code paths in the Installer object. The only difference was that we don't extract all of a themes xpi files. The XREDirProvider ensures that themes aren't used for anything other than theming so I think it makes better sense to just use the same code path for both extensions and themes so I've unified them, which really made the Installer object pointless so I just rolled the extraction code into safeInstallOperation.

	Dave Townsend [:mossop] Assignee
	Comment 3 • 15 years ago

Failed to refresh the patch before attaching apparently.

	Robert Strong (they/them - no direct email)
	Comment 4 • 15 years ago

Comment on attachment 377414 [details] [diff] [review]
extension manager patch rev 1

A couple of nits... I'll finish the review later

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>...
>-/**
>- * Safely attempt to perform a caller-defined install operation for a given
>- * item ID. Using aggressive success-safety checks, this function will attempt
>+ * Safely attempt to install or uninstall a given item ID from an install
>+ * location. Using aggressive success-safety checks, this function will attempt
>  * to move an existing location for an item aside and then allow installation
>  * into the appropriate folder. If any operation fails the installation will
>  * abort and roll back from the moved-aside old version.
>  * @param   itemID
>  *          The GUID of the item to perform the operation on.
nit: Please fix this per the nit below

>  * @param   installLocation
>  *          The Install Location where the item is installed.
nit: Please fix this per the nit below

>- * @param   installCallback
>- *          A caller supplied JS object with the following properties:
>- *          "data"      A data parameter to be passed to the callback.
>- *          "callback"  A function to perform the install operation. This
>- *                      function is passed three parameters:
>- *                      1. The GUID of the item being operated on.
>- *                      2. The Install Location where the item is installed.
>- *                      3. The "data" parameter on the installCallback object.
>- */
>-function safeInstallOperation(itemID, installLocation, installCallback) {
>+ * @param   file
>+ *          An xpi file to install to the location or null to just uninstall
>+ */
>+function safeInstallOperation(itemID, installLocation, file) {
>   var movedFiles = [];
> 
>   /**
>    * Reverts a deep move by moving backed up files back to their original
>    * location.
>    */
>   function rollbackMove()
>   {
>@@ -1985,16 +1604,63 @@ function safeInstallOperation(itemID, in
>     }
>     catch (e) {
>       ERROR("safeInstallOperation: failed to clean up the temporary backup of the " +
>             "older version: " + itemLocationTrash.path);
>       // This is a non-fatal error. Annoying, but non-fatal.
>     }
>   }
> 
>+  /**
>+   * Extracts an XPI's files into the extension's directory.
>+   * @param   extensionID
nit: itemID is used by safeInstallOperation - perhaps use it here as well? Seems like addonID doesn't really work and extensionID implies extensions. My main point is a single name should be decided upon and used consistently.

>+   *          The GUID of the Extension being installed.
nit: GUID have a specific format that we don't adhere to though we do allow GUIDs
http://en.wikipedia.org/wiki/Globally_Unique_Identifier

I'd like to see a consistent term used such as ID which is general enough to convey the meaning without misinforming anyone that this is in fact a GUID.

>+   * @param   installLocation
>+   *          The Install Location where the Extension is being installed.
Here you use the term Extension while below you use the term add-on. I'd suggest using the term add-on and going with lowercase for "Install Location" since it is just descriptive.

>+   * @param   xpiFile
>+   *          The source XPI file that contains the add-on.
>+   */
>+  function extractFiles(extensionID, installLocation, xpiFile) {

	Dave Townsend [:mossop] Assignee
	Comment 5 • 15 years ago

Updated the patch from the nits. I've also filed bug 493705 to look at updating the extension manager to use a consistent naming everywhere.

	Robert Strong (they/them - no direct email)
	Comment 6 • 15 years ago

Comment on attachment 378287 [details] [diff] [review]
extension manager patch rev 2

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>...
>@@ -2014,16 +1633,63 @@ function safeInstallOperation(itemID, in
>     }
>     catch (e) {
>       ERROR("safeInstallOperation: failed to clean up the temporary backup of the " +
>             "older version: " + itemLocationTrash.path);
>       // This is a non-fatal error. Annoying, but non-fatal.
>     }
>   }
> 
>+  /**
>+   * Extracts an XPI's files into the item's directory.
>+   * @param   itemID
>+   *          The ID of the item being installed.
>+   * @param   installLocation
>+   *          The install location where the item is being installed.
>+   * @param   xpiFile
>+   *          The source XPI file that contains the item.
>+   */
>+  function extractFiles(extensionID, installLocation, xpiFile) {
>+    var zipReader = getZipReaderForFile(xpiFile);
>+
>+    // create directories first
>+    var entries = zipReader.findEntries("*/");
>+    while (entries.hasMore()) {
>+      var entryName = entries.getNext();
>+      var target = installLocation.getItemFile(extensionID, entryName);
>+      if (!target.exists()) {
>+        try {
>+          target.create(Ci.nsILocalFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
>+        }
>+        catch (e) {
>+          ERROR("extractFiles: failed to create target directory for extraction " +
>+                " file = " + target.path + ", exception = " + e + "\n");
>+        }
>+      }
>+    }
>+
>+    entries = zipReader.findEntries(null);
>+    while (entries.hasMore()) {
>+      var entryName = entries.getNext();
>+      target = installLocation.getItemFile(extensionID, entryName);
>+      if (target.exists())
>+        continue;
I'm trying to remember why this exists check is here especially with a continue. I suspect it is a "just in case" but it doesn't seem to me that the target would ever exist... same goes for the directory above.

>@@ -2083,38 +1749,40 @@ function safeInstallOperation(itemID, in
>...
>-      ERROR("safeInstallOperation: failed to remove the folder we failed to install " +
>-            "an item into: " + itemLocation.path + " -- There is not much to suggest " +
>-            "here... maybe restart and try again?");
>+  if (file) {
>+    // Extract the xpi's files into the new directory
>+    try {
>+      extractFiles(itemID, installLocation, file);
>+    }
>+    catch (e) {
>+      // This means the install operation failed. Remove everything and roll back.
>+      ERROR("safeInstallOperation: install operation (caller-supplied callback) failed, " +
>+            "rolling back file moves and aborting installation.");
caller-supplied callback is no longer true.

>...
>@@ -4797,18 +4368,17 @@ ExtensionManager.prototype = {
>   _finalizeUninstall: function EM__finalizeUninstall(id) {
>     var ds = this.datasource;
> 
>     var installLocation = this.getInstallLocation(id);
>     if (!installLocation.itemIsManagedIndependently(id)) {
>       try {
>         // Having a callback that does nothing just causes the directory to be
>         // removed.
Need to update this comment to state that passing null for the file to install just causes the directory to be removed

>-        safeInstallOperation(id, installLocation,
>-                             { data: null, callback: function() { } });
>+        safeInstallOperation(id, installLocation, null);

I'll finish the rest of the review later

	Dave Townsend [:mossop] Assignee
	Comment 7 • 15 years ago

(In reply to comment #6)
> >+  function extractFiles(extensionID, installLocation, xpiFile) {
> >+    var zipReader = getZipReaderForFile(xpiFile);
> >+
> >+    // create directories first
> >+    var entries = zipReader.findEntries("*/");
> >+    while (entries.hasMore()) {
> >+      var entryName = entries.getNext();
> >+      var target = installLocation.getItemFile(extensionID, entryName);
> >+      if (!target.exists()) {
> >+        try {
> >+          target.create(Ci.nsILocalFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
> >+        }
> >+        catch (e) {
> >+          ERROR("extractFiles: failed to create target directory for extraction " +
> >+                " file = " + target.path + ", exception = " + e + "\n");
> >+        }
> >+      }
> >+    }
> >+
> >+    entries = zipReader.findEntries(null);
> >+    while (entries.hasMore()) {
> >+      var entryName = entries.getNext();
> >+      target = installLocation.getItemFile(extensionID, entryName);
> >+      if (target.exists())
> >+        continue;
> I'm trying to remember why this exists check is here especially with a
> continue. I suspect it is a "just in case" but it doesn't seem to me that the
> target would ever exist... same goes for the directory above.

I'm not sure about the directory case. I guess technically it is possible for a badly made zip file to include the same directory entry twice so maybe that actually came up in the past?

For the files loop I believe it is simply skipping the directory entries that were already created as part of the previous loop.

	Robert Strong (they/them - no direct email)
	Comment 8 • 15 years ago

Comment on attachment 378287 [details] [diff] [review]
extension manager patch rev 2

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>...
>@@ -1534,404 +1528,29 @@ WinRegInstallLocation.prototype = {

>-/**
>- * Safely attempt to perform a caller-defined install operation for a given
>- * item ID. Using aggressive success-safety checks, this function will attempt
>+ * Safely attempt to install or uninstall a given item ID from an install
>+ * location. Using aggressive success-safety checks, this function will attempt
nit: shouldn't this be "for an install location."?

>  * to move an existing location for an item aside and then allow installation
>  * into the appropriate folder. If any operation fails the installation will
>  * abort and roll back from the moved-aside old version.
>  * @param   itemID
>  *          The GUID of the item to perform the operation on.
>  * @param   installLocation
>  *          The Install Location where the item is installed.
>- * @param   installCallback
>- *          A caller supplied JS object with the following properties:
>- *          "data"      A data parameter to be passed to the callback.
>- *          "callback"  A function to perform the install operation. This
>- *                      function is passed three parameters:
>- *                      1. The GUID of the item being operated on.
>- *                      2. The Install Location where the item is installed.
>- *                      3. The "data" parameter on the installCallback object.
>- */
>-function safeInstallOperation(itemID, installLocation, installCallback) {
>+ * @param   file
>+ *          An xpi file to install to the location or null to just uninstall
>+ */
>+function safeInstallOperation(itemID, installLocation, file) {
>   var movedFiles = [];
> 
>   /**
>    * Reverts a deep move by moving backed up files back to their original
>    * location.
>    */
>   function rollbackMove()
>   {
>@@ -2014,16 +1633,63 @@ function safeInstallOperation(itemID, in
>     }
>     catch (e) {
>       ERROR("safeInstallOperation: failed to clean up the temporary backup of the " +
>             "older version: " + itemLocationTrash.path);
>       // This is a non-fatal error. Annoying, but non-fatal.
>     }
>   }
> 
>+  /**
>+   * Extracts an XPI's files into the item's directory.
>+   * @param   itemID
>+   *          The ID of the item being installed.
>+   * @param   installLocation
>+   *          The install location where the item is being installed.
>+   * @param   xpiFile
>+   *          The source XPI file that contains the item.
>+   */
>+  function extractFiles(extensionID, installLocation, xpiFile) {
please change the param to itemID

>+    var zipReader = getZipReaderForFile(xpiFile);
>+
>+    // create directories first
>+    var entries = zipReader.findEntries("*/");
>+    while (entries.hasMore()) {
>+      var entryName = entries.getNext();
>+      var target = installLocation.getItemFile(extensionID, entryName);
>+      if (!target.exists()) {
>+        try {
>+          target.create(Ci.nsILocalFile.DIRECTORY_TYPE, PERMS_DIRECTORY);
>+        }
>+        catch (e) {
>+          ERROR("extractFiles: failed to create target directory for extraction " +
>+                " file = " + target.path + ", exception = " + e + "\n");
nit: two spaces between extraction and file. Perhaps just a separator between the message and the details.

I've also noticed there are at least a couple of different separator formats used such as ' ... ' and ' -- ' in this section of code as well as ': ' and ' = ' being used. Probably best just to clean this up during the next rewrite.

>+        }
>+      }
>+    }
>+
>+    entries = zipReader.findEntries(null);
>+    while (entries.hasMore()) {
>+      var entryName = entries.getNext();
>+      target = installLocation.getItemFile(extensionID, entryName);
>+      if (target.exists())
>+        continue;
>+
>+      try {
>+        target.create(Ci.nsILocalFile.NORMAL_FILE_TYPE, PERMS_FILE);
>+      }
>+      catch (e) {
>+        ERROR("extractFiles: failed to create target file for extraction " +
>+              " file = " + target.path + ", exception = " + e + "\n");
nit: two spaces between extraction and file. Perhaps just a separator between the message and the details.

>@@ -4726,20 +4285,32 @@ ExtensionManager.prototype = {
>       }
>       return;
>     }
>     var itemLocation = installLocation.getItemLocation(id);
> 
>     if (!file && "stageFile" in installLocation)
>       file = installLocation.getStageFile(id);
> 
>-    // If |file| is null or does not exist, the installer assumes the item is
>-    // a dropped-in directory.
>-    var installer = new Installer(this.datasource, id, installLocation, type);
>-    installer.installFromFile(file);
>+    // If there is a file to install then perform the install operation
Shouldn't a similar comment - as the one removed above - about assuming the item is a dropped in directory when getStageFile returns null be included here?

>+    if (file && file.exists())
>+      safeInstallOperation(id, installLocation, file);
>+
>+    var metadataFile = installLocation.getItemFile(id, FILE_INSTALL_MANIFEST);
>+    if (metadataFile.exists()) {
>+      var metadataDS = getInstallManifest(metadataFile);
>+      if (metadataDS) {
>+        // Add metadata for the extension to the global extension metadata set
nit: this comment seems a tad wrong / out of date.

With comments addressed r=me.

	Dave Townsend [:mossop] Assignee
	Comment 9 • 15 years ago

Fixed nits and landed: http://hg.mozilla.org/mozilla-central/rev/54c56e237847

	AKIHIRO Misaki (a.k.a Kuden)
	Comment 10 • 15 years ago

I filed Bug 500158.

Still, a lot of theme use contents.rdf.
because, MDC document is written like this:
> You must have a top-level chrome.manifest or contents.rdf file
> which registers the chrome for the theme.

	Eric Shepherd [:sheppy]
	Comment 11 • 15 years ago

The immediate doc concerns are updated. The theme docs need a significant update, but that will be tracked in bug 500158.