Last Comment Bug 768276 - Implement new behavior for getInstalled/getSelf/getAll/getNotInstalled
: Implement new behavior for getInstalled/getSelf/getAll/getNotInstalled
Status: VERIFIED FIXED
[blocking-webrtdesktop1+], [qa!]
: dev-doc-needed
Product: Firefox Graveyard
Classification: Graveyard
Component: Web Apps (show other bugs)
: Trunk
: All All
: P1 normal
: Firefox 16
Assigned To: :Felipe Gomes (needinfo me!)
: Jason Smith [:jsmith]
:
Mentors:
: 749033 756306 756307 762698 (view as bug list)
Depends on:
Blocks: Apps-Dev-Doc-Needed 756306
  Show dependency treegraph
 
Reported: 2012-06-25 17:01 PDT by :Felipe Gomes (needinfo me!)
Modified: 2016-02-04 15:00 PST (History)
12 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Part 1 - isLaunchable (2.99 KB, patch)
2012-06-25 17:05 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Part 2 - getInstalled/getSelf/getAll (2.00 KB, patch)
2012-06-25 17:15 PDT, :Felipe Gomes (needinfo me!)
no flags Details | Diff | Splinter Review
Part 1 - isLaunchable (v2) (2.95 KB, patch)
2012-06-26 01:55 PDT, :Felipe Gomes (needinfo me!)
myk: review+
Details | Diff | Splinter Review
Part 2 - getInstalled/getSelf/getAll (v2) (2.04 KB, patch)
2012-06-26 02:02 PDT, :Felipe Gomes (needinfo me!)
jst: review+
Details | Diff | Splinter Review
Part 3 - getNonInstalled (6.45 KB, patch)
2012-06-26 02:07 PDT, :Felipe Gomes (needinfo me!)
jst: review+
Details | Diff | Splinter Review
Part 4 - allAppsLaunchable = true on b2g (706 bytes, patch)
2012-06-26 02:15 PDT, :Felipe Gomes (needinfo me!)
fabrice: review+
Details | Diff | Splinter Review
Part 5 - allAppsLaunchable = true on API tests (that do not perform native install) (1.90 KB, patch)
2012-06-28 16:49 PDT, :Felipe Gomes (needinfo me!)
cmtalbert: review+
Details | Diff | Splinter Review

Description :Felipe Gomes (needinfo me!) 2012-06-25 17:01:21 PDT
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 me!) 2012-06-25 17:05:07 PDT
Created attachment 636550 [details] [diff] [review]
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 me!) 2012-06-25 17:15:18 PDT
Created attachment 636555 [details] [diff] [review]
Part 2 - getInstalled/getSelf/getAll

Make getInstalled/getSelf/getAll use _isLaunchable
Comment 3 Jason Smith [:jsmith] 2012-06-25 18:48:13 PDT
k9o nomination - required to identify apps that are launchable within firefox vs. not launchable.
Comment 4 :Felipe Gomes (needinfo me!) 2012-06-26 01:55:03 PDT
Created attachment 636629 [details] [diff] [review]
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 me!) 2012-06-26 02:02:04 PDT
Created attachment 636633 [details] [diff] [review]
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 me!) 2012-06-26 02:04:05 PDT
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 me!) 2012-06-26 02:07:43 PDT
Created attachment 636635 [details] [diff] [review]
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 me!) 2012-06-26 02:10:37 PDT
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 me!) 2012-06-26 02:15:17 PDT
Created attachment 636636 [details] [diff] [review]
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] 2012-06-26 13:45:40 PDT
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] 2012-06-26 13:52:29 PDT
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] 2012-06-26 13:52:39 PDT
+1 for "getNotInstalled".
Comment 13 :Felipe Gomes (needinfo me!) 2012-06-26 14:33:35 PDT
(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] 2012-06-26 14:59:13 PDT
(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, jst@mozilla.com) 2012-06-27 22:42:07 PDT
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é 2012-06-27 23:12:47 PDT
(In reply to Johnny Stenback (:jst, jst@mozilla.com) 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 me!) 2012-06-28 16:49:16 PDT
Created attachment 637732 [details] [diff] [review]
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 2012-06-29 10:48:40 PDT
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 20 :Felipe Gomes (needinfo me!) 2012-06-29 15:52:47 PDT
*** Bug 756306 has been marked as a duplicate of this bug. ***
Comment 21 :Felipe Gomes (needinfo me!) 2012-06-29 15:53:29 PDT
*** Bug 762698 has been marked as a duplicate of this bug. ***
Comment 22 :Felipe Gomes (needinfo me!) 2012-06-29 15:54:17 PDT
*** Bug 756307 has been marked as a duplicate of this bug. ***
Comment 23 :Felipe Gomes (needinfo me!) 2012-06-29 16:08:39 PDT
*** Bug 749033 has been marked as a duplicate of this bug. ***
Comment 25 Jason Smith [:jsmith] 2012-07-02 12:37:42 PDT
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 2012-07-02 12:52:12 PDT
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] 2012-07-02 14:08:19 PDT
(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] 2012-07-03 09:38:21 PDT
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] 2012-07-11 18:27:00 PDT
Verified on Nightly for Windows 7, Windows XP, OS X 10.7, and Ubuntu 12.

Note You need to log in before you can comment on or make changes to this bug.