1071311 - (sdk-feature) It should be possible to integrate add-ons as features (via copy and paste)

Status: REOPENED

(Firefox :: General, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

For details please read http://work.erikvold.com/jetpack/2014/02/04/the-addon-feature.html

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Note: I spoke to Dave Townsend about this when he was my manager and he said he would like me to work on this after native jetpack is done.  He told me he spoke to Dave Camp about it as well.  I have told Joe Walker about this idea now too.

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

In the past we've integrated add-ons as features, such as Scratchpad (which had no tests), PDF.js, Tab Groups, and I'm sure there are more examples..

When add-ons of the past were integrated into Firefox, they usually had no tests, because writing tests for old school add-ons isn't easy, and if they had tests they would likely have to be rewritten or converted to work with one of the test frameworks which m-c supports https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing

Also, when add-ons of the past were integrated into Firefox, they would often have their source changed and often spread throughout m-c in various folders, such that the source no longer followed a structure that would be a valid for an add-on.

My idea is this simply:

One should be able to make a jetpack add-on, write jetpack tests, and if it is cool enough, they it would land in m-c unchanged *WITH EXACTLY THE SAME STRUCTURE*, such that a contributor could install the source as an add-on, which would disable the feature and use the add-on source instead, so that they can make patches for the add-on feature without every touching m-c or hg, and without having to compile anything.

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Irakli did you want to take this one?

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

cc'ing Joe

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

First of all, I thin it would be just wrong to try and tackle this issues without having native-jetpack support. But even if we had that I feel like there are bunch of pieces that need to be put in place to make this possible. I'm afraid I can't tackle this along with my other goals. That being said I think we'll learn enough by making web-console swappable to be able to improve on it with something like sdk-features.

P.S. I'm also still not convinced about `jetpack-test-options.json` thing, maybe that will come more clear in a future though.

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: https://github.com/mozilla/gecko-dev/compare/master...erikvold:addon-feature

This is a ugly hack, but it's mostly working, any feedback on how to make it less ugly and more likely to land are welcome!

Also suggestions for tests!

Comment 7

[Nick Alexander :nalexander [he/him]]

I can't comment on the GH link (why is it not a PR?) but you have _boostraps in at least one place.

Comment 8

[Nick Alexander :nalexander [he/him]]

This is nifty.  I'm a little worried that the JPM style of having node_modules included in the packaged XPI won't work well when there are multiple addons all including the same sub-library (possibly with different versions).  Presumably we have this issue already.  Perhaps others think this is not a concern?

I'm interested in helping make the moz.build integration simpler, when the time comes, so that we don't have to muck with EXTRA_JAVASCRIPT/EXTRA_PP_JAVASCRIPT and registering jar.mn files everywhere.  Maybe we can get to a single "Include this Jetpack directory" definition that determines what should happen from the package.json and directory structure.

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: https://github.com/erikvold/gecko-dev/pull/6

Looking for ways to improve this and any issues you might see.

With this patch I think the Add-on Manager wiring would need a great deal of testing, for error cases while disabling/enabling, but it shouldn't be used much on the other hand.  I was also thinking about making it possible to opt-out of these add-on features with prefs, like `features.jetpacks.<id>.disabled`, which may be the easier first step than hooking in to the Add-on Manager.

I'm also thinking that we should only re-enable the built-in add-on feature, when the add-on/xpi version is uninstalled.

If this is at all confusing I can write something more detailed out, so please let me know!

Comment 10

[Dave Townsend [:mossop]]

(In reply to Nick Alexander :nalexander from comment #7)
> I can't comment on the GH link (why is it not a PR?) but you have _boostraps
> in at least one place.

The owners of the gecko-dev repository object when you create pull requests on it :(

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #10)
> (In reply to Nick Alexander :nalexander from comment #7)
> > I can't comment on the GH link (why is it not a PR?) but you have _boostraps
> > in at least one place.
> 
> The owners of the gecko-dev repository object when you create pull requests
> on it :(

Yeah, that's fine tho, I just directed the pull request at my gecko-dev fork instead, which I should have done earlier; I just didn't realize it wasn't possible to comment then.

Comment 12

[Dave Townsend [:mossop]]

Comment on attachment 8601568 [details] [review]
https://github.com/erikvold/gecko-dev/pull/6

While keeping this for jetpacks only is the simplest option I don't think it is the right one. It doesn't help pdf.js or shumway if we wanted to move them to this model, I can imagine others that might want to use raw restartless add-ons too. Also it looks like AddonFeatureManager already has much of the logic we'd want to use for native jetpacks, that should be in a central place.

You should create a BootstrapManager module and move the code that handles loading bootstraps from XPIProvider.jsm to it. Then both XPIProvider and AddonFeatureManager can call it giving it presumably a file location, id and type and let it figure out whether it needs to load bootstrap.js, run the native jetpack magic etc. based on type.

One issue I see here is that we start up every feature on startup even though the add-on manager may also be starting up matching add-ons. One way around that would be to make sure AddonFeatureManager does its work after the add-ons manager does and have BootstrapManager refuse to start a feature if an add-on has already been started.

I bet we could build config.json at build time with a simple python script. I'm not sure it needs to contain anything except ID and a type, rootURI can be calculated at runtime cheaply as long as we put them all in the same place.

I'm not sure whether features should be disabled whenever the same add-on is installed or just enabled, I could go either way there. Also listening to a pref to turn features on and off is a great idea, we could build UI for it in the future but even without it lets us build features that can be entirely disabled in beta/release by hotfixes if we find issues.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #12)
> Comment on attachment 8601568 [details] [review]
> https://github.com/erikvold/gecko-dev/pull/6
> 
> While keeping this for jetpacks only is the simplest option I don't think it
> is the right one. It doesn't help pdf.js or shumway if we wanted to move
> them to this model, I can imagine others that might want to use raw
> restartless add-ons too. Also it looks like AddonFeatureManager already has
> much of the logic we'd want to use for native jetpacks, that should be in a
> central place.


I think we should try to deprecate old-school chrome.manifest and restartless add-ons, or at the very least greatly promote jetpacks over those types of add-ons, so I do not think that trying to support landing those legacy style add-ons as features is a worthwhile investment of Mozilla's resources.

I think it would be a better use of Mozilla's resources to have pdf.js and shumway converted to be jetpacks and anyhow they are already shipped with Fx I thought, so there I see no urgency to help those projects.

Comment 14

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8601568 [details] [review]
https://github.com/erikvold/gecko-dev/pull/6

I made some small comments on GH.

Additional issues raised in a mobile meeting this AM:

* localization: maybe this isn't an issue since all .properties files are in tree.

* testing: I guess since this is all toggle-able at runtime this actually isn't a big deal.

Comment 15

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #13)
> (In reply to Dave Townsend [:mossop] from comment #12)
> > Comment on attachment 8601568 [details] [review]
> I think we should try to deprecate old-school chrome.manifest and
> restartless add-ons, or at the very least greatly promote jetpacks over
> those types of add-ons, so I do not think that trying to support landing
> those legacy style add-ons as features is a worthwhile investment of
> Mozilla's resources.

I agree with deprecating non-restartless add-ons and I support promoting jetpacks over other add-ons, but given that we have almost no resourcing to work on jetpack itself I don't know that it makes sense to restrict this in this way. We have other things wanting to ship as add-ons that aren't jetpacks already or wouldn't want to incur the additional penalty of jetpack (bug 1156135 for example, or the pocket integration work).

> I think it would be a better use of Mozilla's resources to have pdf.js and
> shumway converted to be jetpacks and anyhow they are already shipped with Fx
> I thought, so there I see no urgency to help those projects.

No urgency to change, just pointing out that every example we've had of something wanting to ship in Firefox hasn't been a jetpack add-on yet.

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #15)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #13)
> > (In reply to Dave Townsend [:mossop] from comment #12)
> > > Comment on attachment 8601568 [details] [review]
> > I think we should try to deprecate old-school chrome.manifest and
> > restartless add-ons, or at the very least greatly promote jetpacks over
> > those types of add-ons, so I do not think that trying to support landing
> > those legacy style add-ons as features is a worthwhile investment of
> > Mozilla's resources.
> 
> I agree with deprecating non-restartless add-ons and I support promoting
> jetpacks over other add-ons, but given that we have almost no resourcing to
> work on jetpack itself.

We recently had resources first of all, and we could have resources invested in jetpack again obviously. Currently I work on it full time, and you appear to be working on it part time too, and now I'm being asked to work on supporting Google Chrome Extensions with Jetpack, are you saying this is a wasted effort Mossop?

Just looking at the Github graph of contribution https://github.com/mozilla/addon-sdk/graphs/contributors there has been more active contributions in first quarter of 2015 than the project had in the first half of 2014 when it had more resources.  This doesn't even include the contributions to:

* https://github.com/mozilla/jpm
* https://github.com/mozilla/jpm-core
* https://github.com/erikvold/jpm-mobile

> I don't know that it makes sense to restrict this in
> this way. We have other things wanting to ship as add-ons that aren't
> jetpacks already or wouldn't want to incur the additional penalty of jetpack
> (bug 1156135 for example, or the pocket integration work).

What other things do we want to ship that are not add-ons?  should we not make those depend on this bug?

Also what "penalty of jetpack" are you referring to?

> > I think it would be a better use of Mozilla's resources to have pdf.js and
> > shumway converted to be jetpacks and anyhow they are already shipped with Fx
> > I thought, so there I see no urgency to help those projects.
> 
> No urgency to change, just pointing out that every example we've had of
> something wanting to ship in Firefox hasn't been a jetpack add-on yet.

What examples?  I think the data sample you're thinking here is low first of all, and second of all they may be dated examples.  If we want to count the number of add-on/feature projects that could potentially land, or want to land I'm sure we can come up with different data sets since our criteria seems different.

Currently devtools is shaping it's code base in to merely being a jetpack, which is a larger code base than other examples of add-ons shipping in the browser in the past.

Furthermore converting restartless add-ons to jetpacks would be trivial, the code base size would be reduced, it would easier to understand the code in order bring in more contributions, and make it more reliable since the sdk has tests for unloading.

Comment 17

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #15)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #13)
> > (In reply to Dave Townsend [:mossop] from comment #12)
> > > Comment on attachment 8601568 [details] [review]
> No urgency to change, just pointing out that every example we've had of
> something wanting to ship in Firefox hasn't been a jetpack add-on yet.

Also fwiw, I reason I wrote a patch for this in the first place was that Bryan Clark asked me how hard it would be to land a jetpack as a feature, so that something like pocket (or another porject) could ship as a jetpack, so I whipped this together for that purpose.

Comment 18

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #16)
> (In reply to Dave Townsend [:mossop] from comment #15)
> > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #13)
> > > (In reply to Dave Townsend [:mossop] from comment #12)
> > > > Comment on attachment 8601568 [details] [review]
> > > I think we should try to deprecate old-school chrome.manifest and
> > > restartless add-ons, or at the very least greatly promote jetpacks over
> > > those types of add-ons, so I do not think that trying to support landing
> > > those legacy style add-ons as features is a worthwhile investment of
> > > Mozilla's resources.
> > 
> > I agree with deprecating non-restartless add-ons and I support promoting
> > jetpacks over other add-ons, but given that we have almost no resourcing to
> > work on jetpack itself.
> 
> We recently had resources first of all, and we could have resources invested
> in jetpack again obviously. Currently I work on it full time, and you appear
> to be working on it part time too, and now I'm being asked to work on
> supporting Google Chrome Extensions with Jetpack, are you saying this is a
> wasted effort Mossop?

I'm gald to hear you work full-time on it, I wasn't aware of that. Unfortunately my part time contribution comes from e10s work only right now.

As for Google Chrome extension support I think that's a really valuable feature. Whether it is based on Jetpack or not seems like an implementation detail.

> > I don't know that it makes sense to restrict this in
> > this way. We have other things wanting to ship as add-ons that aren't
> > jetpacks already or wouldn't want to incur the additional penalty of jetpack
> > (bug 1156135 for example, or the pocket integration work).
> 
> What other things do we want to ship that are not add-ons?  should we not
> make those depend on this bug?

But 1156135 is the most recent example I know of right now but that apparently needs to ship sooner than this work can. The pocket integration work would have probably been another example and has already landed.

> Also what "penalty of jetpack" are you referring to?

Jetpack uses a pretty large set of bootstrap code to bring up the module loader, load l10n resources, the add-on window (hopefully going away) and probably thing I'm forgetting. Many of the high-level modules import pretty large graphs of modules and we don't share any of that across add-ons right now. For many add-ons that pales into comparison to the add-on code itself. For some it outweighs it. Some of that is fixable of course.

> Furthermore converting restartless add-ons to jetpacks would be trivial, the
> code base size would be reduced, it would easier to understand the code in
> order bring in more contributions, and make it more reliable since the sdk
> has tests for unloading.

I disagree with the "trivial" here but you're right that without specific examples it's worthless to talk about that.

All I'm saying is that we should share the code that loads a restartless add-on into a central module, then both this feature loader and add-ons manager can use that so we only have one piece of code to update say when we want to make changes to jetpack loading. It also gives us the benefit of handling the choice between app feature/add-on on startup. We can still say the only features we'll take in Firefox are jetpack based if we want, but it gives us flexibility if we decide there is a case where that isn't the best choice. It also means that this features stuff works in applications where jetpack doesn't.

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #18)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #16)
> > (In reply to Dave Townsend [:mossop] from comment #15)
> > > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #13)
> > > > (In reply to Dave Townsend [:mossop] from comment #12)
> > > > > Comment on attachment 8601568 [details] [review]
> > > > I think we should try to deprecate old-school chrome.manifest and
> > > > restartless add-ons, or at the very least greatly promote jetpacks over
> > > > those types of add-ons, so I do not think that trying to support landing
> > > > those legacy style add-ons as features is a worthwhile investment of
> > > > Mozilla's resources.
> > > 
> > > I agree with deprecating non-restartless add-ons and I support promoting
> > > jetpacks over other add-ons, but given that we have almost no resourcing to
> > > work on jetpack itself.
> > 
> > We recently had resources first of all, and we could have resources invested
> > in jetpack again obviously. Currently I work on it full time, and you appear
> > to be working on it part time too, and now I'm being asked to work on
> > supporting Google Chrome Extensions with Jetpack, are you saying this is a
> > wasted effort Mossop?
> 
> I'm gald to hear you work full-time on it, I wasn't aware of that.
> Unfortunately my part time contribution comes from e10s work only right now.
> 
> As for Google Chrome extension support I think that's a really valuable
> feature. Whether it is based on Jetpack or not seems like an implementation
> detail.


That's a detail that matters, because implementing google chrome extensions without the sdk, would take at least twice as much work, since I would have to recreate modules which already exist in the sdk.  Reinventing the wheel seems like a waste of time to me.


> > > I don't know that it makes sense to restrict this in
> > > this way. We have other things wanting to ship as add-ons that aren't
> > > jetpacks already or wouldn't want to incur the additional penalty of jetpack
> > > (bug 1156135 for example, or the pocket integration work).
> > 
> > What other things do we want to ship that are not add-ons?  should we not
> > make those depend on this bug?
> 
> But 1156135 is the most recent example I know of right now but that
> apparently needs to ship sooner than this work can. The pocket integration
> work would have probably been another example and has already landed.


Well I guess we have different information about the pocket situation..


> > Also what "penalty of jetpack" are you referring to?
> 
> Jetpack uses a pretty large set of bootstrap code to bring up the module
> loader, load l10n resources, the add-on window (hopefully going away) and
> probably thing I'm forgetting. Many of the high-level modules import pretty
> large graphs of modules and we don't share any of that across add-ons right
> now. For many add-ons that pales into comparison to the add-on code itself.
> For some it outweighs it. Some of that is fixable of course.

So there are 4 issues I see here:

* module loader - this can be addressed with a shared loader, but I think we have to consider this a premature optimization concern.
* l10n resources - this is not a concern with the native loader (used with JPM), which uses the properties file and not a json file.
* add-on window - this is being removed, and I'd like to note that I refused to r+ the module in the first place, and now it looks like I'll be the one that has to rip it out..
* module sharing - this can be addressed with a shared loader, and by other means, but I think we have to consider this a premature optimization concern too.


> > Furthermore converting restartless add-ons to jetpacks would be trivial, the
> > code base size would be reduced, it would easier to understand the code in
> > order bring in more contributions, and make it more reliable since the sdk
> > has tests for unloading.
> 
> I disagree with the "trivial" here but you're right that without specific
> examples it's worthless to talk about that.
> 
> All I'm saying is that we should share the code that loads a restartless
> add-on into a central module, then both this feature loader and add-ons
> manager can use that so we only have one piece of code to update say when we
> want to make changes to jetpack loading. It also gives us the benefit of
> handling the choice between app feature/add-on on startup. We can still say
> the only features we'll take in Firefox are jetpack based if we want, but it
> gives us flexibility if we decide there is a case where that isn't the best
> choice. It also means that this features stuff works in applications where
> jetpack doesn't.


I realize you are arguing for supporting restartless add-ons, I am arguing that would burn more of Mozilla's resources, and surely my own time.  Anyhow I won't write that patch, and I worked on this patch in my free time, so I'll let the bug go.

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Dave Townsend [:mossop] from comment #18)
> (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #16)
> > (In reply to Dave Townsend [:mossop] from comment #15)
> > Furthermore converting restartless add-ons to jetpacks would be trivial, the
> > code base size would be reduced, it would easier to understand the code in
> > order bring in more contributions, and make it more reliable since the sdk
> > has tests for unloading.
> 
> I disagree with the "trivial" here but you're right that without specific
> examples it's worthless to talk about that.


Can you give us an example of a restartless add-on Mozilla would like to land that you think would be non trivial to convert to a jetpack please?  I've converted many restartless add-ons in the past and have always found that work to be trivial, seeing one example that would back up your point would help a lot..

Comment 21

[Dave Townsend [:mossop]]

(In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #20)
> (In reply to Dave Townsend [:mossop] from comment #18)
> > (In reply to Erik Vold [:erikvold] (please needinfo? me) from comment #16)
> > > (In reply to Dave Townsend [:mossop] from comment #15)
> > > Furthermore converting restartless add-ons to jetpacks would be trivial, the
> > > code base size would be reduced, it would easier to understand the code in
> > > order bring in more contributions, and make it more reliable since the sdk
> > > has tests for unloading.
> > 
> > I disagree with the "trivial" here but you're right that without specific
> > examples it's worthless to talk about that.
> 
> 
> Can you give us an example of a restartless add-on Mozilla would like to
> land that you think would be non trivial to convert to a jetpack please? 
> I've converted many restartless add-ons in the past and have always found
> that work to be trivial, seeing one example that would back up your point
> would help a lot..

As I said I don't have specific examples worth talking about here.

Comment 22

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 8601568 [details] [review]
https://github.com/erikvold/gecko-dev/pull/6

I like this.  I'm not sure about the exact implementation, I'll leave that to others.  But it carries a lot of desirable properties in terms of being able to ship features faster and update out of band.  

In terms of future integrations with partners this kind of system could be an option to ship those features faster and yet keep them outside the browser core.  However with the partner related integrations we also want to sandbox them a bit from the core browser.  The SocialAPI had this concept by requiring partners to implement web apps that could have additional hooks within the browser. So it might not be exactly what partner integrations needs, but could be close at least.

In terms of just regular feature develop this does seem like it could speed up the pace of trying things out and testing them on a subset of users.

Comment 23

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

0.  Awesome work on exploring this!  It's not trivial, and it's brave to show off WIP here, and expose it to critique.
1.  Support Chrome Addons *as is*, ++, awesome!  (by which I mean, they would get wrapped into jetpacks :) )
2.  I get burned hard by *experimenting as jetpack*, *landing as native*, at both ends.  Knowing that the feature will eventually have chrome privs means I don't actually need the insulation the jetpack provides.  Then when landing, I have to 'unconvert' it anyway.  I don't have a good solution.  
3.  I want to be able to build apps around Firefox data stores:  places, passwords, prefs.  I want to build those apps as though they are web apps, using npm, webpack, karma, etc.  Any place where jetpack interferes with that is painful.  Building things that are 'mostly website' feels hard to me.  
4. I would rather have a well-stubbed / shimmed "Firefox" object that goes into my website code, maybe.  Hard to articulate :)

Comment 24

[BMO Automation]

Per policy at https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Inactive_Bugs. If this bug is not an enhancement request or a bug not present in a supported release of Firefox, then it may be reopened.