Last Comment Bug 474289 - Automatically install add-ons shipped with the application into new profiles
: Automatically install add-ons shipped with the application into new profiles
Status: VERIFIED FIXED
[rewrite]
: dev-doc-complete
Product: Toolkit
Classification: Components
Component: Add-ons Manager (show other bugs)
: Trunk
: All All
: P2 enhancement (vote)
: mozilla2.0b10
Assigned To: Dave Townsend [:mossop]
:
: Andy McKay [:andym]
Mentors:
Depends on: 633225
Blocks: tomtom 594858 627240
  Show dependency treegraph
 
Reported: 2009-01-19 04:15 PST by Wladimir Palant
Modified: 2013-01-24 11:52 PST (History)
14 users (show)
dtownsend: in‑testsuite+
dtownsend: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wanted


Attachments
patch rev 1 (21.98 KB, patch)
2011-01-13 14:26 PST, Dave Townsend [:mossop]
no flags Details | Diff | Splinter Review
directory service patch (1.97 KB, patch)
2011-01-14 14:13 PST, Dave Townsend [:mossop]
benjamin: review+
Details | Diff | Splinter Review
main patch rev 1 (23.69 KB, patch)
2011-01-14 15:11 PST, Dave Townsend [:mossop]
robert.strong.bugs: review-
Details | Diff | Splinter Review
main patch rev 2 (30.79 KB, patch)
2011-01-18 15:02 PST, Dave Townsend [:mossop]
robert.strong.bugs: review+
Details | Diff | Splinter Review
full patch [Checked in: Comment 25] (33.91 KB, patch)
2011-01-19 13:58 PST, Dave Townsend [:mossop]
dtownsend: review+
mbeltzner: approval2.0+
Details | Diff | Splinter Review

Description Wladimir Palant 2009-01-19 04:15:11 PST
TomTom HOME has an extension that is distributed both pre-installed with the application and as a downloadable XPI (for the case that an update is necessary before an application update). The downloadable version of the extension is always installed in user's profile which by itself is ok. We get a problem however when application is updated - suddenly the version in app directory is newer than the version in profile, yet the version in profile still takes precedence.

I think that install location priorities should be only a secondary mechanism for deciding which extension should be used. First step should be comparing the version number - if one of the extension instances is newer it should be used regardless of where it is installed. Only if the version number of several extension instances is identical priorities should be considered.

My impression from looking at the code is that this approach will break some optimizations - I hope that it can be done nevertheless.
Comment 1 Dave Townsend [:mossop] 2009-01-19 04:25:08 PST
As it stands this is intentional. The user install location has the highest priority even if the user has an older version there. See also bug 413153. However I am in the process of trying to come up with a better plan for managing the priorities of the install locations to try to fit what system admins and app developers might find more useful.
Comment 2 Wladimir Palant 2009-01-19 04:43:03 PST
I guess implementing my suggestion above is a waste of time then. Dave, any idea in which direction your "better plan" will be going?

Sounds like implementing something similar to what is suggested in bug 413153 is our best option right now.
Comment 3 Dave Townsend [:mossop] 2009-01-19 04:51:34 PST
(In reply to comment #2)
> I guess implementing my suggestion above is a waste of time then. Dave, any
> idea in which direction your "better plan" will be going?

It's a little early yet, there is a thread on the newsgroup where I proposed some changes to the install locations and ended up realising that there were lots of different scenarios and we didn't really support many of them well, so the first step is to come up with a list of those scenarios and decide what their relative importance is. This is what I'm doing when I find spare minutes.

http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/a9ca75b3747c9c4e/22e4b7d3c7e9ab66
Comment 4 Wladimir Palant 2009-01-19 05:42:50 PST
From that discussion Benjamin's idea sounds most interesting. The complete solution would be something like this: leave appdir/extensions to appManaged extensions (maybe make appManaged implicit there) and introduce another directory for "default extensions", maybe appdir/defaults/extensions. On application upgrade a component should then go through all extensions in appdir/defaults/extensions and install them in profile if either they aren't installed there or an older version is installed. Complicating factor: it should respect user's choice. This can be done by remembering which extensions/versions have been previously installed automatically. So if we installed the extension before and it isn't there any more this means that the user uninstalled it - don't attempt installation any more. If we installed the extension before but the version currently in profile is older - user explicitly chose to downgrade, don't upgrade him automatically.

Writing code that will do that for us shouldn't be a big problem but ideally extension manager should handle things like this itself.
Comment 5 Dave Townsend [:mossop] 2009-02-12 08:25:51 PST
(In reply to comment #4)
> From that discussion Benjamin's idea sounds most interesting. The complete
> solution would be something like this: leave appdir/extensions to appManaged
> extensions (maybe make appManaged implicit there) and introduce another
> directory for "default extensions", maybe appdir/defaults/extensions. On
> application upgrade a component should then go through all extensions in
> appdir/defaults/extensions and install them in profile if either they aren't
> installed there or an older version is installed. Complicating factor: it
> should respect user's choice. This can be done by remembering which
> extensions/versions have been previously installed automatically. So if we
> installed the extension before and it isn't there any more this means that the
> user uninstalled it - don't attempt installation any more. If we installed the
> extension before but the version currently in profile is older - user
> explicitly chose to downgrade, don't upgrade him automatically.
> 
> Writing code that will do that for us shouldn't be a big problem but ideally
> extension manager should handle things like this itself.

This does sound like something we should have in some form. Benjamin and I agree however that it should just be the simpler case. On first run of a new version of an application any bundled default extensions that hadn't already been installed previously would be installed to the profile. So there wouldn't be any upgrading of extensions in the profile from the application defaults. Instead all upgrades should be delivered using the usual add-on upgrade path.

The suggestion was to have the defaults appear in appdir/distribution alongside the other distribution stuff and use a pref distribution.extension.<ID>.installed to flag whether an add-on has been installed in this manner previously.

There are a few places where the code could go but it probably does make most sense to include it in the extension manager since that will be turned on/off depending on whether the application has extensions enabled. The checkForMismatches method ought to be a good starting point.
Comment 6 Dave Townsend [:mossop] 2011-01-13 14:26:13 PST
Created attachment 503619 [details] [diff] [review]
patch rev 1

Mostly complete, needs one more in-depth test that extensions with more than one file actually work right.

This implements basically that described above, when a new version of an app runs any distributed extensions are installed unless a newer version exists in the profile or the extension had previously been installed by a distribution and since uninstalled. This allows upgrades at the moment unlike what I said above. In retrospect I think this is probably a useful additional feature.
Comment 7 Dave Townsend [:mossop] 2011-01-14 14:13:49 PST
Created attachment 503982 [details] [diff] [review]
directory service patch

This patch adds XREAppDist to the directory service pointing to the distribution directory. The only reason this is necessary is to be able to override it in the tests.
Comment 8 Dave Townsend [:mossop] 2011-01-14 15:11:09 PST
Created attachment 504001 [details] [diff] [review]
main patch rev 1

This is the main part of the patch. It implements the behaviour above and tests it. The only known issue that I think we can live with for now is that it doesn't respect em:unpack or the global preference when installing the add-on, instead if the add-on exists as an xpi in the distribution dir then that is how it is installed and similarly for if it exists as a directory.
Comment 9 Mike Hommey [:glandium] 2011-01-15 01:03:01 PST
What about extensions in XRE_SYS_LOCAL_EXTENSION_PARENT_DIR and XRE_SYS_SHARE_EXTENSION_PARENT_DIR ?
Comment 10 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-15 15:04:08 PST
Comment on attachment 504001 [details] [diff] [review]
main patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -69,16 +69,17 @@ const PREF_EM_ENABLED_SCOPES          = 
> const PREF_EM_SHOW_MISMATCH_UI        = "extensions.showMismatchUI";
> const PREF_EM_DISABLED_ADDONS_LIST    = "extensions.disabledAddons";
> const PREF_XPI_ENABLED                = "xpinstall.enabled";
> const PREF_XPI_WHITELIST_REQUIRED     = "xpinstall.whitelist.required";
> const PREF_XPI_WHITELIST_PERMISSIONS  = "xpinstall.whitelist.add";
> const PREF_XPI_BLACKLIST_PERMISSIONS  = "xpinstall.blacklist.add";
> const PREF_XPI_UNPACK                 = "extensions.alwaysUnpack";
> const PREF_INSTALL_REQUIREBUILTINCERTS = "extensions.install.requireBuiltInCerts";
>+const PREF_INSTALLED_ADDON            = "extensions.installedAddon.";
nit: PREF_BRANCH_INSTALLED_ADDON

>@@ -189,17 +191,30 @@ SafeMoveOperation.prototype = {
>     catch (e) {
>       ERROR("Failed to move file " + aFile.path + " to " +
>             aTargetDirectory.path, e);
>       throw e;
>     }
>     this._movedFiles.push({ oldFile: oldFile, newFile: newFile });
>   },
> 
>-  _moveDirectory: function(aDirectory, aTargetDirectory) {
>+  _copyFile: function(aFile, aTargetDirectory) {
nit: *might* be a tad cleaner to just have an _installFile function with an aCopy param and combines _copyFile and _moveFile.

>+    let newFile = aFile.clone();
>+    try {
>+      newFile.copyTo(aTargetDirectory, null);
>+    }
>+    catch (e) {
>+      ERROR("Failed to copy file " + aFile.path + " to " +
>+            aTargetDirectory.path, e);
>+      throw e;
>+    }
>+    this._movedFiles.push({ oldFile: null, newFile: newFile });
>+  },
>+
>+  _moveDirectory: function(aDirectory, aTargetDirectory, aCopy) {
nit: since this can be a copy or move it would be a good thing to rename it to something like _installDirectory

>     let newDir = aTargetDirectory.clone();
>     newDir.append(aDirectory.leafName);
>     try {
>       newDir.create(Ci.nsILocalFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>     }
>     catch (e) {
>       ERROR("Failed to create directory " + newDir.path, e);
>       throw e;
>@@ -215,66 +230,94 @@ SafeMoveOperation.prototype = {
>...
>-  _moveDirEntry: function(aDirEntry, aTargetDirectory) {
>+  _moveDirEntry: function(aDirEntry, aTargetDirectory, aCopy) {
nit: since this can be a copy or move it would be a good thing to rename it to something like _installDirEntry

>     try {
>       if (aDirEntry.isDirectory())
>-        this._moveDirectory(aDirEntry, aTargetDirectory);
>+        this._moveDirectory(aDirEntry, aTargetDirectory, aCopy);
>+      else if (aCopy)
>+        this._copyFile(aDirEntry, aTargetDirectory);
>       else
>         this._moveFile(aDirEntry, aTargetDirectory);
>     }
>     catch (e) {
>-      ERROR("Failure moving " + aDirEntry.path + " to " + aTargetDirectory.path);
>+      ERROR("Failure " + (aCopy ? "copying" : "moving") + " " + aDirEntry.path +
>+            " to " + aTargetDirectory.path);
>       throw e;
>     }
>   },
> 
>   /**
>    * Moves a file or directory into a new directory. If an error occurs then all
>    * files that have been moved will be moved back to their original location.
>    *
>    * @param  aFile
>    *         The file or directory to be moved.
>    * @param  aTargetDirectory
>    *         The directory to move into, this is expected to be an empty
>    *         directory.
>    */
>   move: function(aFile, aTargetDirectory) {
>     try {
>-      this._moveDirEntry(aFile, aTargetDirectory);
>+      this._moveDirEntry(aFile, aTargetDirectory, false);
>+    }
>+    catch (e) {
>+      this.rollback();
>+      throw e;
>+    }
>+  },
>+
>+  /**
>+   * Copies a file or directory into a new directory. If an error occurs then
>+   * all files that have been copied will be removed.
>+   *
>+   * @param  aFile
>+   *         The file or directory to be copied.
>+   * @param  aTargetDirectory
>+   *         The directory to copy into, this is expected to be an empty
>+   *         directory.
>+   */
>+  copy: function(aFile, aTargetDirectory) {
nit: might be cleaner to just have a single install function that handles move and copy and has an aCopy param.

>+    try {
>+      this._moveDirEntry(aFile, aTargetDirectory, true);
>     }
>     catch (e) {
>       this.rollback();
>       throw e;
>     }
>   },
> 
>   /**
Comment 11 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-15 15:24:50 PST
cc'ing Kev to give him a heads up since this *might* make Firefox 4.0
Comment 12 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-15 15:34:51 PST
I think we are going to need something like appManaged added back so it is possible to differentiate between add-ons that will be updated by an app update and add-ons that won't so it is possible for app update to not include these add-ons in the incompatible list.

Thoughts?
Comment 13 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-15 15:47:32 PST
Comment on attachment 504001 [details] [diff] [review]
main patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -1797,16 +1851,134 @@ var XPIProvider = {
>         // Non-critical, just saves some perf on startup if we clean this up.
>         LOG("Error removing staging dir " + stagingDir.path, e);
>       }
>     });
>     return changed;
>   },
> 
>   /**
>+   * Installs any add-ons bundled with the application into the profile unless
>+   * a newer version already exists or the user has previously uninstalled the
>+   * bundled add-on.
Needs @param aManifests

>+   *
>+   * @return true if any new add-ons were installed
>+   */
>+  installBundledAddons: function XPI_installBundledAddons(aManifests) {
>...
>+      if (existingEntry) {
>+        let existingAddon;
>+        try {
>+          existingAddon = loadManifestFromFile(existingEntry);
>+
>+          if (Services.vc.compare(addon.version, existingAddon.version) <= 0)
>+            continue;
>+        }
>+        catch (e) {
>+          // Bad add-on in the profile so just proceed and install over the top
I think WARN should be called here

>+        }
>+      }
>+      else if (Prefs.getBoolPref(PREF_INSTALLED_ADDON + id, false)) {
>+        continue;
>+      }
>+
>+      // Install the add-on
>+      try {
>+        profileLocation.installAddon(id, entry, null, true);
>+        LOG("Installed bundled add-on " + id);
>+
>+        Services.prefs.setBoolPref(PREF_INSTALLED_ADDON + id, true)
>+
>+        // Keep the add-on's manifest ready for adding to the database later
>+        // also removes any old manifest that might be there from a pending
>+        // install
This comment is a tad unclear

>@@ -6846,19 +7016,22 @@ DirectoryInstallLocation.prototype = {
>    * Installs an add-on into the install location.
>    *
>    * @param  aId
>    *         The ID of the add-on to install
>    * @param  aSource
>    *         The source nsIFile to install from
>    * @param  aExistingAddonID
>    *         The ID of an existing add-on to uninstall at the same time
>+   * @param  aCopy
>+   *         If false the source files will be moved to the new location,
>+   *         otherwise they will only be copied
>    * @return an nsIFile indicating where the add-on was installed to
>    */
>-  installAddon: function DirInstallLocation_installAddon(aId, aSource, aExistingAddonID) {
>+  installAddon: function DirInstallLocation_installAddon(aId, aSource, aExistingAddonID, aCopy) {
nit: this line is getting a tad long.

>     let trashDir = this.getTrashDir();
> 
>     let transaction = new SafeMoveOperation();
> 
>     let self = this;
>     function moveOldAddon(aId) {
>       let file = self._directory.clone().QueryInterface(Ci.nsILocalFile);
>       file.append(aId);
>@@ -6876,20 +7049,25 @@ DirectoryInstallLocation.prototype = {
> 
>     // If any of these operations fails the finally block will clean up the
>     // temporary directory
>     try {
>       moveOldAddon(aId);
>       if (aExistingAddonID && aExistingAddonID != aId)
>         moveOldAddon(aExistingAddonID);
> 
>-      if (aSource.isFile())
>-        Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
>-
>-      transaction.move(aSource, this._directory);
>+      if (aCopy) {
>+        transaction.copy(aSource, this._directory);
>+      }
>+      else {
>+        if (aSource.isFile())
>+          Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
Would an update to an existing install need a flush-cache-entry for the aCopy case?

>+
>+        transaction.move(aSource, this._directory);
>+      }
>     }
>     finally {
>       // It isn't ideal if this cleanup fails but it isn't worth rolling back
>       // the install because of it.
>       try {
>         recursiveRemove(trashDir);
>       }
>       catch (e) {
Comment 14 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-15 15:54:31 PST
Comment on attachment 504001 [details] [diff] [review]
main patch rev 1

Definitely would like to look through this again and get answers to the questions before +'ing. I'm also a tad concerned that applications will start using this and there is no way to disable this for tests.
Comment 15 Dave Townsend [:mossop] 2011-01-18 14:08:23 PST
(In reply to comment #9)
> What about extensions in XRE_SYS_LOCAL_EXTENSION_PARENT_DIR and
> XRE_SYS_SHARE_EXTENSION_PARENT_DIR ?

This is entirely separate to the install locations for now. In this case we can get away with only checking on the startup of a new app, to do similarly elsewhere would mean a startup hit all the time.
Comment 16 Dave Townsend [:mossop] 2011-01-18 14:41:07 PST
(In reply to comment #13)
> > 
> >     // If any of these operations fails the finally block will clean up the
> >     // temporary directory
> >     try {
> >       moveOldAddon(aId);
> >       if (aExistingAddonID && aExistingAddonID != aId)
> >         moveOldAddon(aExistingAddonID);
> > 
> >-      if (aSource.isFile())
> >-        Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
> >-
> >-      transaction.move(aSource, this._directory);
> >+      if (aCopy) {
> >+        transaction.copy(aSource, this._directory);
> >+      }
> >+      else {
> >+        if (aSource.isFile())
> >+          Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
> Would an update to an existing install need a flush-cache-entry for the aCopy
> case?

That is taken care of above in moveOldAddon.
Comment 17 Dave Townsend [:mossop] 2011-01-18 14:59:59 PST
(In reply to comment #12)
> I think we are going to need something like appManaged added back so it is
> possible to differentiate between add-ons that will be updated by an app update
> and add-ons that won't so it is possible for app update to not include these
> add-ons in the incompatible list.
> 
> Thoughts?

We might want to think about that but it is possible for apps to just publish the version of the add-on that ships with the next version of the app in the extension's update.rdf and that should stop app update from warning about it right?

I'd rather avoid making more changes here for the time being and see how it works out.
Comment 18 Dave Townsend [:mossop] 2011-01-18 15:02:02 PST
Created attachment 504857 [details] [diff] [review]
main patch rev 2

Updated patch address comments, adds a pref to disable installing add-ons (it defaults to on so that distributors need to make the least amount of changes to the app) and also including some test files that got missed last time.
Comment 19 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-18 16:13:59 PST
Comment on attachment 504857 [details] [diff] [review]
main patch rev 2

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -69,16 +69,18 @@ const PREF_EM_ENABLED_SCOPES          = 
> const PREF_EM_SHOW_MISMATCH_UI        = "extensions.showMismatchUI";
> const PREF_EM_DISABLED_ADDONS_LIST    = "extensions.disabledAddons";
> const PREF_XPI_ENABLED                = "xpinstall.enabled";
> const PREF_XPI_WHITELIST_REQUIRED     = "xpinstall.whitelist.required";
> const PREF_XPI_WHITELIST_PERMISSIONS  = "xpinstall.whitelist.add";
> const PREF_XPI_BLACKLIST_PERMISSIONS  = "xpinstall.blacklist.add";
> const PREF_XPI_UNPACK                 = "extensions.alwaysUnpack";
> const PREF_INSTALL_REQUIREBUILTINCERTS = "extensions.install.requireBuiltInCerts";
>+const PREF_INSTALL_BUNDLED_ADDONS     = "extensions.installBundledAddons";
The distribution dir already has a bundles concept and directory. Perhaps, PREF_INSTALL_DISTRO_ADDONS or PREF_INSTALL_DISTRIBUTION_ADDONS and extensions.installDistributionAddons?
http://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsXREDirProvider.cpp#572

>+const PREF_BRANCH_INSTALLED_ADDON     = "extensions.installedAddon.";
Perhaps extensions.installedDistroAddon. or similar

> 
> const URI_EXTENSION_UPDATE_DIALOG     = "chrome://mozapps/content/extensions/update.xul";
> 
> const DIR_EXTENSIONS                  = "extensions";
> const DIR_STAGE                       = "staged";
> const DIR_XPI_STAGE                   = "staged-xpis";
> const DIR_TRASH                       = "trash";
> 

>@@ -166,40 +169,43 @@ var gIDTest = /^(\{[0-9a-f]{8}-[0-9a-f]{
>  * A safe way to move a file or the contents of a directory to a new directory.
>  * The move is performed recursively and if anything fails an attempt is made to
>  * rollback the entire operation. The operation may also be rolled back to its
>  * original state after it has completed by calling the rollback method.
>  *
>  * Moves can be chained. Calling move multiple times will remember the whole set
>  * and if one fails all of the move operations will be rolled back.
nit: update the comment now that it also does copy

>  */
>-function SafeMoveOperation() {
>+function SafeInstallOperation() {
>   this._movedFiles = [];
nit: _installedFiles

>   this._createdDirs = [];
> }

>@@ -1797,16 +1840,139 @@ var XPIProvider = {
>         // Non-critical, just saves some perf on startup if we clean this up.
>         LOG("Error removing staging dir " + stagingDir.path, e);
>       }
>     });
>     return changed;
>   },
> 
>   /**
>+   * Installs any add-ons bundled with the application into the profile unless
"Installs add-ons located in the distribution directory's extensions directory..." or something similar

>+   * a newer version already exists or the user has previously uninstalled the
>+   * bundled add-on.
>+   *
>+   * @param  aManifests
>+   *         A dictionary to add new install manifests to to save having to
>+   *         reload them later
>+   * @return true if any new add-ons were installed
>+   */
>+  installBundledAddons: function XPI_installBundledAddons(aManifests) {
Same regarding distribution already having a bundle concept, etc.
Comment 20 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-18 16:49:18 PST
Comment on attachment 504857 [details] [diff] [review]
main patch rev 2

Just nits so r+ing with the pref to disable installing add-ons from distribution/extensions added to automation.py so if an app starts using this it doesn't bust existing tests. If at some point someone wants this turned on for tests other than just changing the pref for xpcshell tests they can first verify that all tests pass before enabling it.

Also, I think the UX team should be given a heads up since this will add another - better way IMO - for add-ons to be installed without user notification.
Comment 21 Robert Strong [:rstrong] (use needinfo to contact me) 2011-01-18 16:52:35 PST
btw: if you want me to take another look at the updated patch don't hesitate to request review
Comment 22 Dave Townsend [:mossop] 2011-01-19 13:58:14 PST
Created attachment 505184 [details] [diff] [review]
full patch
[Checked in: Comment 25]

Updated from comments and merged, ready to land after approval
Comment 23 Dave Townsend [:mossop] 2011-01-19 13:59:35 PST
This is a fairly well contained change in pretty well tested code and will give us the benefit of giving users full control over the Feedback add-on we ship with the next beta
Comment 24 Mike Beltzner [:beltzner, not reading bugmail] 2011-01-19 14:15:40 PST
Comment on attachment 505184 [details] [diff] [review]
full patch
[Checked in: Comment 25]

a=beltzner
Comment 25 Dave Townsend [:mossop] 2011-01-19 14:43:15 PST
Landed: http://hg.mozilla.org/mozilla-central/rev/9d41d0fa20ad
Comment 26 Danil Ivanov 2011-01-21 01:39:53 PST
diff --git a/toolkit/mozapps/extensions/test/xpcshell/test_distribution.js b/toolkit/mozapps/extensions/test/xpcshell/test_distribution.js
--- a/toolkit/mozapps/extensions/test/xpcshell/test_distribution.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_distribution.js
@@ -63,17 +63,17 @@ function setOldModificationTime() {
   // changes will be detected
   shutdownManager()
   let extension = gProfD.clone();
   extension.append("extensions");
   if (Services.prefs.getBoolPref("extensions.alwaysUnpack"))
     extension.append("addon1@tests.mozilla.org");
   else
     extension.append("addon1@tests.mozilla.org.xpi");
-  setExtensionModifiedTime(extension, Date.now - 10000);
+  setExtensionModifiedTime(extension, Date.now() - 10000);
   startupManager(false);
 }
 
 function run_test() {
   do_test_pending();
 
   run_test_1();
 }
Comment 27 Henrik Skupin (:whimboo) 2011-01-24 11:11:36 PST
Tested all possible paths, including upgrades from 3.6.x. Marking as verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b10pre) Gecko/20110121 Firefox/4.0b10pre
Comment 28 Kev Needham [:kev] 2011-02-10 06:09:56 PST
Quick question on this - does installation with this method support xpi expansion set in the XPI's install.rdf? Noticing with a couple partner bundles that it doesn't appear to, and I wanted to know if the always unpack pref needed to be set.
Comment 29 Henrik Skupin (:whimboo) 2011-02-10 06:17:23 PST
Kev, you mean that extracted XPI's in a former version of Firefox, do not get the newest version of the add-on installed into the users profile? I have tested this path and it worked fine for me. Do you have some examples?
Comment 30 Kev Needham [:kev] 2011-02-10 06:27:05 PST
Sorry, not very clear, there.

I've included an .xpi in the /core/distribution/extensions directory of the win32 Fx4 installer. in that xpi, the install.rdf has <em:expand>true</em:expand> set. the xpi is installed to the profile directory, but does not appear to expand properly (the xpi is installed, but needed data directories aren't created). I can mail you a sample.
Comment 31 Henrik Skupin (:whimboo) 2011-02-10 06:36:19 PST
(In reply to comment #30)
> I've included an .xpi in the /core/distribution/extensions directory of the
> win32 Fx4 installer. in that xpi, the install.rdf has
> <em:expand>true</em:expand> set. the xpi is installed to the profile directory,

I assume you want to use "<em:unpack>true</em:unpack>"?
Comment 32 Kev Needham [:kev] 2011-02-10 06:53:04 PST
Yes, I do, and I had expansion on the brain when I typed that. I do use "<em:unpack>true</em:unpack>". Apologies for introducing more confusion.
Comment 33 Henrik Skupin (:whimboo) 2011-02-10 07:20:27 PST
So yes please send me a sample I can use for testing, including the steps. If it's valid we should file a new bug, or do it immediately if you want.
Comment 34 Dave Townsend [:mossop] 2011-02-10 10:09:52 PST
(In reply to comment #28)
> Quick question on this - does installation with this method support xpi
> expansion set in the XPI's install.rdf? Noticing with a couple partner bundles
> that it doesn't appear to, and I wanted to know if the always unpack pref
> needed to be set.

As mentioned in comment 8 it currently ignores em:unpack. If the extension needs to be unpacked then it should be unpacked when shipped.
Comment 35 Eric Shepherd [:sheppy] 2011-03-08 10:00:41 PST
What here in this bug affects documentation? It's not immediately clear to me what new API is introduced here, or if it's just changes to existing stuff (and, for that matter, I'm not sure if this might have already been documented).
Comment 36 Dave Townsend [:mossop] 2011-03-08 10:30:32 PST
(In reply to comment #35)
> What here in this bug affects documentation? It's not immediately clear to me
> what new API is introduced here, or if it's just changes to existing stuff
> (and, for that matter, I'm not sure if this might have already been
> documented).

Similar to the documented global locations for installing extensions (https://developer.mozilla.org/en/Installing_extensions) we now have a new location at <appdir>/distribution/extensions and any extensions here get installed into a user's profile when they first run with that version of the application assuming they have never uninstalled that extension in the past and they don't have a newer version already present in their profile.
Comment 37 Eric Shepherd [:sheppy] 2011-03-10 06:16:31 PST
Documentation:

https://developer.mozilla.org/En/Developer_Guide/Customizing_Firefox

And linked to from:

https://developer.mozilla.org/en/Installing_extensions#Bundling_extensions_with_a_custom_Firefox

And mentioned on Firefox 4 for developers.

Note You need to log in before you can comment on or make changes to this bug.