Expose android native apps trough the navigator.mozApps api

RESOLVED FIXED

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

So that we can launch legacy apps from gaia's homescreen.
Posted patch mozapps-android.patch (obsolete) — Splinter Review
Works fine for me, but I need to check a couple of details before asking for reviews.
Posted patch mozapps-android.patch (obsolete) — Splinter Review
snorp, can you take a look at the java parts? And jerjm the dom/apps parts? thanks!
Attachment #8655147 - Attachment is obsolete: true
Attachment #8656848 - Flags: review?(snorp)
Attachment #8656848 - Flags: review?(ferjmoreno)
Attachment #8656848 - Flags: review?(snorp) → review+
Comment on attachment 8656848 [details] [diff] [review]
mozapps-android.patch

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

Looks good! But I'd like to see the last version. Thanks!

::: dom/apps/AndroidUtils.jsm
@@ +15,5 @@
> +
> +let appsRegistry = null;
> +
> +function debug() {
> +  dump("-*- AndroidUtils " + Array.slice(arguments) + "\n");

Remember to comment this before landing, please.

@@ +58,5 @@
> +    let [origin, manifestURL] =
> +      this.getOriginAndManifestURL(aApp.packagename);
> +    let manifest = {
> +      name: aApp.name,
> +      icons: { "96": aApp.icon }

Shouldn't we add more icon sizes if available?

@@ +98,5 @@
> +
> +          // Wait for all apps to be installed.
> +          Promise.all(promises).then(() => {
> +            appsRegistry._saveApps();
> +            aResolve(); });

No need to wrap all this within a Promise. You can do:

return Messaging.sendRequestForResult(...).then(aApps => {
...
return Promise.all(promises).then(() => { return appsRegistry._saveApps(); });
});

Unless you prefer not to wait for the apps to be saved. Which I would prefer to avoid.

@@ +105,5 @@
> +    });
> +  },
> +
> +  observe: function(aSubject, aTopic, aData) {
> +    let data = JSON.parse(aData);

This can throw.

::: dom/apps/Webapps.jsm
@@ +95,5 @@
> +                                  "resource://gre/modules/Messaging.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "AndroidUtils",
> +                                  "resource://gre/modules/AndroidUtils.jsm");
> +

Do we need this on non Android builds?

@@ +533,5 @@
>      });
>    },
>  
>    // Installs a 3rd party app.
>    installPreinstalledApp: function installPreinstalledApp(aId) {

I would have preferred to leave and extend the preprocessor condition here instead of adding a condition before calling 'installPreinstalledApp'. Maybe move the 'if (AppConstants.MOZ_B2GDROID || AppConstants.platform == "gonk")' here?

@@ +4113,5 @@
> +                                packagename: packageName,
> +                                classname: className });
> +        // We have to wait for Android's uninstall before sending the
> +        // uninstall event, so fake an error here.
> +        response = "Webapps:Uninstall:Return:KO";

Instead of faking an error, I think we can wait here.

You can add something like this on AndroidUtils:

function deferUninstall() {
  this.uninstallDeferred = Promise.defer();
  return uninstallDeferred.promise;
}

On the 'AndroidUtils.observe' handler, when we get the 'Android:Apps:Uninstalled' notification you'll need to do 'this.uninstallDeferred.resolve()'

And here you just need to add:

yield AndroidUtils.deferUninstall();

::: dom/apps/moz.build
@@ +29,5 @@
>      'Webapps.manifest',
>  ]
>  
>  EXTRA_JS_MODULES += [
> +    'AndroidUtils.jsm',

Do we need to have this on non Android builds?
Attachment #8656848 - Flags: review?(ferjmoreno) → feedback+
Posted patch mozapps-android.patch v2 (obsolete) — Splinter Review
Addressing comments from ferjm
Attachment #8656848 - Attachment is obsolete: true
Attachment #8660101 - Flags: review?(ferjmoreno)
(In reply to Fernando Jiménez Moreno [:ferjm] from comment #3)

> @@ +58,5 @@
> > +    let [origin, manifestURL] =
> > +      this.getOriginAndManifestURL(aApp.packagename);
> > +    let manifest = {
> > +      name: aApp.name,
> > +      icons: { "96": aApp.icon }
> 
> Shouldn't we add more icon sizes if available?

Yes. I want to do that in a followup to move away from base64 to android:// uris.

> No need to wrap all this within a Promise. You can do:
> 
> return Messaging.sendRequestForResult(...).then(aApps => {
> ...
> return Promise.all(promises).then(() => { return appsRegistry._saveApps();
> });
> });

Good idea, done!


> > +  observe: function(aSubject, aTopic, aData) {
> > +    let data = JSON.parse(aData);
> 
> This can throw.

done.

> > +XPCOMUtils.defineLazyModuleGetter(this, "AndroidUtils",
> > +                                  "resource://gre/modules/AndroidUtils.jsm");
> > +
> 
> Do we need this on non Android builds?

I refactored that to only Cu.Import() when AppConstants.MOZ_B2GDROID is true.


> >    // Installs a 3rd party app.
> >    installPreinstalledApp: function installPreinstalledApp(aId) {
> 
> I would have preferred to leave and extend the preprocessor condition here
> instead of adding a condition before calling 'installPreinstalledApp'. Maybe
> move the 'if (AppConstants.MOZ_B2GDROID || AppConstants.platform == "gonk")'
> here?

done.

> @@ +4113,5 @@
> > +                                packagename: packageName,
> > +                                classname: className });
> > +        // We have to wait for Android's uninstall before sending the
> > +        // uninstall event, so fake an error here.
> > +        response = "Webapps:Uninstall:Return:KO";
> 
> Instead of faking an error, I think we can wait here.
> 
> You can add something like this on AndroidUtils:
> 
> function deferUninstall() {
>   this.uninstallDeferred = Promise.defer();
>   return uninstallDeferred.promise;
> }
> 
> On the 'AndroidUtils.observe' handler, when we get the
> 'Android:Apps:Uninstalled' notification you'll need to do
> 'this.uninstallDeferred.resolve()'
> 
> And here you just need to add:
> 
> yield AndroidUtils.deferUninstall();

I had done something like that initially, but one thing I could not workaround is that when we trigger the android native uninstall, we have no way to know if the user canceled uninstallation in the prompt. So we would end up with dangling promises (since we need to track uninstallations per package, not a single global one). The current code doesn't seem to cause issues with gaia's homescreen(s) so I'd rather keep that.

> ::: dom/apps/moz.build
> @@ +29,5 @@
> >      'Webapps.manifest',
> >  ]
> >  
> >  EXTRA_JS_MODULES += [
> > +    'AndroidUtils.jsm',
> 
> Do we need to have this on non Android builds?

No we don't!
Forgot to qref...
Attachment #8660101 - Attachment is obsolete: true
Attachment #8660101 - Flags: review?(ferjmoreno)
Attachment #8660109 - Flags: review?(ferjmoreno)
Comment on attachment 8660101 [details] [diff] [review]
mozapps-android.patch v2

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

::: dom/apps/AndroidUtils.jsm
@@ +15,5 @@
> +
> +let appsRegistry = null;
> +
> +function debug() {
> +  dump("-*- AndroidUtils " + Array.slice(arguments) + "\n");

Comment this before landing, please.

@@ +58,5 @@
> +    let [origin, manifestURL] =
> +      this.getOriginAndManifestURL(aApp.packagename);
> +    let manifest = {
> +      name: aApp.name,
> +      icons: { "96": aApp.icon }

Could you file a follow up for the multiple icons issue and add a comment here, please?

@@ +96,5 @@
> +        });
> +
> +        // Wait for all apps to be installed.
> +        return Promise.all(promises)
> +                      .then(() => { return appsRegistry._saveApps(); });

nit: you can take this .then out of this block

return Messaging.sendRequestForResult(...).then(aApps => {
   ...
   return Promises.all(promises);
}).then(appsRegistry._saveApps);

Sorry I didn't put it this way previously.
Attachment #8660101 - Flags: review+
Attachment #8660109 - Flags: review?(ferjmoreno) → review+
Depends on: 1204557
Flags: needinfo?(fabrice)
https://hg.mozilla.org/mozilla-central/rev/de5d084f84b4
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.