The default bug view has changed. See this FAQ.

Implement new behavior for getInstalled/getSelf/getAll/getNotInstalled

VERIFIED FIXED in Firefox 16

Status

Firefox Graveyard
Web Apps
P1
normal
VERIFIED FIXED
5 years ago
a year ago

People

(Reporter: Felipe, Assigned: Felipe)

Tracking

(Blocks: 1 bug, {dev-doc-needed})

Trunk
Firefox 16
dev-doc-needed
Dependency tree / graph

Details

(Whiteboard: [blocking-webrtdesktop1+], [qa!])

Attachments

(5 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Comment 2

5 years ago
Created attachment 636555 [details] [diff] [review]
Part 2 - getInstalled/getSelf/getAll

Make getInstalled/getSelf/getAll use _isLaunchable
k9o nomination - required to identify apps that are launchable within firefox vs. not launchable.
blocking-kilimanjaro: --- → ?
(Assignee)

Comment 4

5 years ago
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.
Attachment #636550 - Attachment is obsolete: true
Attachment #636629 - Flags: review?(myk)
(Assignee)

Comment 5

5 years ago
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).
Attachment #636629 - Attachment is obsolete: true
Attachment #636629 - Flags: review?(myk)
Attachment #636633 - Flags: review?(fabrice)
(Assignee)

Updated

5 years ago
Summary: Implement new behavior for getInstalled/getSelf/getAll/getUninstalled → Implement new behavior for getInstalled/getSelf/getAll/getNonInstalled
(Assignee)

Updated

5 years ago
Attachment #636555 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
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)
Attachment #636629 - Attachment is obsolete: false
Attachment #636629 - Flags: review?(myk)
(Assignee)

Comment 7

5 years ago
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
Attachment #636635 - Flags: review?(fabrice)
(Assignee)

Comment 8

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

Comment 9

5 years ago
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)
Attachment #636636 - Flags: review?(fabrice)
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 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.
Attachment #636629 - Flags: review?(myk) → review+
+1 for "getNotInstalled".
(Assignee)

Comment 13

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

Updated

5 years ago
Priority: -- → P1
Whiteboard: [blocking-webrtdesktop1+]
Target Milestone: --- → Firefox 16

Updated

5 years ago
blocking-kilimanjaro: ? → +

Updated

5 years ago
Attachment #636633 - Flags: review?(fabrice) → review+
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
Attachment #636635 - Flags: review?(fabrice) → review+
(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.
Blocks: 756306
(Assignee)

Comment 17

5 years ago
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.
Attachment #637732 - Flags: review?(dclarke)
Attachment #636636 - Flags: review?(fabrice) → review+

Comment 18

5 years ago
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+
Attachment #637732 - Flags: review?(dclarke) → review+
(Assignee)

Comment 19

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

Updated

5 years ago
Whiteboard: [blocking-webrtdesktop1+] → [blocking-webrtdesktop1+], [qa+]
(Assignee)

Updated

5 years ago
Duplicate of this bug: 756306
(Assignee)

Updated

5 years ago
Duplicate of this bug: 762698
(Assignee)

Updated

5 years ago
Duplicate of this bug: 756307
(Assignee)

Updated

5 years ago
Duplicate of this bug: 749033
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
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
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

5 years ago
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..
(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.
Keywords: dev-doc-needed
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.

Updated

5 years ago
QA Contact: jsmith

Updated

5 years ago
Summary: Implement new behavior for getInstalled/getSelf/getAll/getNonInstalled → Implement new behavior for getInstalled/getSelf/getAll/getNotInstalled
Verified on Nightly for Windows 7, Windows XP, OS X 10.7, and Ubuntu 12.
Status: RESOLVED → VERIFIED
Whiteboard: [blocking-webrtdesktop1+], [qa+] → [blocking-webrtdesktop1+], [qa!]

Updated

5 years ago
Blocks: 783741
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.