Bug 747077 - Addon install module #406

Merged
merged 2 commits into from Apr 24, 2012

3 participants

@ochameau
Mozilla member

I refactored the addon-install module coming from addon builder in order to use promises.
I faced similar issue to http and request unit test where we need a reference to a test file.
Now that XPI are packed, we can't get a file reference to data file, but only URLs.
Here I needed to get a file reference to a test addon.
I've included a "tmp file" module. I can factored it out if it help.

@Gozala Gozala and 1 other commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/install.js
+
+const { Cc, Ci, Cu } = require("chrome");
+const { AddonManager } = Cu.import("resource://gre/modules/AddonManager.jsm");
+const { defer } = require("api-utils/promise");
+
+/**
+ * `install` method error codes:
+ *
+ * https://developer.mozilla.org/en/Addons/Add-on_Manager/AddonManager#AddonInstall_errors
+ */
+exports.ERRORS = Object.freeze({
+ NETWORK_FAILURE: AddonManager.ERROR_NETWORK_FAILURE,
+ INCORRECT_HASH: AddonManager.ERROR_INCORRECT_HASH,
+ CORRUPT_FILE: AddonManager.ERROR_CORRUPT_FILE,
+ FILE_ACCESS: AddonManager.ERROR_FILE_ACCESS
+});
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

I think you can export them directly, no need for ERRORS

@ochameau
Mozilla member

I hesitated between both, looks like the simple choice is better :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/install.js
+
+/**
+ * Immediatly install an addon.
+ *
+ * @param {String} xpiPath
+ * file path to an xpi file to install
+ * @return {Promise}
+ * A promise resolved when the addon is finally installed.
+ * Resolved with addon id as value or rejected with an error code.
+ */
+exports.install = function install(xpiPath) {
+ let deferred = defer();
+
+ // Create nsIFile for the xpi file
+ let file = Cc['@mozilla.org/file/local;1'].createInstance(Ci.nsILocalFile);
+ try {
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

Alternatively you could just warp install function into promised, that way exceptions would've just propagate and reject a promise.

@ochameau
Mozilla member

But it will reject with an nsIFile exception instead of a normalized addon manager error code (here: ERROR_FILE_ACCESS) ?

@Gozala
Mozilla member
Gozala added a note Apr 23, 2012

You're right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/install.js
+ INCORRECT_HASH: AddonManager.ERROR_INCORRECT_HASH,
+ CORRUPT_FILE: AddonManager.ERROR_CORRUPT_FILE,
+ FILE_ACCESS: AddonManager.ERROR_FILE_ACCESS
+});
+
+/**
+ * Immediatly install an addon.
+ *
+ * @param {String} xpiPath
+ * file path to an xpi file to install
+ * @return {Promise}
+ * A promise resolved when the addon is finally installed.
+ * Resolved with addon id as value or rejected with an error code.
+ */
+exports.install = function install(xpiPath) {
+ let deferred = defer();
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

I find let { promise, resolve, reject } = defer(); much better, but I don't mind this either.

@ochameau
Mozilla member

Switched to this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/install.js
+ return;
+ AddonManager.removeAddonListener(listener);
+ deferred.resolve();
+ }
+ };
+ AddonManager.addAddonListener(listener);
+
+ // Order Addonmanager to uninstall the addon
+ AddonManager.getAddonByID(addonId, function (addon) {
+ addon.uninstall();
+ });
+
+ return deferred.promise;
+};
+
+exports.disable = function disable(addonId, callback) {
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

you don't need callback argument here.

@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

I would wrap this disable into promised as well so addonId could be a promise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/install.js
+ },
+ onDownloadFailed: function(aInstall) {
+ this.onInstallFailed(aInstall);
+ }
+ };
+
+ // Order AddonManager to install the addon
+ AddonManager.getInstallForFile(file, function(install) {
+ install.addListener(listener);
+ install.install();
+ });
+
+ return deferred.promise;
+};
+
+exports.uninstall = function uninstall(addonId) {
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

I would wrap this one into promised, so that following could work as well:

var pid = installer.install(path);
if (isFriday)
  installer.disable(pid);

So I don't have to wait for install to succeed to chain.

@ochameau
Mozilla member

Even if I see the value of using promised on all these methods,
I would prefer introducing it only when we are going to hit such usecase.
It feels weird to wrap all methods with such magic method.
(In my PoV it would complexify debugging/comprehension/documentation for simple cases)

@Gozala
Mozilla member
Gozala added a note Apr 23, 2012

Sure deferring that until necessary is a very reasonable approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 1 other commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/addon/install.js
@@ -0,0 +1,99 @@
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

I don't like naming modules in verbs, seems kind of strange. also install.install / install.disable is strange. Could you follow same convention as we did with loader ? addon/runner addon/installer

@ochameau
Mozilla member

Renamed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/file/tmp.js
@@ -0,0 +1,71 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+const { Cc, Ci } = require("chrome");
+
+const system = require("api-utils/system");
+const file = require("api-utils/file");
+const unload = require("api-utils/unload");
+
+const tmpDir = require("system").pathFor("TmpD");
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

You should put some comment about "TmpD" string it's far from obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala and 2 others commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/file/tmp.js
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+"use strict";
+
+const { Cc, Ci } = require("chrome");
+
+const system = require("api-utils/system");
+const file = require("api-utils/file");
+const unload = require("api-utils/unload");
+
+const tmpDir = require("system").pathFor("TmpD");
+
+// List of all tmp file created
+let files = [];
+
+// Remove all tmp files on addon disabling
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

I thought whole point of system tmp dirs is that system cleans them up itself so you don't have to doi manually.

@ochameau
Mozilla member

It allows to clean it up on firefox quit instead of OS shutdown/reboot.
On Windows I'm not even sure it is emptied on restart :/

@Yoric
Yoric added a note Jul 27, 2012

It's not consistently emptied on Windows or, even worse, Android.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/tests/test-addon-install.js
+
+ if (iteration++ < 3) {
+ loop();
+ }
+ else {
+ observers.remove("addon-install-unit-test", eventsObserver);
+ test.done();
+ }
+ }
+ function onFailure(code) {
+ test.fail("Install failed: "+code);
+ observers.remove("addon-install-unit-test", eventsObserver);
+ test.done();
+ }
+
+ function loop() {
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

I think next is a better name here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on an outdated diff Apr 19, 2012
packages/api-utils/lib/file/tmp.js
@@ -0,0 +1,71 @@
@Gozala
Mozilla member
Gozala added a note Apr 19, 2012

Lots's of sync IO here, if that's just for test that's fine, but than it should live in test folder.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala
Mozilla member

Could you please move the tmp.js either to tests or to test-harness. Also .xpi file and temp file have being added to data folder which I'd like to avoid. Could you just create fixtures folder in tests and put them there ? You'll be able to get uri to those files by resolving it relative to module.uri that way we can start deprecating data folders in non add-on code.

@ochameau
Mozilla member

I hesitated to put test data files (xpi and txt file) into subfolder, should I? (Now, they are in tests folder)
I moved tmp to harness and rename it to tmp-file.

@Gozala
Mozilla member

r+

P.S.: Associated bug has wrong pull request attached to it.

@Gozala
Mozilla member

I have removed wrong pull request from bug and have added this one instead with r+

@Gozala
Mozilla member

I hesitated to put test data files (xpi and txt file) into subfolder, should I? (Now, they are in tests folder)
I moved tmp to harness and rename it to tmp-file.

I meant putting files into instead of right in tests:
https://github.com/mozilla/addon-sdk/tree/master/packages/api-utils/tests/fixtures

Maybe you can move before landing.

@gregglind
Mozilla member

Timely! I was just settling in to do this for TestPilot 2

Our needs:

1) instsall from from file or url

2) audit trail:

* keep track of which ones got installed, in such a way that they can be uninstalled (at the end of an experiment).
* know (if possible), how it got installed.
@ochameau
Mozilla member

@gozala something like this?

@Gozala
Mozilla member

Yeap exactly that!

@ochameau ochameau merged commit 677a214 into mozilla:master Apr 24, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment