Skip to content
This repository

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Bug 746131: CFX in JS - Use an SDK addon called by cfx in python that is going to install the xpi #410

Closed
wants to merge 19 commits into from

4 participants

ochameau Irakli Gozalishvili Erik Vold Brian Warner
ochameau
Collaborator

Still a work in progress but feedback is welcomed.
(Based on top of PR #406)
https://bugzilla.mozilla.org/show_bug.cgi?id=746131

cfx/cfx.js
((19 lines not shown))
  19
+}
  20
+
  21
+function main() {
  22
+  let options = getOptions();
  23
+
  24
+  console.log("Install addon at : "+options.xpiPath);
  25
+  AddonInstaller.install(options.xpiPath).then(function () {
  26
+    console.log("Addon installed");
  27
+  });
  28
+}
  29
+
  30
+try {
  31
+  main();
  32
+}
  33
+catch(e) {
  34
+  console.log("CFX.js exception:\n" + e);
1
Irakli Gozalishvili Collaborator
Gozala added a note April 24, 2012

maybe console.exception probably would be better

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/cfx.js
... ...
@@ -0,0 +1,36 @@
  1
+const AddonInstaller = require("api-utils/addon/installer");
  2
+const file = require("api-utils/file");
  3
+const system = require("api-utils/system");
  4
+
  5
+function getOptions() {
  6
+  let optionsFile = system.env["CFX_OPTIONS_FILE"];
  7
+  if (!optionsFile) {
  8
+    throw new Error("Unable to locate options file, environnement not set.");
  9
+  }
  10
+  let data = null;
  11
+  try {
  12
+    data = file.read(optionsFile);
3
Irakli Gozalishvili Collaborator
Gozala added a note April 24, 2012

Maybe we should read this async as well (XHR) ?

Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Has not @ZER0 landed a readURI util function yet ?

ochameau Collaborator
ochameau added a note June 13, 2012

Still nothing, bug 748086.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili
Collaborator

I think it would be useful to add Readme.md to cfx folder explaining what it dose or how to use it. I'm not sure I understand how how to cause cfx to use it.

cfx/cfx.js
((16 lines not shown))
  16
+  }
  17
+  console.log("got options: "+data);
  18
+  return JSON.parse(data);
  19
+}
  20
+
  21
+function main() {
  22
+  let options = getOptions();
  23
+
  24
+  console.log("Install addon at : "+options.xpiPath);
  25
+  AddonInstaller.install(options.xpiPath).then(function () {
  26
+    console.log("Addon installed");
  27
+  });
  28
+}
  29
+
  30
+try {
  31
+  main();
1
Irakli Gozalishvili Collaborator
Gozala added a note April 24, 2012

Might be worth doing following instead:

if (require.main === module) main()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xulrunner-app/CommandLineHandler.js
... ...
@@ -0,0 +1,83 @@
  1
+/* This Source Code Form is subject to the terms of the Mozilla Public
2
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Currently established convention for module names is hypen-delimited-name.js let's stick to it please.

Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Ok, now I see that it's not really a module, but still not sure it's a good enough reason to don't follow conventions. Although I'm ok with having it as is if you feel very strongly about it (in that case probably .jsm would make more sense though)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
... ...
@@ -0,0 +1,101 @@
  1
+cfx.js
  2
+======
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Nit: We usually use # H1 instead of

H1
==
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
... ...
@@ -0,0 +1,101 @@
  1
+cfx.js
  2
+======
  3
+
  4
+cfx.js is an SDK addon meant to replace python command line application.
  5
+This addon is currently quite limited but it is going to implement all features
  6
+of cfx python application in order to remove python dependency.
  7
+It currently just install an XPI file and run it into the Firefox.
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

into -> in a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
... ...
@@ -0,0 +1,101 @@
  1
+cfx.js
  2
+======
  3
+
  4
+cfx.js is an SDK addon meant to replace python command line application.
  5
+This addon is currently quite limited but it is going to implement all features
  6
+of cfx python application in order to remove python dependency.
  7
+It currently just install an XPI file and run it into the Firefox.
  8
+For now, it is not meant to be used standalone, it is only an utility addon
  9
+used by cfx python application.
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

I'd say helper addon used by an add-on sdk.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((13 lines not shown))
  13
+
  14
+ * Activate your SDK environnement
  15
+
  16
+        $ source bin/activate (linux/mac)
  17
+           - or -
  18
+        $ bin\activate (window)
  19
+
  20
+ * Go to cfx.js directory
  21
+
  22
+        $ cd cfx/
  23
+
  24
+ * Generate cfx.js xpi
  25
+
  26
+        $ cfx xpi
  27
+
  28
+ * Eventually copy it to the xulrunner application template
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Either title or this section is confusing. Is it embedding into app ?
Or do you mean install instructions ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((25 lines not shown))
  25
+
  26
+        $ cfx xpi
  27
+
  28
+ * Eventually copy it to the xulrunner application template
  29
+
  30
+        $ mv cfx.xpi xulrunner-app/
  31
+
  32
+How does it work
  33
+================
  34
+
  35
+ * Set an environnement variable called `CFX_OPTIONS_FILE`
  36
+   with an absolute path to a JSON file that contains data needed for actions
  37
+   to be accomplished by this addon
  38
+ * Run firefox with this addon installed, or run it through the xulrunner
  39
+   template.
  40
+ * The addon retrieve that JSON object and execute actions based on these data
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Maybe: The addon reads JSON from CFX_OPTIONS_FILE and performs tasks described by it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((36 lines not shown))
  36
+   with an absolute path to a JSON file that contains data needed for actions
  37
+   to be accomplished by this addon
  38
+ * Run firefox with this addon installed, or run it through the xulrunner
  39
+   template.
  40
+ * The addon retrieve that JSON object and execute actions based on these data
  41
+
  42
+(We aren't putting JSON content into environnement variable as its size is
  43
+ limited)
  44
+
  45
+Expected JSON Data passed to cfx.js
  46
+===================================
  47
+
  48
+    {
  49
+      command: "string" // Name of the command to execute
  50
+      options: "object" // Arguments needed to execute this command
  51
+    }
2
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Is it a key value map ?

Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Could you write some other (real) example this one is very confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((38 lines not shown))
  38
+ * Run firefox with this addon installed, or run it through the xulrunner
  39
+   template.
  40
+ * The addon retrieve that JSON object and execute actions based on these data
  41
+
  42
+(We aren't putting JSON content into environnement variable as its size is
  43
+ limited)
  44
+
  45
+Expected JSON Data passed to cfx.js
  46
+===================================
  47
+
  48
+    {
  49
+      command: "string" // Name of the command to execute
  50
+      options: "object" // Arguments needed to execute this command
  51
+    }
  52
+
  53
+Available commands
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Supported commands

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((45 lines not shown))
  45
+Expected JSON Data passed to cfx.js
  46
+===================================
  47
+
  48
+    {
  49
+      command: "string" // Name of the command to execute
  50
+      options: "object" // Arguments needed to execute this command
  51
+    }
  52
+
  53
+Available commands
  54
+==================
  55
+
  56
+== install-xpi ==
  57
+
  58
+   Install a given xpi into the currently running firefox/xulrunner application
  59
+
  60
+   Expected options:
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Required options

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((47 lines not shown))
  47
+
  48
+    {
  49
+      command: "string" // Name of the command to execute
  50
+      options: "object" // Arguments needed to execute this command
  51
+    }
  52
+
  53
+Available commands
  54
+==================
  55
+
  56
+== install-xpi ==
  57
+
  58
+   Install a given xpi into the currently running firefox/xulrunner application
  59
+
  60
+   Expected options:
  61
+   {
  62
+    xpiPath: "string" // Absolute path to the xpi to install
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Would be nice if we could stick to same convention either "xpi-path" or "installXpi". Also I think it could be just path here as install-xpi makes it quite obvious.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((41 lines not shown))
  41
+
  42
+(We aren't putting JSON content into environnement variable as its size is
  43
+ limited)
  44
+
  45
+Expected JSON Data passed to cfx.js
  46
+===================================
  47
+
  48
+    {
  49
+      command: "string" // Name of the command to execute
  50
+      options: "object" // Arguments needed to execute this command
  51
+    }
  52
+
  53
+Available commands
  54
+==================
  55
+
  56
+== install-xpi ==
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Nit: Not sure it's even valid markdown we usually use

## install-xpi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((57 lines not shown))
  57
+
  58
+   Install a given xpi into the currently running firefox/xulrunner application
  59
+
  60
+   Expected options:
  61
+   {
  62
+    xpiPath: "string" // Absolute path to the xpi to install
  63
+   }
  64
+
  65
+== build-xpi ==
  66
+
  67
+  Build an addon SDK addon.
  68
+
  69
+  Expected options:
  70
+  {
  71
+    xpiPath: "string" // Absolute path to the xpi we want to build
  72
+    harnessRootDir: "string" // Absolute path to the temaplate folder.
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

can it be just template ? or template-path / templatePath

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((60 lines not shown))
  60
+   Expected options:
  61
+   {
  62
+    xpiPath: "string" // Absolute path to the xpi to install
  63
+   }
  64
+
  65
+== build-xpi ==
  66
+
  67
+  Build an addon SDK addon.
  68
+
  69
+  Expected options:
  70
+  {
  71
+    xpiPath: "string" // Absolute path to the xpi we want to build
  72
+    harnessRootDir: "string" // Absolute path to the temaplate folder.
  73
+                             // All files from this folder are going to be
  74
+                             // written in the xpi
  75
+    manifest: "string" // Content of the `install.rdf` to write in the xpi
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Can we call it something else but manifest ? It's pretty confusing since we'll be call manifest something completely different. Maybe install-rdf or something like that ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((61 lines not shown))
  61
+   {
  62
+    xpiPath: "string" // Absolute path to the xpi to install
  63
+   }
  64
+
  65
+== build-xpi ==
  66
+
  67
+  Build an addon SDK addon.
  68
+
  69
+  Expected options:
  70
+  {
  71
+    xpiPath: "string" // Absolute path to the xpi we want to build
  72
+    harnessRootDir: "string" // Absolute path to the temaplate folder.
  73
+                             // All files from this folder are going to be
  74
+                             // written in the xpi
  75
+    manifest: "string" // Content of the `install.rdf` to write in the xpi
  76
+    harnessOptions: {
4
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

harnessOptions was pretty unfortunate name, that we always wanted to change. Maybe we can fix this now ?

ochameau Collaborator
ochameau added a note June 13, 2012

I'm fine renaming this to manifest?
But I don't think it really worth modifying the final name of harness-options.json file in xpi.

Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

I'm fine renaming this to manifest?
But I don't think it really worth modifying the final name of harness-options.json file in xpi.

That's a different discussion, and I'd like to learn you're argument, but let's defer it for the future.

ochameau Collaborator
ochameau added a note June 28, 2012

Some scripts in the wild are unpacking xpi files and are expecting to find this manifest file.
https://github.com/mozilla/amo-validator
I wouldn't be surprised if these guys are reading it:
https://bugzilla.mozilla.org/show_bug.cgi?id=730776#c0
Flightdeck team are most likely playing with it.
There is a chance to break stuff in the wild although it doesn't add a big value to rename this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/cfx.js
((22 lines not shown))
  22
+  let { command, options } = getOptions();
  23
+  if (command === "install-xpi") {
  24
+    AddonInstaller.install(options.xpiPath)
  25
+                  .then(
  26
+                    null,
  27
+                    function onError(error) {
  28
+                      console.log("Failed to install addon: " + error);
  29
+                    });
  30
+  }
  31
+  else if (command === "build-xpi") {
  32
+    xpi.build(options);
  33
+  }
  34
+  else if (command == "no-quit") {
  35
+    // Test command in order to simulate a run that never quits
  36
+    let { Cu } = require("chrome");
  37
+    let { Services } = Cu.import("resource://gre/modules/Services.jsm");
2
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Could you please move imports to the top of the file ? I think you're trying to optimize here, but I think that JSM will be loaded anyway and require chrome is cheap.

Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

Also I think it would be much simpler and clear to use a service instead:

Cc['@mozilla.org/toolkit/app-startup;1'].
  getService(Ci.nsIAppStartup).
  enterLastWindowClosingSurvivalArea()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/cfx.js
((7 lines not shown))
  7
+  let optionsFile = system.env["CFX_OPTIONS_FILE"];
  8
+  if (!optionsFile) {
  9
+    throw new Error("Unable to locate options file, environnement not set.");
  10
+  }
  11
+  let data = null;
  12
+  try {
  13
+    data = file.read(optionsFile);
  14
+  }
  15
+  catch(e) {
  16
+    throw new Error("Unable to read options file: " + optionsFile + "\n" + e);
  17
+  }
  18
+  return JSON.parse(data);
  19
+}
  20
+
  21
+function main() {
  22
+  let { command, options } = getOptions();
5
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

I think we should read files async, which could be delayed for the future, but API should be adjusted accordingly.

ochameau Collaborator
ochameau added a note June 13, 2012

I'd prefer waiting for such asynchronous file API to be here before changing the layout of this code. It should be straightforward to do later.

Irakli Gozalishvili Collaborator
Gozala added a note June 13, 2012

The problem is not weather it will be simple change or not, problem is an API breakage, that's why I prefer to start with async API from the beginning.

ochameau Collaborator
ochameau added a note June 13, 2012

There is no API exposed here. Nothing is exported out of cfx.js. Am I misunderstanding something?

Irakli Gozalishvili Collaborator
Gozala added a note June 13, 2012

Right, I have not realized that :) In that case sure feel free to keep as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xulrunner-app/CommandLineHandler.js
((48 lines not shown))
  48
+    // data object sent to bootstrap's startup method:
  49
+    // An object crafted by AddonManager code, but jetpack only uses
  50
+    // resourceURI attribute
  51
+    let data = {
  52
+      resourceURI: xpiJarURI
  53
+    };
  54
+
  55
+    // load reason code, always use 'startup'
  56
+    let reasonCode = 1;
  57
+
  58
+    // Fake AddonManager behavior by calling startup method
  59
+    bootstrap.startup(data, reasonCode);
  60
+
  61
+    // Fake 'final-ui-startup' which is expected to fire in
  62
+    // `api-utils/addon/runner.js`
  63
+    Services.obs.notifyObservers(null, "final-ui-startup", null);
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

I'm not sure it's a good idea. I think we should dispatch our custom notification and handle it from addon/runner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xulrunner-app/CommandLineHandler.js
... ...
@@ -0,0 +1,83 @@
  1
+/* This Source Code Form is subject to the terms of the Mozilla Public
  2
+ * License, v. 2.0. If a copy of the MPL was not distributed with this file,
  3
+ * You can obtain one at http://mozilla.org/MPL/2.0/. */
  4
+
  5
+// This XPCOM components allows to run a sdk addon as a xulrunner application
  6
+
  7
+const Cc = Components.classes;
  8
+const Ci = Components.interfaces;
  9
+const Cu = Components.utils;
  10
+
  11
+Cu.import("resource://gre/modules/XPCOMUtils.jsm");
  12
+Cu.import("resource://gre/modules/Services.jsm");
  13
+
  14
+/*
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

It's little confusing why do you you use https://developer.mozilla.org/en/XPCOM_Interface_Reference/nsICommandLine for this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xulrunner-app/application.ini
... ...
@@ -0,0 +1,11 @@
  1
+[App]
  2
+Vendor=Mozilla
  3
+Name=CFX in Javascript
1
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012

I think "Cuddlefish" would make more sense here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xulrunner-app/chrome.manifest
... ...
@@ -0,0 +1,3 @@
  1
+component {537df286-d9ae-4c7a-a633-6266b1325289} CommandLineHandler.js
  2
+contract @mozilla.org/stdoutapp/clh;1 {537df286-d9ae-4c7a-a633-6266b1325289}
  3
+category command-line-handler x-default @mozilla.org/stdoutapp/clh;1
3
Irakli Gozalishvili Collaborator
Gozala added a note June 06, 2012
ochameau Collaborator
ochameau added a note June 13, 2012

I'm not following you here?
In any case, according to this documentation I changed x-default to m-cfx.
And used a more specific xpcom name @mozilla.org/cfx/clh;1

Irakli Gozalishvili Collaborator
Gozala added a note June 13, 2012

I thought x-deafult was a special entry that was not documented on MDN, so I suggested to document it. But m-cfx is fine as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ochameau
Collaborator

I've tried to address all your comments, except the one I replied to.
And I forgot to commit test file in the previous revision ...

cfx/README.md
((53 lines not shown))
  53
+
  54
+### install-xpi
  55
+
  56
+   Install a given xpi into the currently running firefox/xulrunner application
  57
+
  58
+   Required `options` object:
  59
+   {
  60
+     path: "string" // Absolute path to the xpi to install
  61
+   }
  62
+
  63
+### build-xpi
  64
+
  65
+  Build an addon SDK addon.
  66
+
  67
+  Required `options` object:
  68
+  {
4
Irakli Gozalishvili Collaborator
Gozala added a note June 13, 2012

You're still mixing two naming conventions in the single JSON, please stick to either camel case or dash delimited naming convention. The later one seems to be more popular in JSON so I'd prefer that one.

ochameau Collaborator
ochameau added a note June 13, 2012

I used camel case in order to be able to use destructuring and/or regular attribute access like option.xpiPath/let { xpiPath } = options;, instead of only options["xpi-path"]. Having said that I'm fine about dash-delimited too.

Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I see why you used camel case and it's convenient, but I still think that consistency is more important. So if you don't mind let's go with either dash-delimited keys or single word names. I think you could just use xpi, template, rdf as names.

ochameau Collaborator
ochameau added a note June 26, 2012

Renamed to dash-delimited. I kept long names as they are explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
... ...
@@ -0,0 +1,99 @@
  1
+# cfx.js
  2
+
  3
+cfx.js is an SDK addon meant to replace python command line application.
  4
+This addon is currently quite limited but it is going to implement all features
  5
+of cfx python application in order to remove python dependency.
  6
+It currently just allow to build an XPI file.
  7
+For now, it is not meant to be used standalone, it is an helper addon used by
  8
+add-on sdk `cfx` command line application.
  9
+
  10
+
  11
+# How does it work
  12
+
  13
+ * cfx python application set `CFX_OPTIONS_FILE` environnement variable to an
  14
+   absolute path to a JSON file that contains necessary information to execute
  15
+   some commands,
2
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I don't think you want to end with , here.

ochameau Collaborator
ochameau added a note June 27, 2012

Actually I did. Isn't the same rule between french and english ?
When you have multiple bullets, you end up with coma for all bullets expect the last one, with a point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((6 lines not shown))
  6
+It currently just allow to build an XPI file.
  7
+For now, it is not meant to be used standalone, it is an helper addon used by
  8
+add-on sdk `cfx` command line application.
  9
+
  10
+
  11
+# How does it work
  12
+
  13
+ * cfx python application set `CFX_OPTIONS_FILE` environnement variable to an
  14
+   absolute path to a JSON file that contains necessary information to execute
  15
+   some commands,
  16
+ * Then, cfx python application run it through the xulrunner template,
  17
+ * Finally, the addon reads JSON from `CFX_OPTIONS_FILE` and performs tasks
  18
+   described by it.
  19
+
  20
+(We aren't putting JSON content into environnement variable as the size of an
  21
+ environnement variable is quite limited)
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I don't think such implementation details needs to be in readme. If you really want it, just add "(use JSON file since options may not fit in environment variable)" at the end of first bullet (line 15).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((46 lines not shown))
  46
+
  47
+    {
  48
+      command: "string" // Name of the command to execute
  49
+      options: "object" // Arguments needed to execute this command
  50
+    }
  51
+
  52
+## Supported commands
  53
+
  54
+### install-xpi
  55
+
  56
+   Install a given xpi into the currently running firefox/xulrunner application
  57
+
  58
+   Required `options` object:
  59
+   {
  60
+     path: "string" // Absolute path to the xpi to install
  61
+   }
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I think more complete examples would help a lot:

{
  "command": "install-xpi",
  "options": {
    "path": "/path/to/addon.xpi"
  }
}

Maybe also note what the path format will be for windows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((59 lines not shown))
  59
+   {
  60
+     path: "string" // Absolute path to the xpi to install
  61
+   }
  62
+
  63
+### build-xpi
  64
+
  65
+  Build an addon SDK addon.
  66
+
  67
+  Required `options` object:
  68
+  {
  69
+    xpiPath: "string" // Absolute path to the xpi we want to build
  70
+    templatePath: "string" // Absolute path to the temaplate folder.
  71
+                           // All files from this folder are going to be
  72
+                           // written in the xpi
  73
+    installRdf: "string" // Content of the `install.rdf` to write in the xpi
  74
+    harnessOptions: {
3
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Maybe we can change harnessOptions to metadata ?

ochameau Collaborator
ochameau added a note June 26, 2012

Do you mind renaming this old beast in the next step ? i.e. when we are going to build the manifest from JS side.
Just because it is called harness options all over the place in python, so I'd like to avoid using two different names in two sides, nor modifying the whole python code.

Irakli Gozalishvili Collaborator
Gozala added a note June 26, 2012

Sure, just was trying to come up with a better name, no rush though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/README.md
((82 lines not shown))
  82
+          "test": section, // section name => absolute path to it
  83
+          "lib": section
  84
+        }
  85
+      },
  86
+      locale: { // A dictionnary of available locales for this addon
  87
+                // keys are language code and values are another dictionnary
  88
+                // with all translated strings
  89
+        "en-US": { // language code
  90
+          "key": "translation" // key to translate => translated key
  91
+        }
  92
+      }
  93
+      // All these `harnessOptions` attributes are used to build the xpi
  94
+      // but you can pass other attributes. They will be written into
  95
+      // `harness-options.json` at xpi's root folder
  96
+    },
  97
+    extraHarnessOptions: "dictionnary" // A set of additional attributes to
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I think just extra would do here as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/cfx.js
... ...
@@ -0,0 +1,62 @@
  1
+const { Cc, Ci } = require("chrome");
  2
+const AddonInstaller = require("api-utils/addon/installer");
  3
+const file = require("api-utils/file");
  4
+const system = require("api-utils/system");
  5
+const xpi = require("./xpi");
  6
+const { CfxError } = require("./exception");
  7
+
  8
+function getOptions() {
  9
+  let optionsFile = system.env["CFX_OPTIONS_FILE"];
  10
+  if (!optionsFile) {
  11
+    throw new Error("Unable to locate options file, environnement not set.");
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

environnement -> environment

Also I'd say "environment variable is not set" instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/cfx.js
((3 lines not shown))
  3
+const file = require("api-utils/file");
  4
+const system = require("api-utils/system");
  5
+const xpi = require("./xpi");
  6
+const { CfxError } = require("./exception");
  7
+
  8
+function getOptions() {
  9
+  let optionsFile = system.env["CFX_OPTIONS_FILE"];
  10
+  if (!optionsFile) {
  11
+    throw new Error("Unable to locate options file, environnement not set.");
  12
+  }
  13
+  let data = null;
  14
+  try {
  15
+    data = file.read(optionsFile);
  16
+  }
  17
+  catch(e) {
  18
+    throw new Error("Unable to read options file: " + optionsFile + "\n" + e);
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Nit: We tend to just use Error without new.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/cfx.js
((10 lines not shown))
  10
+  if (!optionsFile) {
  11
+    throw new Error("Unable to locate options file, environnement not set.");
  12
+  }
  13
+  let data = null;
  14
+  try {
  15
+    data = file.read(optionsFile);
  16
+  }
  17
+  catch(e) {
  18
+    throw new Error("Unable to read options file: " + optionsFile + "\n" + e);
  19
+  }
  20
+  return JSON.parse(data);
  21
+}
  22
+
  23
+function main() {
  24
+  let { command, options } = getOptions();
  25
+  if (command === "install-xpi") {
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I would find it easier to read & maintain if it was something like:

commands[command] || commands.help(options)

Where commands is a dictionary of command functions that extract their options and do whatever they need to.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/cfx.js
((32 lines not shown))
  32
+  }
  33
+  else if (command === "build-xpi") {
  34
+    xpi.build(options);
  35
+  }
  36
+  else if (command == "no-quit") {
  37
+    // Test command in order to simulate a run that never quits
  38
+    Cc['@mozilla.org/toolkit/app-startup;1'].
  39
+      getService(Ci.nsIAppStartup).
  40
+      enterLastWindowClosingSurvivalArea();
  41
+  }
  42
+  else {
  43
+    console.log("Unknown cfxjs command '" + command + "'");
  44
+  }
  45
+}
  46
+
  47
+try {
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Also if you're ok with previous suggestion I would rather move try / catch into main itself so it will end up like:

function main() {
  try {
    let { command, options } = getOptions();
    commands[command] || commands.help(options)
  } catch (error) {
    if (error instanceof CfxError) {
      // Stacktrace isn't usefull in case of custom cfx exceptions,
      // prints custom message instead
      console.log(e);
    }
    else {
      console.error("Unknown internal error in cfx.js:");
      console.exception(e);
    }
    system.exit(system.E_FORCE);
  }
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/exception.js
... ...
@@ -0,0 +1,23 @@
  1
+// Set of custom exception class for cfx.js
  2
+// Designed to be handled nicely from main module
  3
+
  4
+function CfxError(name) {
  5
+  this.name = name;
  6
+}
  7
+CfxError.prototype = new Error();
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Please use CfxError.prototype = Object.create(Error.prototype) as new Error is not quite right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/exception.js
... ...
@@ -0,0 +1,23 @@
  1
+// Set of custom exception class for cfx.js
  2
+// Designed to be handled nicely from main module
  3
+
  4
+function CfxError(name) {
5
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Honestly I don't see any value in making this prototype hierarchies. They just make whole thing look more complicated. This would have worked just fine:

function cfxError(message, name) {
  var error = Error(message);
  error.cfxError = true;
  error.name = name || 'CFXError'
  return error;
}

function InvalidArgument(message) {
  return cfxError(message, 'InvalidArgument')
}

function InternalCfxError(message) {
  return cfxError(message, 'InternalCfxError')
}
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Although in catch you would just did if (error.cfxError) { ...

Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

After reading code of other modules, I'm convinced that you don't even need this module. Just set error.cfxError = true on throw or define helper error functions in the appropriate modules.

ochameau Collaborator
ochameau added a note June 26, 2012

I'm expecting these exceptions to be used by all cfx modules, in order to classify kind of errors and print the most meaningfull error message we can. And especially distinguish user error from internal errors.
Otherwise, implementation wise, I've just followed this page: https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Error
I'm fine with dropping the prototype usage in favor of your code snippet, but I think it make sense to keep this module in order to reuse these normalized errors?

Irakli Gozalishvili Collaborator
Gozala added a note June 26, 2012

I think what I was trying to say is that this complicates things, as you like to say users will have to learn about these error types. If only desire is to handle such errors differently we could just set isInternal or alike property and avoid a lot of complexity. If you want to have custom error types it's still ok by me (just make sure to use Object.create(Error.prototype) instead), although I would prefer more simpler approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/preferences.js
((71 lines not shown))
  71
+  }
  72
+
  73
+  return DOMSerializer.serializeToString(root);
  74
+}
  75
+exports.generateOptionsXul = generateOptionsXul;
  76
+
  77
+function generatePrefsJS(options, jetpackId) {
  78
+  let pref_list = []
  79
+
  80
+  for (let key in options) {
  81
+    let pref = options[key];
  82
+    if ('value' in pref) {
  83
+      let value = pref.value;
  84
+
  85
+      // isFloat
  86
+      // TODO: Understand why python code ignore float and only floats?
9
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Please use function form of typeof as suggested by style guide.

Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Maybe @warner can tell us why python code was ignoring floats ?

Brian Warner Owner
warner added a note June 25, 2012

No idea. The code (python in python-lib/cuddlefish/options_defaults.py and JS in python-lib/cuddlefish/app-extension/bootstrap.js) was ignoring floats when I got there. Looks like Erik Vold added that in 299ec79. Can prefs be floats? The comments on that commit suggest they have to be integers.

ochameau Collaborator
ochameau added a note June 26, 2012

@erikvold would you be able to take a look at this and confirm that I implemented preferences correctly in JS?

ochameau Collaborator
ochameau added a note June 26, 2012

Please use function form of typeof as suggested by style guide.

https://github.com/mozilla/addon-sdk/wiki/Coding-style-guide
There isn't anything about typeof?

Irakli Gozalishvili Collaborator
Gozala added a note June 26, 2012

There isn't anything about typeof?

Inherited from general mozilla style guide :)
https://developer.mozilla.org/En/Developer_Guide/Coding_Style#Operators

Good point, I'll add that to our style guide as well!

Erik Vold Collaborator
erikvold added a note June 26, 2012
Can prefs be floats? 

no, unless a string type is used.

Erik Vold Collaborator
erikvold added a note June 26, 2012
@erikvold would you be able to take a look at this and confirm that I implemented preferences correctly in JS?

Sure I'll take a look tonight.

Erik Vold Collaborator
erikvold added a note June 26, 2012

the preferences.js file looks good to me @ochameau I'll have time to test it out tomorrow night.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/preferences.js
((72 lines not shown))
  72
+
  73
+  return DOMSerializer.serializeToString(root);
  74
+}
  75
+exports.generateOptionsXul = generateOptionsXul;
  76
+
  77
+function generatePrefsJS(options, jetpackId) {
  78
+  let pref_list = []
  79
+
  80
+  for (let key in options) {
  81
+    let pref = options[key];
  82
+    if ('value' in pref) {
  83
+      let value = pref.value;
  84
+
  85
+      // isFloat
  86
+      // TODO: Understand why python code ignore float and only floats?
  87
+      if (typeof value == "number" && value % 1 !== 0)
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

could you write helper function isFload and use it here if (isFloat(value)) .... would be much easier to understand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/preferences.js
((74 lines not shown))
  74
+}
  75
+exports.generateOptionsXul = generateOptionsXul;
  76
+
  77
+function generatePrefsJS(options, jetpackId) {
  78
+  let pref_list = []
  79
+
  80
+  for (let key in options) {
  81
+    let pref = options[key];
  82
+    if ('value' in pref) {
  83
+      let value = pref.value;
  84
+
  85
+      // isFloat
  86
+      // TODO: Understand why python code ignore float and only floats?
  87
+      if (typeof value == "number" && value % 1 !== 0)
  88
+        continue;
  89
+      else if (typeof value == "boolean")
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Please prefer === unless type conversion is desired (as suggested by style guide)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/preferences.js
((77 lines not shown))
  77
+function generatePrefsJS(options, jetpackId) {
  78
+  let pref_list = []
  79
+
  80
+  for (let key in options) {
  81
+    let pref = options[key];
  82
+    if ('value' in pref) {
  83
+      let value = pref.value;
  84
+
  85
+      // isFloat
  86
+      // TODO: Understand why python code ignore float and only floats?
  87
+      if (typeof value == "number" && value % 1 !== 0)
  88
+        continue;
  89
+      else if (typeof value == "boolean")
  90
+        value = pref.value ? "true" : "false";
  91
+      else if (typeof value == "string")
  92
+        value = "\"" + pref.value.replace(/"/g, "\\\"") + "\"";
2
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Please add comment explaining what this does and more importantly why. Example of pref => normalizedPref would also be helpful.

ochameau Collaborator
ochameau added a note June 26, 2012

I refactored this code. It was the pure equivalent of python code, but we can use JSON.stringify instead.
The goal here is to stringify the preference value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/preferences.js
... ...
@@ -0,0 +1,103 @@
  1
+const { Cc, Ci } = require("chrome");
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

license block + "use strict"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/exception.js
... ...
@@ -0,0 +1,23 @@
  1
+// Set of custom exception class for cfx.js
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Please "use strict"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/xpi.js
... ...
@@ -0,0 +1,271 @@
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

license block + "use strict"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/test/test-xpi.js
... ...
@@ -0,0 +1,177 @@
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

license block + "use strict"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/cfx.js
... ...
@@ -0,0 +1,62 @@
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

license block + "use strict"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/preferences.js
((81 lines not shown))
  81
+    let pref = options[key];
  82
+    if ('value' in pref) {
  83
+      let value = pref.value;
  84
+
  85
+      // isFloat
  86
+      // TODO: Understand why python code ignore float and only floats?
  87
+      if (typeof value == "number" && value % 1 !== 0)
  88
+        continue;
  89
+      else if (typeof value == "boolean")
  90
+        value = pref.value ? "true" : "false";
  91
+      else if (typeof value == "string")
  92
+        value = "\"" + pref.value.replace(/"/g, "\\\"") + "\"";
  93
+      else
  94
+        value = pref.value;
  95
+
  96
+      prefKey = "extensions." + jetpackId + "." + pref.name;
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

You forgot to define prefKey

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/preferences.js
((63 lines not shown))
  63
+                                        pref.name + "');");
  64
+      setting.appendChild(button);
  65
+    }
  66
+    else if (pref.type == "boolint") {
  67
+      setting.setAttribute("on", pref.on);
  68
+      setting.setAttribute("off", pref.off);
  69
+    }
  70
+    root.appendChild(setting);
  71
+  }
  72
+
  73
+  return DOMSerializer.serializeToString(root);
  74
+}
  75
+exports.generateOptionsXul = generateOptionsXul;
  76
+
  77
+function generatePrefsJS(options, jetpackId) {
  78
+  let pref_list = []
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

camelCase please.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((4 lines not shown))
  4
+const file = require("api-utils/file");
  5
+const preferences = require("./preferences");
  6
+const { InternalCfxError, InvalidArgument } = require("./exception");
  7
+
  8
+const FILE_SEPARATOR = require("api-utils/runtime").OS === "WINNT" ? "\\" : "/";
  9
+
  10
+// Utility function which allow to concatenate file path fragments
  11
+// This function accept n-th arguments. It basically join given string with
  12
+// the correct file separator depending on your OS
  13
+function removeEmpty(value) {
  14
+  return value !== ""
  15
+}
  16
+function joinPath() {
  17
+  let list = Array.slice(arguments);
  18
+  return list.filter(removeEmpty).join(FILE_SEPARATOR);
  19
+}
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I"m fine this, but I think we should probably import node's path module. I have being using it intensively for linker too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/preferences.js
((16 lines not shown))
  16
+      throw new InvalidArgument("The '" + pref.name + "' pref requires a 'title'");
  17
+
  18
+    // Make sure that the pref type is a valid inline pref type
  19
+    if (VALID_PREF_TYPES.indexOf(pref.type) === -1)
  20
+      throw new InvalidArgument(pref.type + " is not a valid inline pref type");
  21
+
  22
+    // Make sure the 'control' type has a 'label'
  23
+    if (pref.type == "control" && !("label" in pref))
  24
+      throw new InvalidArgument("The 'control' inline pref type requires a 'label'");
  25
+
  26
+    // TODO: Check that pref["type"] matches default value type
  27
+  }
  28
+}
  29
+exports.validate = validate;
  30
+
  31
+function createXULDocument(content) {
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I think nsIDOMParser is the right way to go about this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/package.json
... ...
@@ -0,0 +1,7 @@
  1
+{
  2
+  "id": "cfx",
  3
+  "main": "cfx.js",
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

please use: "main": "./cfx.js". Also could you please add name and description ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((11 lines not shown))
  11
+// This function accept n-th arguments. It basically join given string with
  12
+// the correct file separator depending on your OS
  13
+function removeEmpty(value) {
  14
+  return value !== ""
  15
+}
  16
+function joinPath() {
  17
+  let list = Array.slice(arguments);
  18
+  return list.filter(removeEmpty).join(FILE_SEPARATOR);
  19
+}
  20
+
  21
+// Utility function to ignore unwanted files in .xpi
  22
+const IGNORED_FILE_PREFIXES = ["."];
  23
+const IGNORED_FILE_SUFFIXES = ["~", ".swp"];
  24
+const IGNORED_DIRS = [".git", ".svn", ".hg"];
  25
+
  26
+function isAcceptableDirectory(name) {
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I would rename to isRecognizedDirectory or isSupportedDirectory. Acceptable feels to harsh to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
... ...
@@ -0,0 +1,271 @@
  1
+const { Cc, Ci } = require("chrome");
  2
+
  3
+const { ZipWriter } = require("./zip");
  4
+const file = require("api-utils/file");
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

It's little strange that you use both nsIFile and this module wrapping it together. I'd suggest to stick either one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
... ...
@@ -0,0 +1,271 @@
  1
+const { Cc, Ci } = require("chrome");
  2
+
  3
+const { ZipWriter } = require("./zip");
  4
+const file = require("api-utils/file");
  5
+const preferences = require("./preferences");
  6
+const { InternalCfxError, InvalidArgument } = require("./exception");
  7
+
  8
+const FILE_SEPARATOR = require("api-utils/runtime").OS === "WINNT" ? "\\" : "/";
  9
+
  10
+// Utility function which allow to concatenate file path fragments
  11
+// This function accept n-th arguments. It basically join given string with
  12
+// the correct file separator depending on your OS
  13
+function removeEmpty(value) {
2
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

It's little confusing to have comment of joinPath on top of this function. I would move it below this function and maybe add comment saying that it's internal utility used by joinPath.

Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Also name of this function is very confusing. It does not removes anything it just checks if value is empty. I'd suggest to rename it to isNotEmpty instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((27 lines not shown))
  27
+  return IGNORED_DIRS.indexOf(name) === -1;
  28
+}
  29
+
  30
+function isAcceptableFile(name, blackList) {
  31
+  for (let i = 0; i < IGNORED_FILE_PREFIXES.length; i++) {
  32
+    let prefix = IGNORED_FILE_PREFIXES[i];
  33
+    if (name.substr(0, prefix.length) == prefix)
  34
+      return false;
  35
+  }
  36
+  for (let i = 0; i < IGNORED_FILE_SUFFIXES.length; i++) {
  37
+    let suffix = IGNORED_FILE_SUFFIXES[i];
  38
+    if (name.substr(-1 * suffix.length) == suffix)
  39
+      return false;
  40
+  }
  41
+  return true;
  42
+}
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I think it would be way simpler if you would leveraged Array methods instead:

function isIgnoredFile(name) {
  return IGNORED_FILE_PREFIXES.some(function(prefix) {
    return name.indexOf(prefix) === 0
  }) || IGNORED_FILE_SUFFIXES.some(function(suffix) {
    return name.substr(-1 * suffix.length) === suffix
  })
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/xpi.js
((43 lines not shown))
  43
+
  44
+// Utility function in order to read a folder recursively
  45
+// Returns a list of all sub-directories. Each directory is represented with
  46
+// an object contains following attributes:
  47
+// - path: relative path the the directory (is an empty string for root folder)
  48
+// - dirnames: a list of all directory names existing in this directory
  49
+// - filenames: a list of all file names available in this directory
  50
+function walkDir(path) {
  51
+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
  52
+  try {
  53
+    file.initWithPath(path);
  54
+  } catch(e) {
  55
+    throw new Error("This directory path is not valid : " + path + "\n" + e);
  56
+  }
  57
+  let directories = [];
  58
+  function readDir(path, file) {
4
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I don't force to a change here, but I do think that leveraging array map / filter / reduce would made this a lot easier:

function isDirectory(entry) {
  return entry.isDirectory();
}

function isFile(entry) {
  return !isDirectory(entry)
}

function getLeafName(entry) {
  return entry.leafName
}

function readDir(path) {
  let file = MozFile(path)
  let entries = enumerator2array(file.directoryEntries)
  let dirs = entries.filter(isDirectory).map(getLeafName).filter(isAcceptableDirectory)
  let files = entries.filter(isFile).map(getLeafName).filter(isAcceptableFile)

  return dirs.reduce(function(result, name) {
    return result.concat(readDir(joinPath(path, name)))
  }, [{ path: path, dirnames: dirs, filenames: files }])
}
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

After reading it even further I think you could make just return array of dir paths and file paths without creating these objects:

function readDir(path) {
  let file = MozFile(path)
  let entries = enumerator2array(file.directoryEntries)
  let dirs = entries.filter(isDirectory).
    map(getLeafName).
    filter(isAcceptableDirectory).
    map(function() { return joinPath(path, name); });

  let files = entries.filter(isFile).
    map(getLeafName).
    filter(isAcceptableFile).
    map(function() { return joinPath(path, name); });

   let nested = dirs.map(readDir);

   return { dirs: dirs.concat(nested.dirs), files: files.concat(nested.files) }
}
ochameau Collaborator
ochameau added a note June 26, 2012

walkDir behaves like os.walk from python: http://www.saltycrane.com/blog/2007/03/python-oswalk-example/
The only difference is that path is a relative path from the original path given to walkDir. (Just because we don't have relpath function and I didn't wanted to implement it)

Here it looks like you return all directories and all files without being able to figure out from which folder each of these are.
The first version seems to implement the same behavior than os.walk.

I'm not sure it is easier to follow especially in lines around reduce usage. Whereas the original code can surely be read by any guy used to read js xpcom code. I'd really like that jetpack code can be easily read/patched/maintained by any mozilla-central devs and that we end up using similar pratices to existing mozilla-central code:
http://mxr.mozilla.org/mozilla-central/search?string=.directoryEntries&find=\.js&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central
Not necessary all SDK code, but especially the low level one, which interact with xpcom and can be broken by new mozilla-central revision. If we would like to be integrated with other mozilla teams, we will need to unhide our unit tests on tinderbox and allow them to understand what they broke and why. So that they can land fixes in addon-sdk repo while landing a modification in mozilla-central. I'm considering such abstraction on top of xpcom interfaces as being negative about this, even if it can be usefull for new addon developers.

Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

Anyway it was just a suggestion that IMO would have improved maintainability, but I'm ok leaving as is. On your argument regarding moz-central, I'm not fully convinced, we do try to innovate which is very hard without doing things old way. Not very important here anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((81 lines not shown))
  81
+// Utility function for zip library usage. zip library only accept `/` as file
  82
+// separator for relative in-zip paths.
  83
+function makeZipPath(path) {
  84
+  return path.split(FILE_SEPARATOR).join("/");
  85
+}
  86
+
  87
+// Return a pretty printed JSON string with 2 spaces indentation
  88
+function prettyPrintJSON(json) {
  89
+  return JSON.stringify(json, null, 2);
  90
+}
  91
+
  92
+// Write icon files
  93
+function writeIcons(zip, harnessOptions) {
  94
+  if ('icon' in harnessOptions) {
  95
+    zip.addFile("icon.png", harnessOptions.icon);
  96
+    delete harnessOptions.icon;
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Why delete ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((95 lines not shown))
  95
+    zip.addFile("icon.png", harnessOptions.icon);
  96
+    delete harnessOptions.icon;
  97
+  }
  98
+  if ('icon64' in harnessOptions) {
  99
+    zip.addFile("icon64.png", harnessOptions.icon64);
  100
+    delete harnessOptions.icon64;
  101
+  }
  102
+}
  103
+
  104
+// Write default preferences and xul options document (opened from about:addons)
  105
+function writePreferences(zip, jetpackId, preferences) {
  106
+  // Handle `preferences` from package.json
  107
+  if (preferences) {
  108
+    preferences.validate(preferences);
  109
+
  110
+    opts_xul = preferences.generateOptionsXul(preferences,
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

you introduce global here and please use camelCase. No need to follow python that closely :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((99 lines not shown))
  99
+    zip.addFile("icon64.png", harnessOptions.icon64);
  100
+    delete harnessOptions.icon64;
  101
+  }
  102
+}
  103
+
  104
+// Write default preferences and xul options document (opened from about:addons)
  105
+function writePreferences(zip, jetpackId, preferences) {
  106
+  // Handle `preferences` from package.json
  107
+  if (preferences) {
  108
+    preferences.validate(preferences);
  109
+
  110
+    opts_xul = preferences.generateOptionsXul(preferences,
  111
+                                              jetpackID);
  112
+    zip.addData("options.xul", opts_xul);
  113
+
  114
+    prefs_js = preferences.generatePrefsJS(preferences,
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

global!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((147 lines not shown))
  147
+  // another dictionnary whose keys are section name (lib or test) and final
  148
+  // value is absolute path to this package's section
  149
+  zip.mkdir("resources");
  150
+  for (let packageName in packages) {
  151
+    // Always write the top directory, even if it contains no files, since
  152
+    // the harness will try to access it.
  153
+    zip.mkdir('resources/' + packageName);
  154
+    for (let sectionName in packages[packageName]) {
  155
+      let abs_dirname = packages[packageName][sectionName]
  156
+      let base_arcpath = ["resources", packageName, sectionName].join("/")
  157
+      // Always write the top directory, even if it contains no files, since
  158
+      // the harness will try to access it.
  159
+      zip.mkdir(base_arcpath);
  160
+      // cp -r stuff from abs_dirname/ into ZIP/resources/RESOURCEBASE/
  161
+      for each (let directory in walkDir(abs_dirname)) {
  162
+        let goodfiles = directory.filenames;
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

This name is confusing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((130 lines not shown))
  130
+    for each(let name in directory.dirnames) {
  131
+      let relpath = joinPath(directory.path, name);
  132
+      zip.mkdir(makeZipPath(relpath));
  133
+    }
  134
+    for each(let name in directory.filenames) {
  135
+      if (["install.rdf", "application.ini"].indexOf(name) !== -1)
  136
+        continue;
  137
+      let relpath = joinPath(directory.path, name);
  138
+      let abspath = joinPath(templatePath, relpath);
  139
+      zip.addFile(makeZipPath(relpath), abspath);
  140
+    }
  141
+  }
  142
+}
  143
+
  144
+// Write all packages to resources/ folder
  145
+function writePackages(zip, packages, limitTo, xpiPath) {
2
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Please add comments describing arguments, it's far from obvious in this case. Also If I understand limitTo correctly it should probably be named relativeTo instead.

ochameau Collaborator
ochameau added a note June 26, 2012

limitTo is the white-list of file to accept in xpi file, used to implement strip-xpi feature.
I've added comment on top of all write* functions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((186 lines not shown))
  186
+        }
  187
+      }
  188
+    }
  189
+  }
  190
+}
  191
+
  192
+// Write locale manifest and locale files to the xpi
  193
+function writeLocales(zip, locales) {
  194
+  // We store a sorted list of locales
  195
+  let languages = Object.keys(locales).sort();
  196
+  let localesManifest = {
  197
+    locales: languages
  198
+  };
  199
+  zip.addData("locales.json", prettyPrintJSON(localesManifest) + "\n");
  200
+  zip.mkdir("locale/");
  201
+  for each(let language in languages) {
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Please prefer standard array.forEach over non-standard for each

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((198 lines not shown))
  198
+  };
  199
+  zip.addData("locales.json", prettyPrintJSON(localesManifest) + "\n");
  200
+  zip.mkdir("locale/");
  201
+  for each(let language in languages) {
  202
+    let locale = locales[language];
  203
+    zip.addData("locale/" + language + ".json", prettyPrintJSON(locale));
  204
+  }
  205
+}
  206
+
  207
+// Write `harness-options.json` manifest file at xpi root
  208
+function writeManifest(zip, harnessOptions, extraHarnessOptions) {
  209
+  // TODO: print better error message on harness-options loading error
  210
+  //       "Error: Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsIChannel.open]"
  211
+
  212
+  // Include extra manifest options given manually to cfx
  213
+  for (let key in extraHarnessOptions) {
2
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Would not it make more sense to combine extraHarnessOptions with harnessOptions itself ? What's the point of accepting two different options that get combined later anyway ?

ochameau Collaborator
ochameau added a note June 26, 2012

extraHarnessOptions is given by the user while harnessOptions is fully computed by python code.
I'd prefer keeping it as-is in order to avoid combining them in python side.
It is something we will easily cleanup when we will build a cleaner manifest from js side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((230 lines not shown))
  230
+    throw new InternalCfxError(
  231
+      "Missing `templatePath` in cfxjs options");
  232
+  if (!("locale" in options.harnessOptions))
  233
+    throw new InternalCfxError(
  234
+      "Missing `locale` in cfxjs options harnessOptions attribute");
  235
+  let harnessOptions = options.harnessOptions;
  236
+
  237
+  let zip = new ZipWriter(options.xpiPath);
  238
+
  239
+  try {
  240
+    zip.addData("install.rdf", options.installRdf);
  241
+
  242
+    writeIcons(zip, harnessOptions);
  243
+
  244
+    writePreferences(zip, harnessOptions.jetpackID,
  245
+                     harnessOptions.preferences ? harnessOptions.preferences
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Why not harnessOptions.preferences || null ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((232 lines not shown))
  232
+  if (!("locale" in options.harnessOptions))
  233
+    throw new InternalCfxError(
  234
+      "Missing `locale` in cfxjs options harnessOptions attribute");
  235
+  let harnessOptions = options.harnessOptions;
  236
+
  237
+  let zip = new ZipWriter(options.xpiPath);
  238
+
  239
+  try {
  240
+    zip.addData("install.rdf", options.installRdf);
  241
+
  242
+    writeIcons(zip, harnessOptions);
  243
+
  244
+    writePreferences(zip, harnessOptions.jetpackID,
  245
+                     harnessOptions.preferences ? harnessOptions.preferences
  246
+                                                : null);
  247
+    delete harnessOptions.preferences;
2
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Why delete ? Please at least document! Some for all the following deletes.

ochameau Collaborator
ochameau added a note June 26, 2012

There is some delete in order to avoid writing these attribute into harness-options.json file.
I've added some comment on top of this one, I hope it is clear for following ones?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((252 lines not shown))
  252
+                  options.xpiPath);
  253
+    delete harnessOptions.packages;
  254
+
  255
+    writeLocales(zip, harnessOptions.locale);
  256
+    delete harnessOptions.locale;
  257
+
  258
+    writeManifest(zip, harnessOptions, options.extraHarnessOptions);
  259
+  }
  260
+  catch(e) {
  261
+    // In case or any error, we delete the eventually created xpi file
  262
+    // in order to avoid processing it in futher steps in python code
  263
+    zip.close();
  264
+    try {
  265
+      file.remove(options.xpiPath);
  266
+    }
  267
+    catch(e) {};
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Capturing error and doing nothing with it is no good, either don't catch or do something with it. If this error is really useless then use finally { throw e } instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/xulrunner-app/application.ini
... ...
@@ -0,0 +1,11 @@
  1
+[App]
  2
+Vendor=Mozilla
  3
+Name=Cuddlefish in Javascript
  4
+Version=1.0
  5
+BuildID=20120101
  6
+Copyright=
  7
+ID=cfx-js@mozilla.org
1
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

Can we use cfx-js in all other places instead of cfx.js then ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/xulrunner-app/command-line-handler.js
((44 lines not shown))
  44
+    Services.scriptloader.loadSubScript(
  45
+      xpiJarURI.spec + "bootstrap.js",
  46
+      bootstrap);
  47
+
  48
+    // data object sent to bootstrap's startup method:
  49
+    // An object crafted by AddonManager code, but jetpack only uses
  50
+    // resourceURI attribute
  51
+    let data = {
  52
+      resourceURI: xpiJarURI
  53
+    };
  54
+
  55
+    // load reason code, always use 'enable'
  56
+    let reasonCode = 3;
  57
+
  58
+    // Fake AddonManager behavior by calling startup method
  59
+    bootstrap.startup(data, reasonCode);
5
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I'm not convinced that pretending that it's enable is a good idea, I think we should use our own reasonCode to make it possible to handle this case differently and to avoid confusing other parts of the platform.

ochameau Collaborator
ochameau added a note June 26, 2012

Any new custom reason code will have to be handled differently in bootstrap.js and/or addon/runner, I'd like to avoid that.
I don't see why we would have different startup code between running as xulrunner app, firefox addon, thunderbird addon, ...
But I can easily agree that using enable is a bit hacky as it is enabled during startup. So ideally we would just use startup reason. But in case of startup, addon/runner is waiting for some event to happen before running main module.
In previous patch version, I used startup and dispatched the expected event.
What do you think about modifying addon/runner in order to use startup but avoid waiting for any event?
I'm wondering if it make sense to listen to any event in * case:
https://github.com/mozilla/addon-sdk/blob/master/packages/api-utils/lib/addon/runner.js#L15

Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

We can talk about changing how / when do we bootstrap add-ons, but I'd rather have that discussion under separate thread and definitely as separate step. I don't like enable or startup as both may be fired by an actual platform causing strange behaviors. In general pretending that we do something that we don't asks for problems.

I think change to handle diff stratup reason can be very minimal. What is actually a reason you'd like to avoid that ?

ochameau Collaborator
ochameau added a note June 28, 2012

For the same reason that forces us to have a buildstep. I thought that it was important to build and use cfx-js as a 100% regular SDK addon. This command-line-handler.js component just fakes AddonManager action. It is like having cfx-js.xpi installed in this xulrunner app. Note that no other component other have access to addon's bootstrap.js. So no other startup event can be fired to it.
I could have just use AddonManager API in order to install/run the addon. Then I'd not have been able to change the reason code.

In term of dogfooding, it is quite important to avoid using any different path from regular addons. In this particular case, I don't see why running an addon from xulrunner (instead of firefox) should change the startup reason code ? What would be this startup code ? command-line-startup ?

I may see your point. You want to do different actions during bootstrap based on the platform, then I'm not sure reason code should be used for that. We may prefer application id or something else.

Irakli Gozalishvili Collaborator
Gozala added a note July 05, 2012

I think we can start with this and then change if turns out to be problematic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xulrunner-app/command-line-handler.js
((55 lines not shown))
  55
+    // load reason code, always use 'enable'
  56
+    let reasonCode = 3;
  57
+
  58
+    // Fake AddonManager behavior by calling startup method
  59
+    bootstrap.startup(data, reasonCode);
  60
+  }
  61
+  catch(e) {
  62
+    let msg = "Exception while running boostrap.js:\n" + e + "\n" + e.stack;
  63
+    dump(msg);
  64
+    Cu.reportError(msg);
  65
+  }
  66
+}
  67
+
  68
+// Register a nsICommandLineHandler xpcom object in order to call
  69
+// runApplication method
  70
+function CommandLineHandler() {}
3
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I still don't understand why we register nsICommandLineHandler if we don't handle command line arguments.

ochameau Collaborator
ochameau added a note June 26, 2012

It would allow us to handle them in a next step.
One alternative would be to use another kind of xpcom that will register itself in a category to instanciate it at app startup.
But as soon as we will have to handle command line arguments, we will have to use this form back.

Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

I don't really object to using nsICommandLineHandler it was just unclear why, maybe you could put your comment above as a comment somewhere so it's clear for people wondering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/zip.js
((13 lines not shown))
  13
+const { COMPRESSION_DEFAULT } = Ci.nsIZipWriter;
  14
+
  15
+function createNsFile(path) {
  16
+  let file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
  17
+  try {
  18
+    file.initWithPath(path);
  19
+  } catch(e) {
  20
+    throw new Error("This zip file path is not valid : " + path + "\n" + e);
  21
+  }
  22
+  return file;
  23
+}
  24
+
  25
+const converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
  26
+                  createInstance(Ci.nsIScriptableUnicodeConverter);
  27
+converter.charset = "UTF-8";
  28
+function streamForData(data) {
3
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I think nsIStringInputStream is more efficient and more appropriate alternative. For examples see:
https://developer.mozilla.org/en/Creating_Sandboxed_HTTP_Connections#Creating_HTTP_POSTs

ochameau Collaborator
ochameau added a note June 26, 2012

It doesn't seem to accept exotic UTF8 characters.
@Mossop, any advice about utf8 string and xpcoms? :)

  let inputStream = Cc["@mozilla.org/io/string-input-stream;1"]  
                  .createInstance(Ci.nsIStringInputStream);
  inputStream.setData(data, data.length);
  return inputStream;
  const converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
                  createInstance(Ci.nsIScriptableUnicodeConverter);
  converter.charset = "UTF-8";
  return converter.convertToInputStream(data);

I'm currently using second form in order to build an inputstream for nsIZipWriter:
zw.addEntryStream(pathInZip, 0, COMPRESSION_DEFAULT, stream, false);
data is a string containing wild UTF8 characters.
While second form works, the first doesn't throw any exception, but the file isn't written in zip file.

Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

That is strange, I would have expected nsIStringInputStream to inherit whatever encoding string was using, but yeah I never understood when to use which stream. If that does not works lets leave as is then. Although if we could get someone to explain which stream should be used when that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 25, 2012
cfx/zip.js
((17 lines not shown))
  17
+  try {
  18
+    file.initWithPath(path);
  19
+  } catch(e) {
  20
+    throw new Error("This zip file path is not valid : " + path + "\n" + e);
  21
+  }
  22
+  return file;
  23
+}
  24
+
  25
+const converter = Cc["@mozilla.org/intl/scriptableunicodeconverter"].
  26
+                  createInstance(Ci.nsIScriptableUnicodeConverter);
  27
+converter.charset = "UTF-8";
  28
+function streamForData(data) {
  29
+  return converter.convertToInputStream(data);
  30
+}
  31
+
  32
+exports.ZipWriter = function (zipPath, mode) {
4
Irakli Gozalishvili Collaborator
Gozala added a note June 25, 2012

I personally don't see much value in building such class like a construct and would prefer more node fs-ish API:

exports.open = function(path, mode) {}
exports.mkdir = function(zd, path) {}
exports.addFile = function(zd, to, from) {}
exports.write = function(zd, path, content) {}
exports.close = function(zd) {}

That being said if we do it, it definitely would make more sense as an incremental step after.

ochameau Collaborator
ochameau added a note June 26, 2012

Sure, I can change this. What should openreturn?
Some empty object that will be used to fetch the nsIZipWriter instance via namespace, or the nsIZipWriter instance itself directly, or some other object?

Irakli Gozalishvili Collaborator
Gozala added a note June 26, 2012

I'm fine with either of these options. I'd probably suggest something like:

// Maybe just define some private name in module
var name = Math.random().toString(32).substr(2)

// And use that name to define internal properties
return Object.defineProperty({}, name, {
  value: nsIZipWriter
})
Irakli Gozalishvili Collaborator
Gozala added a note June 26, 2012

It also maybe worth adapting ES.next private names shim I wrote until we get an actual one
once bug 645416 is fixed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili
Collaborator
Gozala commented June 25, 2012

I have pointed out various stuff in around, some of which are just a remarks. You should definitely make sure to add "use strict" and license blocks to all JS files, that would have could all of the undeclared variables that I've pointed out. I took at the python code as well, but it would be great if someone with more expertise could take a look at it as well, maybe @warner or @mhammond could find some time for that.

I'm not quite comfortable with a build step that users have to perform and wonder If there is something we could do about it to avoid that.

Feel free to request a meeting if you want to go through thee together faster ;)

Irakli Gozalishvili
Collaborator
Gozala commented June 25, 2012

I have pointed out various stuff in around, some of which are just a remarks. You should definitely make sure to add "use strict" and license blocks to all JS files, that would have could all of the undeclared variables that I've pointed out. I took at the python code as well, but it would be great if someone with more expertise could take a look at it as well, maybe @warner or @mhammond could find some time for that.

I'm not quite comfortable with a build step that users have to perform and wonder If there is something we could do about it to avoid that.

Feel free to request a meeting if you want to go through thee together faster ;)

ochameau
Collaborator

I addressed all comments except ones where I've added comments + the one about nsIDOMParser (haven't had time to looked at it - #410 (comment))

cfx/preferences.js
((83 lines not shown))
  83
+  return typeof(value) == "number" && value % 1 !== 0;
  84
+}
  85
+
  86
+/**
  87
+ * Based on preferences manifest written in package.json file of an addon,
  88
+ * returns the necessary prefs.js file content. This file is going to set
  89
+ * default preference values when the addon will be installed.
  90
+ */
  91
+function generatePrefsJS(options, jetpackId) {
  92
+  let prefList = []
  93
+
  94
+  for (let key in options) {
  95
+    let pref = options[key];
  96
+    if (!('value' in pref))
  97
+      continue;
  98
+    // TODO: Understand why python code ignore float and only floats?
2
Erik Vold Collaborator
erikvold added a note June 26, 2012

the only acceptable types are bool, int, and string. So that is why floats were ignored. It might be better to throw a warning, or error here tho, and the other acceptable JSON types (ie: object, array, and null) should be treated the same as a float.

ochameau Collaborator
ochameau added a note June 27, 2012

I've updated this patch again in order to accept only bool, int or string and throw an error otherwise.
Feel free to tell me if there is still something wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
ochameau
Collaborator

About build step. I agree it is kind of painfull to have such build step. The benefit is that cfx.js is just a regular SDK "addon". And if we want to keep it as being a regular SDK addon, we have to face the mandatory build step.
We can make it easier with some build-update script. Otherwise we can do some hacks like this:
ochameau@ochameau:cfx.js...ochameau:cfx.js-no-build
Where we instanciate a custom loader to make it work dynamically. But I'd prefer to avoid such thing. I don't think it is a good idea to maintain such code in cfx itself. We should either integrate such behavior into the SDK itself or at least modify bootstrap/cuddlefish/loader in order to make it super easy. (In this experiement cfx.js works, but I'm lacking of addon-kit, self support, locales, ...)
Would it be something we can later iterate on?

Irakli Gozalishvili
Collaborator
Gozala commented June 27, 2012

Would it be something we can later iterate on?

This is huge change for a users, I believe decisions on weather we should or not do such changes is up to project manager @dcm-moz / @canuckistani I personally would rather see custom cfx-js specefic loader instance than a build step. Alternatively we could do bundle build add-on or do it transparently from users.

We should either integrate such behavior into the SDK itself or at least modify bootstrap/cuddlefish/loader in order to
make it super easy

Do you have something specific in mind ?

cfx/preferences.js
((83 lines not shown))
  83
+
  84
+/**
  85
+ * Based on preferences manifest written in package.json file of an addon,
  86
+ * returns the necessary prefs.js file content. This file is going to set
  87
+ * default preference values when the addon will be installed.
  88
+ */
  89
+function generatePrefsJS(options, jetpackId) {
  90
+  let prefList = []
  91
+
  92
+  for (let key in options) {
  93
+    let pref = options[key];
  94
+    if (!('value' in pref))
  95
+      continue;
  96
+
  97
+    if (["boolean", "number", "string"].indexOf(typeof(pref.value)) === -1
  98
+        || isFloat(pref.value))
1
Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

I think this would have being little easier:

if (!isPrimitive(pref.value) || isFloat(pref.value)) { ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((187 lines not shown))
  187
+ *     List of all white-listed files. If given, only file whose absolute path
  188
+ *     is in this list are copied into the xpi.
  189
+ * @param {String} xpiPath
  190
+ *     Absolute path to the xpi file. Used to avoid copying the xpi in itself!
  191
+ */
  192
+function writePackages(zip, packages, limitTo, xpiPath) {
  193
+  // `packages` is a dictionnary whose keys are package name and values are
  194
+  // another dictionnary whose keys are section name (lib or test) and final
  195
+  // value is absolute path to this package's section
  196
+  zip.mkdir("resources");
  197
+  for (let packageName in packages) {
  198
+    // Always write the top directory, even if it contains no files, since
  199
+    // the harness will try to access it.
  200
+    zip.mkdir('resources/' + packageName);
  201
+    for (let sectionName in packages[packageName]) {
  202
+      let abs_dirname = packages[packageName][sectionName]
1
Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

camelCase please, here and below. Also little more descriptive names would help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
cfx/xpi.js
((314 lines not shown))
  314
+  if (!("limit-to" in options))
  315
+    throw new InternalCfxError(
  316
+      "Missing `limit-to` in cfxjs options");
  317
+  let limitTo = options["limit-to"];
  318
+
  319
+  let zip = new ZipWriter(xpiPath);
  320
+
  321
+  try {
  322
+    zip.addData("install.rdf", installRdf);
  323
+
  324
+    writeIcons(zip, harnessOptions);
  325
+
  326
+    writePreferences(zip, harnessOptions.jetpackID,
  327
+                     harnessOptions.preferences || null);
  328
+    // We delete harnessOptions attributes in order to avoid writing them
  329
+    // into xpi's manifest file.
2
Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

I would honestly prefer something like serialize(harnessOptions) that would omit keys that should not be written (or maybe in writeManifest itself can do it). That way cherry-picking of properties is encapsulated in one location that is easy to find / maintain in contrast to deletes all over the place.

ochameau Collaborator
ochameau added a note July 04, 2012

I pushed new revision with some modification on python side in order to avoid adding such attribute in first place.
So that harnessOptions will only contain attribute that have to end up in harness-options.json file.
All others (locale, packages, icon, icon64) are now in options object.
This changed allowed me to prepare the next step. So now the JS side receive SDK and Addon absolute path,
so that you can start building manifest in JS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili Gozala commented on the diff June 27, 2012
cfx/xpi.js
((334 lines not shown))
  334
+    writePackages(zip, harnessOptions.packages, limitTo, xpiPath);
  335
+    delete harnessOptions.packages;
  336
+
  337
+    writeLocales(zip, harnessOptions.locale);
  338
+    delete harnessOptions.locale;
  339
+
  340
+    writeManifest(zip, harnessOptions, extraHarnessOptions);
  341
+  }
  342
+  catch(e) {
  343
+    // In case or any error, we delete the eventually created xpi file
  344
+    // in order to avoid processing it in futher steps in python code
  345
+    zip.close();
  346
+    try {
  347
+      // We do not care about any error while removing the xpi file,
  348
+      // we try to ensure that we do not leave any temporary file around.
  349
+      // It may not even be created.
2
Irakli Gozalishvili Collaborator
Gozala added a note June 27, 2012

What's a point of catching and re-throwing more detailed errors from rm if they are ignored anyway ?

ochameau Collaborator
ochameau added a note July 03, 2012

Here I ignore errors from rm but not errors from the upper try/catch statement. i.e. errors happening in one of these writeSomething functions. I'm using finally as suggested in previous comment #410 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Irakli Gozalishvili
Collaborator
Gozala commented June 27, 2012

I made few minor points and I think only few things need to be sorted out before we can land:

  • Need to make sure that build step is fine by project managers and there is no better way to go about this.
  • Document why nsICommandLineHandler is used (comments).

In addition you might want to:

  • Get someone to do a python review, I looked at it and code makes sense to me, but I don't have enough expertise to tell if you do something in a way that considered to be a bad practice or if it will work for all python versions etc..
  • I don't know if you still plan to do this https://github.com/mozilla/addon-sdk/pull/410/files#r1047385 or if you were planning to do it in the separate step.

Happy to see this coming along nicely!

ochameau
Collaborator

I'm not sure it is clear about the build step. You won't have to build cfx.js if you want to use the SDK, but only if you want to patch cfx javascript code!

Irakli Gozalishvili
Collaborator
Gozala commented June 28, 2012

I'm not sure it is clear about the build step. You won't have to build cfx.js if you want to use the SDK, but only if you want > to patch cfx javascript code!

Ok then I clearly don't understand how this works :( Could we maybe talk on vidyo tomorrow morning I think little go through would be very helpful.