1072090 - Add a way to enable and disable an app.

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[[:fabrice] Fabrice Desré]

We have use cases where we will need to prevent an app from being launched, while not uninstall it.

Jonas, I added the following to the api:
app.enable = true|false
app.onstatechange = function() {...}

So setting/reading the state is available synchronously, and an event is available to get notified if when the app state is changed from a different instance object referring to the same app. For instance, disabling the app from the settings app could trigger a change in the homescreen app.

Comment 1

[[:fabrice] Fabrice Desré]

Attachment: apps-enable.patch

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ugh, I wrote a comment for this somewhere but appear to have lost it.

The purpose of this is to temporarily suspend an app while we grab a snapshot of it's data, right?

If so, would it make sense to make sure that you can still call app.launch(), but that the app doesn't actually launch until app.enabled is set to true again? That way we don't have to check all possible code paths that are launching apps to make sure they retry as needed.

In particular I'm worried that system messages (and soon service worker messages) will make us randomly behave "glitchy" if we do things like background backups using this.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, should we restrict this to only apps that have webapps-manage permission? In particular it seems bad if an app accidentally set this to false for another app. Or even worse, sets it to true while we're in the middle of doing a backup.

Comment 4

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #2)
> Ugh, I wrote a comment for this somewhere but appear to have lost it.
> 
> The purpose of this is to temporarily suspend an app while we grab a
> snapshot of it's data, right?

That's not the use I had in mind. For "apps as add-ons" we also need a way to disable an add-on without uninstalling the app. But the backup/restore use case is valid too indeed.

> If so, would it make sense to make sure that you can still call
> app.launch(), but that the app doesn't actually launch until app.enabled is
> set to true again? That way we don't have to check all possible code paths
> that are launching apps to make sure they retry as needed.

I don't know. launch() returns a DOMRequest that fails in this case, so we let the caller manage the failure.

> In particular I'm worried that system messages (and soon service worker
> messages) will make us randomly behave "glitchy" if we do things like
> background backups using this.

Right, I'm not sure what to do in these cases. Some system messages have an implicit TTL (eg. nfc discovery) so it would not make sense to buffer them and launch them afterward.

Comment 5

[[:fabrice] Fabrice Desré]

Attachment: apps-enable.patch

We discussed the api with Jonas, and decided to put it on mozApps.mgmt, along with the onstatechange event.
We also don't want to prevent disabled apps from launching.

Comment 6

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

Comment on attachment 8509213 [details] [diff] [review]
apps-enable.patch

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

Looks reasonable, I only have nits, so r=myk.

But some of the event/function names in this patch are prone to ambiguity and misinterpretation.

"EnableApp" sounds like it only enables the app, whereas it actually sets the "enabled" state (to either true or false).  And there are other kinds of state, like the "install state", so referring to the enabled state as simply the "state" of the app is confusing.

I'd make these changes to the various occurrences of those names:

Webapps:EnableApp -> Webapps:SetEnabled 
Webapps:EnableApp:Return -> Webapps:SetEnabled:Return

statechange -> enabledchange or setenabled 
Webapps:UpdateState -> Webapps:UpdateEnabledState or Webapps:UpdateEnabled

::: dom/apps/Webapps.jsm
@@ +4317,5 @@
>        aMm.sendAsyncMessage("Webapps:ReplaceReceipt:Return:OK", aData);
>      });
>    },
>  
> +  enableApp: function(aData, aMM) {

Nit: aMM -> aMm (for consistency with the other occurrences in this file)

::: dom/apps/tests/test_app_enabled.html
@@ +48,5 @@
> +
> +SimpleTest.waitForExplicitFinish();
> +
> +/**
> +  * Install 2 apps from the same origin and uninstall them.

Nit: this comment doesn't make sense in this context; looks like it comes from test_install_multiple_apps_origin.html, upon which this test is perhaps based; test_import_export.html has the same problem.

@@ +91,5 @@
> +  // Re-enable the app.
> +  navigator.mozApps.mgmt.onstatechange = function(event) {
> +    ok(true, "onstatechange received");
> +    is(event.application.enabled, true, "Application is enabled");
> +    continueTest();

Shouldn't we check *app.enabled* here too?

Comment 7

[[:fabrice] Fabrice Desré]

Attachment: apps-enable.patch v2

New patch with Myk's comments addressed, carrying over his r+. I named the event `onenabledstatechange` which is quite long.

Still need Jonas to sign off the webidl changes!

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8510620 [details] [diff] [review]
apps-enable.patch v2

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

::: dom/apps/Webapps.js
@@ +417,5 @@
> +  },
> +
> +  get onstatechange() {
> +    return this.__DOM_IMPL__.getEventHandler("onstatechange");
> +  },

This doesn't appear in the .webidl for the App object. So is this only used internally? Or is something else going on here?

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Jonas Sicking (:sicking) from comment #8)
> Comment on attachment 8510620 [details] [diff] [review]
> apps-enable.patch v2
> 
> Review of attachment 8510620 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/apps/Webapps.js
> @@ +417,5 @@
> > +  },
> > +
> > +  get onstatechange() {
> > +    return this.__DOM_IMPL__.getEventHandler("onstatechange");
> > +  },
> 
> This doesn't appear in the .webidl for the App object. So is this only used
> internally? Or is something else going on here?

Nope, that's a leftover I forgot to remove. good catch!

Comment 10

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/556dc4f97e08

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for m-oth orange: https://hg.mozilla.org/integration/b2g-inbound/rev/3eb727d509f4

https://treeherder.mozilla.org/ui/logviewer.html#?job_id=691590&repo=b2g-inbound

Comment 12

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/b2g-inbound/rev/feafedc5b6ef

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/feafedc5b6ef

Comment 14

[Chris Mills [:cmills]]

I've documented the new method and event handler:

https://developer.mozilla.org/en-US/docs/Web/API/DOMApplicationsManager/onenabledstatechange
https://developer.mozilla.org/en-US/docs/Web/API/DOMApplicationsManager/setEnabled

I've made sure it is covered in the 2.2 release notes:

https://developer.mozilla.org/en-US/Firefox_OS/Releases/2.2#Web_API_changes

And included the state property on the DOM Applications page:

https://developer.mozilla.org/en-US/docs/Web/API/DOMApplication

A tech review would be great. Thanks!