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

RESOLVED WONTFIX

Status

RESOLVED WONTFIX
5 years ago
10 months ago

People

(Reporter: fabrice, Assigned: marco)

Tracking

(Blocks: 2 bugs)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 23 obsolete attachments)

WIP
29.39 KB, patch
fabrice
: feedback+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
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.
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
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).
Created attachment 799624 [details] [diff] [review]
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).
Attachment #799624 - Flags: feedback?(myk)
Attachment #799624 - Flags: feedback?(fabrice)
Created attachment 799639 [details] [diff] [review]
Part 1 - Interfaces and registry
Attachment #799639 - Flags: feedback?(fabrice)
Attachment #799624 - Flags: feedback?(myk)
Attachment #799624 - Flags: feedback?(fabrice)
Created attachment 799641 [details] [diff] [review]
Part 2 - Browser and Webapp Runtime
Attachment #799641 - Flags: feedback?(myk)
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);
Attachment #799644 - Flags: feedback?(fabrice)
(Reporter)

Updated

5 years ago
Attachment #799624 - Attachment is obsolete: true
(Reporter)

Comment 9

5 years ago
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.
Attachment #799644 - Flags: feedback?(fabrice) → feedback+
(Reporter)

Comment 10

5 years ago
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?
Attachment #799639 - Flags: feedback?(fabrice)
(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 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.
Attachment #799641 - Flags: feedback?(myk) → feedback+
Created attachment 801671 [details] [diff] [review]
Part 1 - Interfaces and registry

About the AppsInstaller component, what about calling it "NativeAppsService"? I can't come up with a better name :D
Attachment #799639 - Attachment is obsolete: true
Attachment #801671 - Flags: review?(fabrice)
Created attachment 801708 [details] [diff] [review]
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).
Attachment #799641 - Attachment is obsolete: true
Attachment #801708 - Flags: review?(myk)
Created attachment 801709 [details] [diff] [review]
Part 2 - Browser and Webapp Runtime

Forgot to qref.
Attachment #801708 - Attachment is obsolete: true
Attachment #801708 - Flags: review?(myk)
Attachment #801709 - Flags: review?(myk)
Created attachment 801710 [details] [diff] [review]
Part 3 - Android
Attachment #799642 - Attachment is obsolete: true
Attachment #801710 - Flags: review?(wjohnston)
Created attachment 801713 [details] [diff] [review]
Part 4 - B2G

I haven't found any documentation about using |new Promise()| and using it triggers an undefined error.
Attachment #799644 - Attachment is obsolete: true
Attachment #801713 - Flags: review?(fabrice)
Created attachment 801714 [details] [diff] [review]
Part 5 - Test fixes
Attachment #799645 - Attachment is obsolete: true
Attachment #801714 - Flags: review?(fabrice)
Created attachment 801800 [details] [diff] [review]
Part 4 - B2G
Attachment #801713 - Attachment is obsolete: true
Attachment #801713 - Flags: review?(fabrice)
Attachment #801800 - Flags: review?(fabrice)
Created attachment 801802 [details] [diff] [review]
Part 3 - Android
Attachment #801710 - Attachment is obsolete: true
Attachment #801710 - Flags: review?(wjohnston)
Attachment #801802 - Flags: review?(wjohnston)
Created attachment 801804 [details] [diff] [review]
Part 4 - B2G
Attachment #801800 - Attachment is obsolete: true
Attachment #801800 - Flags: review?(fabrice)
Attachment #801804 - Flags: review?(fabrice)
Created attachment 801820 [details] [diff] [review]
Part 3 - Android
Attachment #801802 - Attachment is obsolete: true
Attachment #801802 - Flags: review?(wjohnston)
Attachment #801820 - Flags: review?(wjohnston)
Created attachment 801824 [details] [diff] [review]
Part 4 - B2G
Attachment #801804 - Attachment is obsolete: true
Attachment #801804 - Flags: review?(fabrice)
Attachment #801824 - Flags: review?(fabrice)
Created attachment 801887 [details] [diff] [review]
Part 2 - Browser and Webapp Runtime

Updated after the landing of bug 899353.

Hopefully after bug 910466 our receiveMessage will be cleaner.
Attachment #801709 - Attachment is obsolete: true
Attachment #801709 - Flags: review?(myk)
Attachment #801887 - Flags: review?(myk)
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.
Created attachment 806096 [details] [diff] [review]
Part 1 - Interfaces and registry
Attachment #801671 - Attachment is obsolete: true
Attachment #801671 - Flags: review?(fabrice)
Attachment #806096 - Flags: review?(fabrice)
Created attachment 806098 [details] [diff] [review]
Part 2 - Browser and Webapp Runtime
Attachment #801887 - Attachment is obsolete: true
Attachment #801887 - Flags: review?(myk)
Attachment #806098 - Flags: review?(myk)
Created attachment 806102 [details] [diff] [review]
Part 3 - Android
Attachment #801820 - Attachment is obsolete: true
Attachment #801820 - Flags: review?(wjohnston)
Attachment #806102 - Flags: review?(wjohnston)
Created attachment 806103 [details] [diff] [review]
Part 4 - B2G
Attachment #801824 - Attachment is obsolete: true
Attachment #801824 - Flags: review?(fabrice)
Attachment #806103 - Flags: review?(fabrice)
Created attachment 806105 [details] [diff] [review]
Part 5 - Test fixes
Attachment #801714 - Attachment is obsolete: true
Attachment #801714 - Flags: review?(fabrice)
Attachment #806105 - Flags: review?(fabrice)
(Reporter)

Comment 31

5 years ago
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.
Attachment #806096 - Flags: review?(fabrice) → feedback?(myk)
(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 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.)
Attachment #806096 - Flags: feedback?(myk) → feedback+
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!
Attachment #806098 - Flags: review?(myk)
Attachment #806105 - Flags: review?(fabrice)
Attachment #806102 - Flags: review?(wjohnston)
Attachment #806103 - Flags: review?(fabrice)
(Reporter)

Comment 35

5 years ago
(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.
(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!
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);
}
(Reporter)

Updated

4 years ago
Blocks: 1000315
Created attachment 8411910 [details] [diff] [review]
WIP
Attachment #806096 - Attachment is obsolete: true
Attachment #806098 - Attachment is obsolete: true
Attachment #806102 - Attachment is obsolete: true
Attachment #806103 - Attachment is obsolete: true
Attachment #806105 - Attachment is obsolete: true
Attachment #8411910 - Flags: feedback?(myk)
Attachment #8411910 - Flags: feedback?(fabrice)
(Reporter)

Updated

4 years ago
Attachment #8411910 - Flags: feedback?(fabrice) → feedback+
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 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.
Attachment #8411910 - Flags: feedback?(myk) → feedback+
(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?
(Reporter)

Comment 42

4 years ago
(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!
Depends on: 1005370
Depends on: 1006394
Depends on: 1007402

Updated

4 years ago
No longer blocks: 1000315

Updated

4 years ago
Blocks: 998243
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX

Updated

10 months ago
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.