Drop support for contents.rdf chrome registrations

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

({dev-doc-complete})

Trunk
mozilla1.9.2a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

10 years ago
The migration code to convert to chrome.manifest has been around since Firefox 1.5, probably time to retire it now.
(Assignee)

Updated

10 years ago
Blocks: 302527
(Assignee)

Comment 1

10 years ago
Chrome registry part of the patch
Attachment #377396 - Flags: review?(benjamin)
(Assignee)

Comment 2

10 years ago
Posted patch extension manager patch rev 1 (obsolete) — Splinter Review
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.
Attachment #377397 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

10 years ago
Posted patch extension manager patch rev 1 (obsolete) — Splinter Review
Failed to refresh the patch before attaching apparently.
Attachment #377397 - Attachment is obsolete: true
Attachment #377414 - Flags: review?(robert.bugzilla)
Attachment #377397 - Flags: review?(robert.bugzilla)
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) {

Updated

10 years ago
Attachment #377396 - Flags: review?(benjamin) → review+
(Assignee)

Comment 5

10 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.
Attachment #377414 - Attachment is obsolete: true
Attachment #378287 - Flags: review?(robert.bugzilla)
Attachment #377414 - Flags: review?(robert.bugzilla)
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
(Assignee)

Comment 7

10 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.
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.
(Assignee)

Comment 9

10 years ago
Fixed nits and landed: http://hg.mozilla.org/mozilla-central/rev/54c56e237847
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9.2 → mozilla1.9.2a1
Depends on: 500158
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.
Keywords: dev-doc-needed
The immediate doc concerns are updated. The theme docs need a significant update, but that will be tracked in bug 500158.
You need to log in before you can comment on or make changes to this bug.