Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Refactor helper in order to work with new loader released in SDK 1.4. #3

Merged
merged 3 commits into from

3 participants

@ochameau
Owner

Simplify implementation by using: PageMod and AddonManager.
New asynchronous API using promise pattern.

ochameau added some commits
@ochameau ochameau Refactor helper in order to work with new loader released in SDK 1.4.
Simplify implementation by using: PageMod and AddonManager.
New asynchronous API using promise pattern.
37b1082
@ochameau ochameau Split ABH main module in multiple modules:
 - addon-install which implements simple addon file install,
 - file-utils that handle some FS-related action, mainly due to packed XPIs.
+ Removed `directory-service` that is useless thanks to require(system).pathFor.
+ Fix unregistered listeners in `AddonInstall`.
+ Added new license header.
8c9f280
@ochameau ochameau Stop using Namespace as `addon` private is collected. 7638093
@csuwildcat csuwildcat merged commit 7da28e4 into from
@Gozala Gozala commented on the diff
lib/addon-install.js
@@ -0,0 +1,76 @@
+/* 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/. */
+
+const { Cc, Ci, Cu } = require('chrome');
+const { AddonManager } = Cu.import("resource://gre/modules/AddonManager.jsm");
+
+const { Base } = require("api-utils/base");
+
+/**
+ * Class to manage an addon: install and uninstall it.
+ */
@Gozala Owner
Gozala added a note

Ok it took me a while to get my head around this class. I find it very confusing to have a class based API for this. I think it's complicated I don't really see a benefit. I think you should just export simple function from this module: install and unload.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff
lib/addon-install.js
@@ -0,0 +1,76 @@
@Gozala Owner
Gozala added a note

I think it would make sense to move this module to the sdk itself and consume it from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff
lib/addons-builder-helper.js
((59 lines not shown))
- }
- };
+/**
+ * Utility method to toggle the XUL JS Console.
+ * Return `message` attribute send back to webpage.
+ */
+let consoleWindow = null;
+function toggleConsoleWindow(command) {
+ consoleWindow = windowManager.getMostRecentWindow('global:console');
+ switch(command) {
+ case 'open':
+ consoleWindow = consoleWindow ? consoleWindow.focus() :
+ windowManager.getMostRecentWindow(null)
+ .open('chrome://global/content/console.xul',
+ '_blank',
+ 'chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar');
@Gozala Owner
Gozala added a note

I think we should use SDK based high / low level APIs instead, and if they are not good enough let's improve them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff
lib/addons-builder-helper.js
((72 lines not shown))
+ .open('chrome://global/content/console.xul',
+ '_blank',
+ 'chrome,extrachrome,menubar,resizable,scrollbars,status,toolbar');
+ break;
+ case 'close':
+ consoleWindow = consoleWindow ? consoleWindow.close() : null;
+ break;
+ case 'isOpen':
+ return consoleWindow ? true : false;
+ break;
+ default:
+ return 'An unrecognized command was passed through the "contents" ' +
+ 'property of the mozFlightDeck send object. Available commands ' +
+ 'are "open", "close", and "isOpen"';
+ }
+ return null;
}
@Gozala Owner
Gozala added a note

I find it confusing that this is called toggleConsoleWindow taking a string argument defining what it actually should do, how is that a toggling 0_o ?

Why not just expose individual functions: isOpen, close, open. Would be be way more intuitive IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff
lib/addons-builder-helper.js
((211 lines not shown))
var self = this;
var currExtension = null;
- self.__defineGetter__(
- "isInstalled",
- function isInstalled() {
- return (currExtension != null);
- });
+ return {
+ get isInstalled() {
+ return (currExtension != null && currExtension.isInstalled);
@Gozala Owner
Gozala added a note

this could be more correct and easier to read return currExtension && currExtension.isInstalled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff
lib/addons-builder-helper.js
((223 lines not shown))
- self.__defineGetter__(
- "installedID",
- function installedID() {
+ get installedID() {
if (!currExtension)
return null;
return currExtension.id;
@Gozala Owner
Gozala added a note

simpler form would be: return currExtension && currExtension.id

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Gozala Gozala commented on the diff
lib/addons-builder-helper.js
((25 lines not shown))
if (typeof(pref) == "string") {
- try {
+ try {
config.trustedOrigins = config.trustedOrigins.concat(pref.split(","));
} catch (e) {
console.log("Error when reading preference " + CONFIG_PREF);
@Gozala Owner
Gozala added a note

I guess console.error would be better here.

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

Also as a general note, it feels funny that we expose promised based API to the content but then we do intensive callback based compositions on the add-on side. I think we need to add minimal promise support to the SDK, it would make this code base much simpler. That being said, it's a different issue not really related to this change.

@ochameau ochameau referenced this pull request from a commit in ochameau/addon-builder-helper
@ochameau ochameau Address review comments made in PR #3.
+ add file-utils unit-test.
0001176
@ochameau ochameau referenced this pull request from a commit in ochameau/addon-builder-helper
@ochameau ochameau Address review comments made in PR #3.
+ use new api-utils/addon/installer module.
b078d16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 23, 2012
  1. @ochameau

    Refactor helper in order to work with new loader released in SDK 1.4.

    ochameau authored
    Simplify implementation by using: PageMod and AddonManager.
    New asynchronous API using promise pattern.
  2. @ochameau

    Split ABH main module in multiple modules:

    ochameau authored
     - addon-install which implements simple addon file install,
     - file-utils that handle some FS-related action, mainly due to packed XPIs.
    + Removed `directory-service` that is useless thanks to require(system).pathFor.
    + Fix unregistered listeners in `AddonInstall`.
    + Added new license header.
  3. @ochameau
Something went wrong with that request. Please try again.