Add a way to enable and disable an app.

RESOLVED FIXED in mozilla36

Status

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: fabrice, Assigned: fabrice)

Tracking

({dev-doc-complete})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Assignee

Description

5 years ago
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.
Assignee

Comment 1

5 years ago
Posted patch apps-enable.patch (obsolete) — Splinter Review
Assignee

Updated

5 years ago
Flags: needinfo?(jonas)
Assignee

Updated

5 years ago
Summary: Add a way enable and disable an app. → Add a way to enable and disable an app.
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.
Flags: needinfo?(jonas)
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.
Assignee

Comment 4

5 years ago
(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.
Assignee

Comment 5

5 years ago
Posted patch apps-enable.patch (obsolete) — Splinter Review
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.
Attachment #8494235 - Attachment is obsolete: true
Attachment #8509213 - Flags: review?(myk)
Attachment #8509213 - Flags: review?(mar.castelluccio)
Attachment #8509213 - Flags: review?(jonas)
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?
Attachment #8509213 - Flags: review?(myk) → review+
Assignee

Comment 7

5 years ago
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!
Attachment #8509213 - Attachment is obsolete: true
Attachment #8509213 - Flags: review?(mar.castelluccio)
Attachment #8509213 - Flags: review?(jonas)
Attachment #8510620 - Flags: review?(jonas)
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?
Attachment #8510620 - Flags: review?(jonas) → review+
Assignee

Comment 9

5 years ago
(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!
Assignee

Updated

5 years ago
Flags: needinfo?(fabrice)
https://hg.mozilla.org/mozilla-central/rev/feafedc5b6ef
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Blocks: 1122979
Blocks: 1122981

Updated

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