768276 - Implement new behavior for getInstalled/getSelf/getAll/getNotInstalled

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

From IRC today: 

"ok to summarize: getInstalled(), getAll(), getSelf() will all return app objects only if they are natively installed. we add a new function: getNonInstalled() that returns app objects that cannot be launched."

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 1 - isLaunchable

Part 1 - Implement Webapps._isLaunchable to check if an app is launchable on different platforms, and provides a flag that the consumer can set to consider all apps launchable (as is the case for b2g and possibly Android).

This part puts together the patches by Dan, Tim and Marco for each desktop platform from bug 762698, bug 756306 and bug 756307

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 2 - getInstalled/getSelf/getAll

Make getInstalled/getSelf/getAll use _isLaunchable

Comment 3

[Jason Smith [:jsmith]]

k9o nomination - required to identify apps that are launchable within firefox vs. not launchable.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 1 - isLaunchable (v2)

This is part 1 as described in comment 1. Although this function is going into dom/apps, most of the code here is related to the webapprt environment on each desktop platform. The review for the original patches were requested to me but since I slightly modified these functions when putting the patches together, I'm going to pass the review to Myk.

Also added here is the allAppsLaunchable flag as planned with Fabrice. We need this for the different behavior on desktop and b2g, and we can't use something like #ifdef WIDGET == GONK because this wouldn't work correctly on the desktop builds of b2g.

I'm still not sure what's the best default for the flag, true or false. I believe the most sensible value is false, although it make might it a bit tricker to write tests, so I might change it later. It doesn't matter much.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 2 - getInstalled/getSelf/getAll (v2)

Make getInstalled/getSelf/getAll use the _isLaunchable function implemented in part 1.

Fabrice, on IRC we talked that getSelf should fire an error if the app is not launchable, but after reading some of the code around I changed this. The comment for getSelf says:

  /**
   * the request will return the application currently installed, or null.
   */

It feels wrong if every consumer of mozApps.getSelf would need to check for both null *and* an exception. Null already represents the non-presence of the app, and it translates well on both cases (desktop where we care about launchable == natively installed, and b2g where not).

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 636629 [details] [diff] [review]
Part 1 - isLaunchable (v2)

(Sorry for the bug churn, I obsoleted the wrong patch when attached part 2)

Comment 7

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 3 - getNonInstalled

Straightforward implementation of getNonInstalled, following the same code as getInstalled.

I wasn't sure if this new function should be part of mozApps or somewhere else (mozApps.mgmt?), so I went with the former which is where getInstalled exists. Here's the description:

/**
 * the request will return the applications acquired from this origin but which
 * are not launchable (e.g. by not being natively installed), or null.
 */

I believe this is what was agreed upon

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Still need to do:

- Make b2g set allAppsLaunchable = true on shell.js
- write tests
- test all these patches together which I haven't done yet ;) But I believe they are correct

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 4 - allAppsLaunchable = true on b2g

Set allAppsLaunchable = true on b2g.  Using the default as false because otherwise we'd have to change it to false both on desktop and webapprt.
(I might change this if it makes it trickier to write tests but I don't think it will)

Comment 10

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

Grammar nit: "non" is being used as a prefix in this case, so "noninstalled" is a single word, and the name of the new method should be "getNoninstalled".  Alternately, use the adverb "not", which would make it "getNotInstalled".

Comment 11

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

Comment on attachment 636629 [details] [diff] [review]
Part 1 - isLaunchable (v2)

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

::: dom/apps/src/Webapps.jsm
@@ +497,5 @@
> +    desktopINI.append("owa-" + uniqueName + ".desktop");
> +
> +    return desktopINI.exists();
> +#else
> +    return true;

allAppsLaunchable and the ifdefs take care of the cases we know about, so this will only happen unexpectedly, and it feels like returning true will mask bugs.  I would at least return false in this case and would consider throwing an exception.

Comment 12

[Anant Narayanan [:anant]]

+1 for "getNotInstalled".

Comment 13

[:Felipe Gomes (needinfo for replies!)]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #11)
> allAppsLaunchable and the ifdefs take care of the cases we know about, so
> this will only happen unexpectedly, and it feels like returning true will
> mask bugs.  I would at least return false in this case and would consider
> throwing an exception.

Hmm I believe that this means that, for new platforms that we don't know how to detect if an app is installed or not, we say it is installed. It feels the best default for me. For example, without this Android will return no apps installed until they add code to the platform detection section, or flip the allAppsLaunchable flag.

(In reply to Anant Narayanan [:anant] from comment #12)
> +1 for "getNotInstalled".

Will change it to getNotInstalled

Comment 14

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

(In reply to Felipe Gomes (:felipe) from comment #13)
> Hmm I believe that this means that, for new platforms that we don't know how
> to detect if an app is installed or not, we say it is installed. It feels
> the best default for me. For example, without this Android will return no
> apps installed until they add code to the platform detection section, or
> flip the allAppsLaunchable flag.

In that case why not explicitly flip the flag for Android, just as we're doing for B2G, instead of falling back to default behavior on that platform?  Currently we intend to support five platforms, and this code explicitly handles four while handling the fifth implicitly via a default fallback, which feels error prone.

Someone we might change the default behavior in the future without realizing it affects Android, since the code doesn't say anything about the default behavior handling Android.

At the very least, this code should sport a comment explaining that the default behavior is not just a fallback for situations we don't anticipate but is actually intended to handle platforms we support, like Android.


(In reply to Felipe Gomes (:felipe) from comment #4)
> Also added here is the allAppsLaunchable flag as planned with Fabrice. We
> need this for the different behavior on desktop and b2g, and we can't use
> something like #ifdef WIDGET == GONK because this wouldn't work correctly on
> the desktop builds of b2g.

You may be able to define desktop-specific code with #ifdef MOZ_PHOENIX.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 636635 [details] [diff] [review]
Part 3 - getNonInstalled

   // nsIDOMGlobalPropertyInitializer implementation
   init: function(aWindow) {

Please file a followup bug to have this split out from the rest of the methods that are exposed to the calling JS.

- In interfaces/apps/nsIDOMApplicationRegistry.idl:

+  /**
+   * the request will return the applications acquired from this origin but which
+   * are not launchable (e.g. by not being natively installed), or null.
+   */
+  nsIDOMDOMRequest getNonInstalled();
+

Bump the IID here, this is a binary incompatible change.

r=jst

Comment 16

[[:fabrice] Fabrice Desré]

(In reply to Johnny Stenback (:jst, [e-mail]) from comment #15)
> Comment on attachment 636635 [details] [diff] [review]
> Part 3 - getNonInstalled
> 
>    // nsIDOMGlobalPropertyInitializer implementation
>    init: function(aWindow) {
> 
> Please file a followup bug to have this split out from the rest of the
> methods that are exposed to the calling JS.

I filed bug 767662 for that.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Attachment: Part 5 - allAppsLaunchable = true on API tests (that do not perform native install)

We also need to set allAppsLaunchable = true on the API tests as they do not perform native installation and thus would never see the install attempts succeed if apps are skipped for not being natively installed.

Comment 18

[cmtalbert]

Comment on attachment 637732 [details] [diff] [review]
Part 5 - allAppsLaunchable = true on API tests (that do not perform native install)

Myk asked me to review this on IRC, so taking the review.

This looks good to me.
r+

Comment 19

[:Felipe Gomes (needinfo for replies!)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/94ea4e12bb78
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c9c6b8e048d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7901569b6ec
https://hg.mozilla.org/integration/mozilla-inbound/rev/df79962b3826
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3f4b88c6411

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/94ea4e12bb78
https://hg.mozilla.org/mozilla-central/rev/3c9c6b8e048d
https://hg.mozilla.org/mozilla-central/rev/e7901569b6ec
https://hg.mozilla.org/mozilla-central/rev/df79962b3826
https://hg.mozilla.org/mozilla-central/rev/d3f4b88c6411

Comment 25

[Jason Smith [:jsmith]]

Definitely some issues are evident as a result of this. Notably:

- Marketplace's use of getInstalled() shows that uninstalling the app still shows installed, which means we aren't doing something right on the webrt side
- "Strange behavior" (investigating now) when installing the app cache app on fabrice's test site - http://people.mozilla.com/~fdesre/openwebapps/test.html, it lists 2 apps as installed from getInstalled, not one
- Anant and Nick indicated that nothing is being shown on myapps on OS X even with apps installed

Investigating and will log bugs shortly.

Comment 26

[Ed Lee :Mardak]

Double check if the apps viewable from the OS are for the Firefox profile being checked. I know on my machine, I have at least 3 profiles that have installed apps and probably more profiles that I've already deleted..

Comment 27

[Jason Smith [:jsmith]]

(In reply to Edward Lee :Mardak from comment #26)
> Double check if the apps viewable from the OS are for the Firefox profile
> being checked. I know on my machine, I have at least 3 profiles that have
> installed apps and probably more profiles that I've already deleted..

Right I'll keep that in mind. For the testing I'm doing now, I'm trying to use clean and dirty profiles.

Comment 28

[Anant Narayanan [:anant]]

The main documentation update will be to add a description for the navigator.mozApps.mgmt.getNotInstalled() function - it will return all apps that have been "acquired" by the user but are not "natively" installed on the current device and thus cannot be launched via the AppObject.launch method.

The getAll and getInstalled functions will now only return apps that can be launched, which on desktop and android currently mean only apps that are "natively" installed.

Comment 29

[Jason Smith [:jsmith]]

Verified on Nightly for Windows 7, Windows XP, OS X 10.7, and Ubuntu 12.