Developing add-ons for the new extension API should be simple. For that we need a load extension button and a package extension button from a local directory in the add-on page. Preferably hidden behind some checkbox... We also have to figure out when to enable this, since this is running unsigned extensions essentially.

Does this look like the right approach Dave? I suspect there are some cases I'm missing, but I want to make sure I'm not totally off base. Also, I'm not really sure how we'll address the signing issue. It would be pretty easy to disable signing along this path, but I don't know if it's a good idea :-). I've been testing it by toggling the pref.

Comment on attachment 8646681 [details] [diff] [review] patch Review of attachment 8646681 [details] [diff] [review]: ----------------------------------------------------------------- This is looking good ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm @@ +2951,5 @@ > } > } > > try { > + // FIXME: Do I need to handle the case where this is a dir add-on? Yeah you probably do @@ -5571,5 @@ > * XPI is incorrectly signed > */ > loadManifest: Task.async(function* AI_loadManifest(file) { > - let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"]. > - createInstance(Ci.nsIZipReader); This change will break things when we try to use the zipreader later

(In reply to Dave Townsend [:mossop] from comment #2) > ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm > @@ +2951,5 @@ > > } > > } > > > > try { > > + // FIXME: Do I need to handle the case where this is a dir add-on? > > Yeah you probably do Actually, I looked over this code more carefully and I don't think we need to do anything. I think this code will only be invoked for restartless add-ons if we crash during installation. If that happens, I think we'll end up here: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2825 That's maybe not the best outcome. However, I believe that we automatically skip unsigned restartless add-ons in the staging directory anyway: http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2883 Given that people are very unlikely to be installing signed add-ons via this method, it seems reasonable to me not to change the current behavior. After all, what are the chances that we crash during installation? > @@ -5571,5 @@ > > * XPI is incorrectly signed > > */ > > loadManifest: Task.async(function* AI_loadManifest(file) { > > - let zipreader = Cc["@mozilla.org/libjar/zip-reader;1"]. > > - createInstance(Ci.nsIZipReader); > > This change will break things when we try to use the zipreader later Maybe I missed something, but I tried to remove all uses of zipreader below.

(In reply to Bill McCloskey (:billm) from comment #3) > Actually, I looked over this code more carefully and I don't think we need > to do anything. Oh, I forgot that people could install non-restartless add-ons this way :-(.

Bill, Do you mind if I carry over your patch? I would like to see this feature in Firefox for Devtools needs. (We are trying to support new development workflow involving an addon to easily reload devtools codebase) I imagine we mostly miss some test for this? Also, I was wondering if it make sense to introduce the UI (also or instead) in about:debugging, which is more developers oriented than about:addons.

(In reply to Alexandre Poirot [:ochameau] from comment #5) > Also, I was wondering if it make sense to introduce the UI > (also or instead) in about:debugging, > which is more developers oriented than about:addons. I did not no about about:debug. Is this used by many add-on developers?

I would suggest "also" instead of "instead", if only for Chrome-parity… ;)

I was hoping to get to this in the next couple days. I'll assign it over to you if I haven't made any progress by the end of next week.

rhelmer has the backend pieces for this almost done in bug 1209341, we probably only need UI hookups in bug 1209344 so I'm not sure if this bug is useful anymore.

about:debugging just appeared during FF44 cycle. It only targets Workers and Addons so far, but it should over time allow you to debug more things and especially contexts running on remote devices like apps and tabs on Fennec/FirefoxOS. We haven't been communicating about this as it is brand new and certainly needs some polish. If the backend is already ready, I can certainly add the UI in about:debugging, as I already now this codebase. (In reply to Bill McCloskey (:billm) from comment #8) > I was hoping to get to this in the next couple days. I'll assign it over to > you if I haven't made any progress by the end of next week. Ok. I won't have much time until Monday, so it sounds like a good plan!

Hmm, I didn't realize bug 1209341 will support unpacked add-ons. I feel like there's value in being able to load unpacked add-ons in a non-temporary way if you disable the signing pref, especially for non-restartless add-ons. But maybe it's not a high priority.

Let's try to move the "install from file" UI discussion to bug 1209344. bill, can you close that bug or rename it to something more specific you have in mind?

OK, I re-titled the bug.

I would actually like to see that happen as well. The usecase is quite simple: * you may break firefox with your addon and have to restart it often * you may want to see how your addon behaves during firefox startup Mossop, First, are you ok with that? Robert, You are not already working on that, right? If yes, how would we implement this? Codewise: - should we make it so that existing installTemporaryAddon function install the addon permanently if the signature pref is off? - or, should we come up with another function? UI: - should the existing "load temporary addon" be permanent if signature is off? - should we display another action in about:debugging, if signature is off to load permanently?

(In reply to Alexandre Poirot [:ochameau] from comment #14) > I would actually like to see that happen as well. > The usecase is quite simple: > * you may break firefox with your addon and have to restart it often > * you may want to see how your addon behaves during firefox startup > > Mossop, First, are you ok with that? Yes I'm fine with this. > If yes, how would we implement this? > > Codewise: > - should we make it so that existing installTemporaryAddon function install > the addon permanently if the signature pref is off? > - or, should we come up with another function? I think we should use a different function and here's why. I think the temporary add-ons are something we can make special in ways that I wouldn't want to support for non-temporary add-ons. Things like watching the filesystem and reloading the add-on if something changes. So I'd rather keep them separated and make it possible to still use that even if you're also able to install unsigned add-ons.

Here is a tentative patch to do that. It hacks the existing about:debugging feature to install addons permanently. More work is needed in about:debugging to switch between temporary/permanent. Note that working in chrome is simplier, local addons are installed permanently. So I would like to prevent have to dance between temporary and permanent and keep firefox as simple.

Just looping in security to check this is ok.

Checking in with Kev too on this. BillM noticing this is assigned to you, make more sense to move to ochameau?

Dave, I would like to focus on the AddonManager part of this bug. May be I should open a specific bug? Here, I don't modify the behavior of about:addons nor about:debugging, I would like to keep that for another patch. Otherwise, what do you think about handling permanently sideloaded addons via a specific InstallLocation, like what we do for temporary install?

Comment on attachment 8797157 [details] Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently. I'm going to let Rob handle your questions here, he owns the add-ons manager now.

Comment on attachment 8797157 [details] Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently. https://reviewboard.mozilla.org/r/82770/#review81438 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4121 (Diff revision 1) > > addon = XPIDatabase.addAddonMetadata(addon, file.persistentDescriptor); > > XPIStates.addAddon(addon); > XPIDatabase.saveChanges(); > + //XPIStates.save(); Debugging? ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4136 (Diff revision 1) > AddonManagerPrivate.callAddonListeners("onInstalled", addon.wrapper); > > return addon.wrapper; > }), > > +/* This function is commented out, was this an earlier approach to the above replacement of `installTemporaryAddon` (which now calls `installAddonFromSources`?)

Comment on attachment 8797157 [details] Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently. https://reviewboard.mozilla.org/r/82770/#review81448 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8846 (Diff revision 1) > } > > +const SourceInstallLocation = { > + locked: false, > + name: KEY_APP_SOURCE, > + scope: AddonManager.SCOPE_TEMPORARY, Hm `SCOPE_TEMPORARY` doesn't seem accurate anymore - not sure if any of the existing ones match exactly what you want to do, `SCOPE_USER` is probably closest.

(In reply to Alexandre Poirot [:ochameau] from comment #23) > Dave, I would like to focus on the AddonManager part of this bug. May be I > should open a specific bug? > Here, I don't modify the behavior of about:addons nor about:debugging, I > would like to keep that for another patch. I agree, UI should be handled in a separate patch. Temporary add-on loading is only supported via about:debugging and not about:addons, I'm not sure about where this one should go - this might be appropriate for about:addons gear->"Install Add-on From Folder..." or so to go with the existing "From File". I would be OK with it just being in about:debugging if that makes sense from the devtools POV though. > Otherwise, what do you think about handling permanently sideloaded addons > via a specific InstallLocation, like what we do for temporary install? I see why we need a new API to trigger this since AddonManager supports loading extensions from a directory but only if they are present at startup (AFAICT), but I am not sure why a new InstallLocation is needed - wouldn't MutableDirectoryInstallLocation work? It makes sense to have TemporaryInstallLocation mostly because there is a lot of behavior we don't wish to implement, like supporting updates. Is this the case too for "from source dir" add-ons, or do you want to support features like updates? Overall I am fine with this approach, just would like to understand better why this should implement a new InstallLocation.

Just pushed a new patch addressing what you highlighted. I forgot that I kept work-in-progress things in the patch... I introduce a new dedicated scope. I understand about the new InstallLocation. TBH, the AddonManager code is quite scary, I didn't knew exactly how to proceed and was very happy once I got a working patch! I'm happy to go with a simplier alternative. Wouldn't MutableDirectoryInstallLocation end up deleting the source folder on uninstall? http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8387 Also, I know the addon manager does scan the addon folders for modtime or checksum. Do you know if that kicks in with the new InstallLocation? What about the MutableDirectoryInstallLocation? Regarding the signature, this features forces you to toggle the xpinstall.signatures.required pref to false. The new install location doesn't try to bypass any signature check.

Comment on attachment 8797157 [details] Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently. https://reviewboard.mozilla.org/r/82770/#review81540 ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4041 (Diff revision 2) > + * > * @return a Promise that resolves to an Addon object on success, or rejects > * if the add-on is not a valid restartless add-on or if the > - * same ID is already temporarily installed > + * same ID is already installed. > */ > - installTemporaryAddon: Task.async(function*(aFile) { > + installAddonFromSources: Task.async(function*(aFile, aInstallLocation = SourceInstallLocation) { kmag suggested that you could use the existing "proxy file" feature, to avoid having to create a new type of install location: https://developer.mozilla.org/en-US/Add-ons/Setting_up_extension_development_environment#Firefox_extension_proxy_file This would require writing a text file to the extension directory in the profile, which contains the path to the addon. You'd still need a way to activate it without a restart and without having to re-scan the whole extensions dir - I think you could easily modify `AddonInstall.initLocalInstall` (http://searchfox.org/mozilla-central/rev/3411332d29d377bf86405fc3ea72461ea0cb0295/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5411) to work with directories as well as files. What do you think?

I tried the MutableDirectoryInstallLocation and yes, it ends up deleting the original source directory: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8322 installAddon is doing that by trying to move I also tried the proxy file route but it forces to modify many things including startInstall: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6306 which stoped me pursuing this. Modifying initLocationInstall was reasonable but startInstall is much less.

ALT+Space and bugzilla immediately send your comment even if it takes 10s to display any html :x So. I'm not sure the proxy file is reasonable, it also sounds like a hack. But I haven't spent too much time with the MutableDirectoryInstallLocation, I may be able to come up with something by still sharing some code with the temporary addon codepath.

Here is an alternative patch that works, based on your suggestion to use MutableDirectoryLocation. Note that I also used the proxy file trick. f? to see if I should move forward with this patch instead? If you agree, it may be good to refactor installAddon to accept a dictionnary as argument? the aExistingId, aCopy, aSymLink are just messy and would be better agregated into an option object.

Comment on attachment 8797800 [details] [diff] [review] alt patch using MutableDirectoryLocation This lgtm, although maybe I wouldn't use the term "symlink" if it's not an actual symbolic link and actually a text file, but I see what you mean :)

(In reply to Alexandre Poirot [:ochameau] from comment #33) > Created attachment 8797800 [details] [diff] [review] > alt patch using MutableDirectoryLocation > > Here is an alternative patch that works, based on your suggestion to use > MutableDirectoryLocation. > Note that I also used the proxy file trick. > f? to see if I should move forward with this patch instead? > > If you agree, it may be good to refactor installAddon to accept a > dictionnary as argument? > the aExistingId, aCopy, aSymLink are just messy and would be better > agregated into an option object. Sure.

Comment on attachment 8797157 [details] Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently. https://reviewboard.mozilla.org/r/82770/#review82848 ::: toolkit/mozapps/extensions/test/xpcshell/test_install_from_sources.js:6 (Diff revision 4) > +/* Any copyright is dedicated to the Public Domain. > + * http://creativecommons.org/publicdomain/zero/1.0/ > + */ > + > +const ID = "bootstrap1@tests.mozilla.org"; > +const sampleRDFManifest = { Is this used anywhere?

Comment on attachment 8797157 [details] Bug 1185460 - Implements AddonManager.installAddonFromSources to install addon from sources permanently. https://reviewboard.mozilla.org/r/82770/#review82852

(In reply to Robert Helmer [:rhelmer] from comment #38) > Comment on attachment 8797157 [details] > Bug 1185460 - Implements AddonManager.installAddonFromSources to install > addon from sources permanently. > > https://reviewboard.mozilla.org/r/82770/#review82848 > > ::: toolkit/mozapps/extensions/test/xpcshell/test_install_from_sources.js:6 > (Diff revision 4) > > +/* Any copyright is dedicated to the Public Domain. > > + * http://creativecommons.org/publicdomain/zero/1.0/ > > + */ > > + > > +const ID = "bootstrap1@tests.mozilla.org"; > > +const sampleRDFManifest = { > > Is this used anywhere? No, I removed it. It's a leftover from a copy paste from test_temporary.js.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/839c876d0dab Implements AddonManager.installAddonFromSources to install addon from sources permanently. r=rhelmer