910473 - Design and implement a proper glue between Webapps.jsm and the various runtime

Status: RESOLVED WONTFIX

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

Relying on observer notifications and a couple of callbacks already streches what we can reasonably do on the desktop runtime, and any change is painful.

Every application (b2g, fx android, fx desktop, webapprt) will need to implement a glue interface to interface with Webapps.jsm

I feel that desktop firefox is our most complicated use case for now so we should drive the design with the needs of desktop firefox in mind.

Comment 1

[Marco Castelluccio [:marco]]

I have written a patch, but it's far from small :D
I've created two components, a UI Glue component and an Installer component (maybe we could give a better name to the Installer component, as it also handles uninstallation, launch and all of the OS specific stuff).

I haven't changed any behavior at the moment and I think we should do that in followup bugs (this separation in different components will allow further simplifications).

Comment 2

[Marco Castelluccio [:marco]]

Attachment: Patch

Please take just a quick look, I still need to fix some build issues (and a b2g issue, for some reason this event [http://mxr.mozilla.org/mozilla-central/source/b2g/chrome/content/shell.js#603] isn't triggered).

Comment 3

[Marco Castelluccio [:marco]]

Attachment: Part 1 - Interfaces and registry

Comment 4

[Marco Castelluccio [:marco]]

Attachment: Part 2 - Browser and Webapp Runtime

Comment 5

[Marco Castelluccio [:marco]]

Attachment: Part 3 - Android

Comment 6

[Marco Castelluccio [:marco]]

Attachment: Part 4 - B2G

Comment 7

[Marco Castelluccio [:marco]]

Attachment: Part 5 - Test fixes

Comment 8

[Marco Castelluccio [:marco]]

Comment on attachment 799644 [details] [diff] [review]
Part 4 - B2G

(I've temporary removed the timestamp property, because it isn't working per bug 912164, I'll re-add it if necessary).

Also, I think I'll modify the launch function like this:

void launch(in mozIApplication app, in jsval params);

Comment 9

[[:fabrice] Fabrice Desré]

Comment on attachment 799644 [details] [diff] [review]
Part 4 - B2G

Review of attachment 799644 [details] [diff] [review]:
-----------------------------------------------------------------

::: b2g/components/WebappsInstaller.js
@@ +6,5 @@
> +
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cu = Components.utils;
> +const Cr = Components.results;

Cr is unused.

::: b2g/components/WebappsUI.js
@@ +6,5 @@
> +
> +const Cu = Components.utils;
> +const Cc = Components.classes;
> +const Ci = Components.interfaces;
> +const Cr = Components.results;

Cr is unused.

@@ +10,5 @@
> +const Cr = Components.results;
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Promise.jsm");

Chrome code can now create promises with new Promise(). Use that instead.

Comment 10

[[:fabrice] Fabrice Desré]

Comment on attachment 799639 [details] [diff] [review]
Part 1 - Interfaces and registry

Review of attachment 799639 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/interfaces/apps/nsIAppsInstaller.idl
@@ +13,5 @@
> +     * Returns a promise that is resolved when the application has been
> +     * installed.
> +     * The promise is resolved with the profile directory path.
> +     */
> +    jsval install(in mozIApplication app, [optional] in jsval parameters);

What are the optional parameters? The same ones from the install()/installPackage() calls?

@@ +17,5 @@
> +    jsval install(in mozIApplication app, [optional] in jsval parameters);
> +
> +    void uninstall(in mozIApplication app);
> +
> +    void launch(in jsval app);

Why is this a jsval and not a mozIApplication?

::: dom/interfaces/apps/nsIAppsUIGlue.idl
@@ +12,5 @@
> +    /*
> +     * Returns a promise that is resolved with |true| if the user confirms
> +     * or |false| if the user denies an installation.
> +     */
> +    jsval askConfirmation(in mozIApplication app, [optional] in jsval parameters);

What are the parameters?

Comment 11

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #10)
> Comment on attachment 799639 [details] [diff] [review]
> Part 1 - Interfaces and registry
> 
> Review of attachment 799639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/interfaces/apps/nsIAppsInstaller.idl
> @@ +13,5 @@
> > +     * Returns a promise that is resolved when the application has been
> > +     * installed.
> > +     * The promise is resolved with the profile directory path.
> > +     */
> > +    jsval install(in mozIApplication app, [optional] in jsval parameters);
> 
> What are the optional parameters? The same ones from the
> install()/installPackage() calls?

Here the only parameter is the categories array.

> @@ +17,5 @@
> > +    jsval install(in mozIApplication app, [optional] in jsval parameters);
> > +
> > +    void uninstall(in mozIApplication app);
> > +
> > +    void launch(in jsval app);
> 
> Why is this a jsval and not a mozIApplication?

As I said in comment 8, I think I'll modify this to be |void launch(in mozIApplication app, [optional] in jsval params);|
The params are needed because we also pass a timestamp and in the future we may want to pass other parameters (for example a parameter to launch the app in debug mode could be useful on Desktop).

> ::: dom/interfaces/apps/nsIAppsUIGlue.idl
> @@ +12,5 @@
> > +    /*
> > +     * Returns a promise that is resolved with |true| if the user confirms
> > +     * or |false| if the user denies an installation.
> > +     */
> > +    jsval askConfirmation(in mozIApplication app, [optional] in jsval parameters);
> 
> What are the parameters?

The parameters here are |from| and |outerWindowID|, they're necessary on desktop for bug 771294 (maybe we want a similar check also on Android and b2g?)

Comment 12

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 799641 [details] [diff] [review]
Part 2 - Browser and Webapp Runtime

(In reply to Marco Castelluccio [:marco] from comment #1)
> I have written a patch, but it's far from small :D
> I've created two components, a UI Glue component and an Installer component
> (maybe we could give a better name to the Installer component, as it also
> handles uninstallation, launch and all of the OS specific stuff).

WebappsInstaller does seem like a strange name for a module that exposes install, launch, and uninstall methods.  But then WebappsUI and nsIAppsUIGlue are both vague (UI for what?).  Naming things is hard.

On a more substantive note, it isn't clear why we have two components for the small set of platform-specific methods we want to expose to the cross-platform registry.  I think we'd be better served with a single component per platform that consolidates the platform-specific API (while the implementation can continue to live in multiple modules as appropriate).


> I haven't changed any behavior at the moment and I think we should do that
> in followup bugs (this separation in different components will allow further
> simplifications).

Good idea!  But that makes it hard to see what kind of feedback I should be providing, as this patch looks to be pure refactoring.  feedback+ on the general plan to segregate platform-specific code, but please let me know if you have a more specific question in mind.

Comment 13

[Marco Castelluccio [:marco]]

Attachment: Part 1 - Interfaces and registry

About the AppsInstaller component, what about calling it "NativeAppsService"? I can't come up with a better name :D

Comment 14

[Marco Castelluccio [:marco]]

Attachment: Part 2 - Browser and Webapp Runtime

I haven't looked yet at the jar: substitution, for now I'm just building the toolkit component only for desktop.

Note that this introduces a little regression. We no longer show the app icon in the alert notification, but we always show the default icon. I think we should get rid of the alert notification soon in favor of something better (maybe a popup notification with a "Launch" button).

Comment 15

[Marco Castelluccio [:marco]]

Attachment: Part 2 - Browser and Webapp Runtime

Forgot to qref.

Comment 16

[Marco Castelluccio [:marco]]

Attachment: Part 3 - Android

Comment 17

[Marco Castelluccio [:marco]]

Attachment: Part 4 - B2G

I haven't found any documentation about using |new Promise()| and using it triggers an undefined error.

Comment 18

[Marco Castelluccio [:marco]]

Attachment: Part 5 - Test fixes

Comment 19

[Marco Castelluccio [:marco]]

Attachment: Part 4 - B2G

Comment 20

[Marco Castelluccio [:marco]]

Attachment: Part 3 - Android

Comment 21

[Marco Castelluccio [:marco]]

Attachment: Part 4 - B2G

Comment 22

[Marco Castelluccio [:marco]]

Attachment: Part 3 - Android

Comment 23

[Marco Castelluccio [:marco]]

Attachment: Part 4 - B2G

Comment 24

[Marco Castelluccio [:marco]]

Attachment: Part 2 - Browser and Webapp Runtime

Updated after the landing of bug 899353.

Hopefully after bug 910466 our receiveMessage will be cleaner.

Comment 25

[Marco Castelluccio [:marco]]

I think these patches will be obsolete soon and I'll need to update them, so if you have anything else to review you can skip these.

Comment 26

[Marco Castelluccio [:marco]]

Attachment: Part 1 - Interfaces and registry

Comment 27

[Marco Castelluccio [:marco]]

Attachment: Part 2 - Browser and Webapp Runtime

Comment 28

[Marco Castelluccio [:marco]]

Attachment: Part 3 - Android

Comment 29

[Marco Castelluccio [:marco]]

Attachment: Part 4 - B2G

Comment 30

[Marco Castelluccio [:marco]]

Attachment: Part 5 - Test fixes

Comment 31

[[:fabrice] Fabrice Desré]

Comment on attachment 806096 [details] [diff] [review]
Part 1 - Interfaces and registry

Review of attachment 806096 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't take a look at the code yet. I'd like to get Myk input on the idl first.

::: dom/interfaces/apps/nsIAppsInstaller.idl
@@ +6,5 @@
> +
> +interface mozIApplication;
> +
> +[scriptable, uuid(a3e931fb-47d3-477d-b83e-7c1105221730)]
> +interface nsIAppsInstaller : nsISupports

I don't like this name. It's not just about installing apps, so maybe nsIAppsLifeCycleManager ? or nsIAppsEmbedder ? let's bikesched ;)

@@ +19,5 @@
> +    AString getInstallPath(in mozIApplication app);
> +
> +    void uninstall(in mozIApplication app);
> +
> +    void launch(in mozIApplication app, [optional] in jsval parameters);

what are the parameters? Just the entry point?

@@ +20,5 @@
> +
> +    void uninstall(in mozIApplication app);
> +
> +    void launch(in mozIApplication app, [optional] in jsval parameters);
> +    void close(in mozIApplication app);

close() also needs an entry point parameter until we get rid of them.

uninstall(), launch() and close() should also return promises since nothing guarantees that they will succeed everywhere.

::: dom/interfaces/apps/nsIAppsUIGlue.idl
@@ +12,5 @@
> +    /*
> +     * Returns a promise that is resolved with |true| if the user confirms
> +     * or |false| if the user denies an installation.
> +     */
> +    jsval askConfirmation(in mozIApplication app, [optional] in jsval parameters);

s/askConfirmation/askInstallConfirmation

I think we also need one for uninstalls.

Lastly, is there real value in having 2 interfaces there? I would be ok with a single "Embedding" interface.

Comment 32

[Marco Castelluccio [:marco]]

(In reply to Fabrice Desré [:fabrice] from comment #31)
> ::: dom/interfaces/apps/nsIAppsInstaller.idl
> @@ +6,5 @@
> > +
> > +interface mozIApplication;
> > +
> > +[scriptable, uuid(a3e931fb-47d3-477d-b83e-7c1105221730)]
> > +interface nsIAppsInstaller : nsISupports
> 
> I don't like this name. It's not just about installing apps, so maybe
> nsIAppsLifeCycleManager ? or nsIAppsEmbedder ? let's bikesched ;)

I'm really bad with names... :D
What about something with "native" in it? Like "nsIAppsNativeService" or "nsINativeAppsService".
Or something with "OS", like "nsIAppsOSService"?

> @@ +19,5 @@
> > +    AString getInstallPath(in mozIApplication app);
> > +
> > +    void uninstall(in mozIApplication app);
> > +
> > +    void launch(in mozIApplication app, [optional] in jsval parameters);
> 
> what are the parameters? Just the entry point?

The entry point and the timestamp, maybe we may want to add other parameters in the future (like a parameter to launch the app in debug mode).

> @@ +20,5 @@
> > +
> > +    void uninstall(in mozIApplication app);
> > +
> > +    void launch(in mozIApplication app, [optional] in jsval parameters);
> > +    void close(in mozIApplication app);
> 
> close() also needs an entry point parameter until we get rid of them.
> 
> uninstall(), launch() and close() should also return promises since nothing
> guarantees that they will succeed everywhere.

Ok. Is there a better way to specify that they return a promise? (instead of jsval)

> ::: dom/interfaces/apps/nsIAppsUIGlue.idl
> @@ +12,5 @@
> > +    /*
> > +     * Returns a promise that is resolved with |true| if the user confirms
> > +     * or |false| if the user denies an installation.
> > +     */
> > +    jsval askConfirmation(in mozIApplication app, [optional] in jsval parameters);
> 
> s/askConfirmation/askInstallConfirmation
> 
> I think we also need one for uninstalls.

We haven't a UI for uninstalls at the moment, can we add it in another bug? (to avoid these patches to become huge)

> Lastly, is there real value in having 2 interfaces there? I would be ok with
> a single "Embedding" interface.

I thought two interfaces could be good for tests (we may want to mock only the UI component and not the installer component, or the other way around) and, mostly, for the Webapp Runtime where we want the same Installer but a different UIGlue.

Comment 33

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 806096 [details] [diff] [review]
Part 1 - Interfaces and registry

Hmm, there are a lot of considerations here.

* Ideally the application-specific functionality would not have a public
  interface, since we don't intend for consumers to access it directly
  but rather through the mozApps API.  So my first thought was to put the code
  into subscripts loaded via the subscript loader.

* But we want tests to be able to override it, and that generally requires
  it to be accessible to tests via either a public XPCOM interface (which can
  be overridden by a test-specified component) or a public JavaScript Module
  interface (which can be overridden by direct modifications to the scope
  of the module instance).

* The implementations are application-specific, so it doesn't make sense to put
  them into a Gecko directory like dom/apps/.
* But neither does it make sense to put them into application directories
  that the Gecko implementation then references, since that creates a Gecko
  dependency on the application.

* The applications in question (B2G, Fennec, Firefox) are all Toolkit-based,
  so the implementations could live in toolkit/.
* But that creates a Gecko dependency on Toolkit (and potentially a Toolkit
  dependency on the application), which is just as bad.


I'm not sure how far we want to go in this bug, but if I was designing the implementation from scratch, I would:

* create application-specific implementations, one per application, that live
  in the application directories (i.e. b2g/, mobile/android/, and browser/)
  and implement the public-facing interface specified by the mozApps API;

* delegate most work to the cross-application implementation in dom/apps/,
  overriding that implementation only when necessary;

* refactor API calls that involve both app-specific and cross-app operations
  (like *install*) into sequences driven entirely by the app-specific code,
  so cross-app code never calls into app-specific code;


Then, for test-overridability, I would:

* implement both the app-specific and the cross-app code as JavaScript Modules
  and rely on the ability of tests to override their global scopes, including
  their exported objects;

* make the minimal additional changes necessary to support test overrides,
  like waiting to initialize an object until after module evaluation, so a test
  can import the module and then modify the object before it gets initialized
  (without exposing a public method for modifying the object).


And as for the name of the API, I would:

* prefer "webapp" to "app" or "application", because the latter two terms
  are overloaded in the codebase to refer to Gecko-based applications
  (B2G, Fennec, Firefox, etc.);

* use the plural "webapps" for an interface providing access to apps generally,
  *except* when the word is used as an adjective modifying another noun,
  in which case it should be singular (f.e. "WebappService") per grammar
  principles and because it's simpler and easier to read and write;

* avoid using qualifiers like "native" or "os" or "platform"
  because the implementation in question is actually application-specific
  (although there may be some OS-specific functionality that we preprocess
  or further isolate into OS-specific modules).


If we don't want to go that far, however, then the current direction (of defining interfaces to app-specific functionality that the cross-app implementation calls as needed) seems reasonable, except that I would:

* specify a single interface to app-specific functionality, for simplicity,
  since it isn't necessary to have multiple interfaces in order to support test
  overridability;

* use modules rather than components, which are easier to develop, maintain,
  and use;

* prefer a generic name like Webapps.jsm loaded from an application-specific
  resource: alias like resource://app/modules/, although that hasn't been done
  before, so it'd require more thought and input from others).

(Alternately, you could put the module into one of the existing standard locations like resource://gre/modules/ or resource:///modules/ and give it the generic name WebappEmbedding.jsm, although the webapp runtime's implementation would probably still need a different name or location, since we ship it alongside Firefox.)

Comment 34

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 806098 [details] [diff] [review]
Part 2 - Browser and Webapp Runtime

Given a conversation I had with Marco on Friday about preferring modules to components, plus my feedback earlier today in this bug, I think this review request is now obsolete, so I'm cancelling it.  But please rerequest review if you think it's still relevant!

Comment 35

[[:fabrice] Fabrice Desré]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #33)
> Comment on attachment 806096 [details] [diff] [review]
> Part 1 - Interfaces and registry

> I'm not sure how far we want to go in this bug, but if I was designing the
> implementation from scratch, I would:
> 
> * create application-specific implementations, one per application, that live
>   in the application directories (i.e. b2g/, mobile/android/, and browser/)
>   and implement the public-facing interface specified by the mozApps API;

That's very different from what we do for all DOM apis. The usual design is to have one single dom frontend (what we have int Webapps.js currently) and different dom backends per application and/or platform (usually per application by in our case desktop is so special...).

In our case we have a common frontend, a partially common backend, and application-specific backend pieces. I'm a bit afraid to let each application module do too much coordination directly with the dom front-end and still think that the common backend should be in the driver seat, though I could be convinced otherwise.

Comment 36

[Myk Melez [:myk] [@mykmelez]]

(In reply to Fabrice Desré [:fabrice] from comment #35)
> That's very different from what we do for all DOM apis. The usual design is
> to have one single dom frontend (what we have int Webapps.js currently) and
> different dom backends per application and/or platform (usually per
> application by in our case desktop is so special...).

I think that's basically what I'm suggesting, except that I didn't include the DOM frontend.  Nevertheless, that frontend doesn't change things much, since it is pretty skeletel, and seems to exist mostly because a DOM API needs to be implemented as an XPCOM interface.  So we could include the common DOM frontend while still implementing substantially what I suggested.

Then the mozApps XPCOM interface (dom/apps/src/nsIDOMApplicationRegistry.idl) and its XPCOM component (dom/apps/src/Webapps.js) would continue to live in dom/, but the component would import an application module (f.e. browser/modules/Webapps.jsm), which would implement app-specific functionality while itself importing a common module (dom/apps/src/Webapps.jsm) to obtain shared functionality.


> In our case we have a common frontend, a partially common backend, and
> application-specific backend pieces. I'm a bit afraid to let each
> application module do too much coordination directly with the dom front-end
> and still think that the common backend should be in the driver seat, though
> I could be convinced otherwise.

I'm afraid of it too!  Mostly because it's a substantial change from the current implementation.  In theory, it makes more sense, because it establishes a more conventional dependency relationship between the application and platform modules.  And because the mozApps DOM API is different from most others, because the application intervenes quite substantially in its implementation (whereas other DOM APIs either have no app-specific functionality or merely ask the user for permission to do something).

But if switching driver and passenger feels like too big or risky a change, then I'm ok with the smaller step of leaving the common backend in the driver seat.  Either way will be better than what we have now!

Comment 37

[Marco Castelluccio [:marco]]

I've had another idea:
 - The dom/apps/src/Webapps.jsm module will define a DOMApplicationRegistry object where the cross-platform functionality resides.
 - The application specific modules will define a *AppSpecific*DOMApplicationRegistry object that inherits from DOMApplicationRegistry.

The parent class, DOMApplicationRegistry, will provide an implementation of the procedures that calls the application specific functions when needed.

For example:

DOMApplicationRegistry::doInstall() {
  // Do something

  this.install();  // This function will be defined in the application specific objects.

  // Do something
}

This way we can add hooks to extend the DOMApplicationRegistry functionality (when several app-specific objects need to extend the functionality in the same way, for example in the install case).
Otherwise we could extend the functionality by overriding the DOMApplicationRegistry functions in the children (when only one app-specific object needs to extend a functionality):

B2GDOMApplicationRegistry::loadRegistry() {
  // Do some B2G-specific initialization stuff.

  DOMApplicationRegistry.prototype.loadRegistry.call(this);
}

Comment 38

[Marco Castelluccio [:marco]]

Attachment: WIP

Comment 39

[Marco Castelluccio [:marco]]

Oh, my comment got removed (or more probably I forgot to write it down).

WebappManager.jsm will be the glue module, providing the following functions: |askInstallConfirmation|, |createProfile|, |install|, |update|, |getPackagePath|, |launch|, |uninstall|, |isLaunchable|.

In the desktop land, NativeApp.jsm will no longer be used only to install apps, but will also do some of the stuff that WebappOSUtils.jsm used to do (and WebappOSUtils.jsm will be removed, part of the module will go in NativeApp.jsm and part in WebappManager.jsm). WebappManager.jsm will be the interface between the registry and the NativeApp module, there will be one WebappManager module for Firefox and one for the webapp runtime.

Comment 40

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 8411910 [details] [diff] [review]
WIP

Review of attachment 8411910 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Marco Castelluccio [:marco] from comment #37)
> I've had another idea:
>  - The dom/apps/src/Webapps.jsm module will define a DOMApplicationRegistry
> object where the cross-platform functionality resides.
>  - The application specific modules will define a
> *AppSpecific*DOMApplicationRegistry object that inherits from
> DOMApplicationRegistry.

That makes sense to me, although it isn't what this patch is doing, as this patch makes Webapps.jsm import WebappManager.jsm and then makes DOMApplicationRegistry access WebappManager instead of creating an app-specific DOMApplicationRegistry object that inherits from the cross-platform implementation.


(In reply to Marco Castelluccio [:marco] from comment #39)
> WebappManager.jsm will be the glue module, providing the following
> functions: |askInstallConfirmation|, |createProfile|, |install|, |update|,
> |getPackagePath|, |launch|, |uninstall|, |isLaunchable|.

Aren't these the kinds of methods that an app-specific DOMApplicationRegistry would override?  Based on your earlier comment, I was under the impression that the app-specific DOMApplicationRegistry object would be the glue object.


> In the desktop land, NativeApp.jsm will no longer be used only to install
> apps, but will also do some of the stuff that WebappOSUtils.jsm used to do
> (and WebappOSUtils.jsm will be removed, part of the module will go in
> NativeApp.jsm and part in WebappManager.jsm). WebappManager.jsm will be the
> interface between the registry and the NativeApp module, there will be one
> WebappManager module for Firefox and one for the webapp runtime.

That all makes sense!


Anyway, feedback+ for the general direction, but I could use some clarification about your current thinking.

Comment 41

[Marco Castelluccio [:marco]]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #40)
> Comment on attachment 8411910 [details] [diff] [review]
> WIP
> 
> Review of attachment 8411910 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Marco Castelluccio [:marco] from comment #37)
> > I've had another idea:
> >  - The dom/apps/src/Webapps.jsm module will define a DOMApplicationRegistry
> > object where the cross-platform functionality resides.
> >  - The application specific modules will define a
> > *AppSpecific*DOMApplicationRegistry object that inherits from
> > DOMApplicationRegistry.
> 
> That makes sense to me, although it isn't what this patch is doing, as this
> patch makes Webapps.jsm import WebappManager.jsm and then makes
> DOMApplicationRegistry access WebappManager instead of creating an
> app-specific DOMApplicationRegistry object that inherits from the
> cross-platform implementation.

Me and Fabrice talked over IRC and agreed upon going with the simpler solution
for the time being (to avoid adding other observer notifications in bug 100315)
because of the stall on the design decisions.

Actually, modifying the simpler solution with the WebappManager to the other
design with the inheritance should be easy so I might do it now if Fabrice
agrees (or in a follow-up if it turns out to be too much work, I haven't
researched yet the best way to implement it).

At the moment I have most of the stuff working on desktop and b2g (I'm having
problems manually testing on b2g desktop builds because the mouse input is being
ignored (?), but mochitests are passing).

I'm a bit stuck on a decision, I could use your help here.

In AppsUtils.jsm we're using WebappOSUtils.jsm to get the package path (in getAppInfo).
getAppInfo is used by both Webapps.jsm and AppsServiceChild.jsm.
WebappOSUtils is now gone in favor of WebappManager, but WebappManager depends on
AppsUtils (because it uses ManifestHelper). This creates a circular dependency (AppsUtils <-> WebappManager).
I could either:
1) Remove AppsUtils::getAppInfo and copy it to both Webapps.jsm and AppsServiceChild.jsm.
2) Create a new ManifestHelper module so that WebappManager imports ManifestHelper but not AppsUtils.
3) Move AppsUtils::getAppInfo to Webapps.jsm and make "Webapps::GetList" return the getAppInfo return value too. This way AppsServiceChild.jsm doesn't need to call getAppInfo because the info is already cached.
4) Live with the circular dependency.

In the solution with the inheritance, getPackagePath would be a DOMApplicationRegistry function. This would make the first, the second and the fourth solutions unfeasible (because AppsServiceChild.jsm would need to import DOMApplicationRegistry, but it can't).
So I guess the third solution would be the best. What do you think? Any other ideas?

Comment 42

[[:fabrice] Fabrice Desré]

(In reply to Marco Castelluccio [:marco] from comment #41)

> In the solution with the inheritance, getPackagePath would be a
> DOMApplicationRegistry function. This would make the first, the second and
> the fourth solutions unfeasible (because AppsServiceChild.jsm would need to
> import DOMApplicationRegistry, but it can't).
> So I guess the third solution would be the best. What do you think? Any
> other ideas?

I'm fine with doing 3). Just don't forget to also change the Webapps::AddApp messages to include what we have in getAppInfo!

Comment 43

[Myk Melez [:myk] [@mykmelez]]

Per bug 1238576, we're going to stop exposing navigator.mozApps on desktop/Android (except where MOZ_B2G is set, such as B2G Desktop), so we won't fix this bug.