Closed
Bug 768276
Opened 12 years ago
Closed 12 years ago
Implement new behavior for getInstalled/getSelf/getAll/getNotInstalled
Categories
(Firefox Graveyard :: Web Apps, defect, P1)
Firefox Graveyard
Web Apps
Tracking
(blocking-kilimanjaro:+)
People
(Reporter: Felipe, Assigned: Felipe)
References
(Blocks 1 open bug)
Details
(Keywords: dev-doc-needed, Whiteboard: [blocking-webrtdesktop1+], [qa!])
Attachments
(5 files, 2 obsolete files)
(deleted),
patch
|
myk
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
jst
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
fabrice
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
cmtalbert
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
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•12 years ago
|
||
Make getInstalled/getSelf/getAll use _isLaunchable
Comment 3•12 years ago
|
||
k9o nomination - required to identify apps that are launchable within firefox vs. not launchable.
blocking-kilimanjaro: --- → ?
Assignee | ||
Comment 4•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Summary: Implement new behavior for getInstalled/getSelf/getAll/getUninstalled → Implement new behavior for getInstalled/getSelf/getAll/getNonInstalled
Assignee | ||
Updated•12 years ago
|
Attachment #636555 -
Attachment is obsolete: true
Assignee | ||
Comment 6•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
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)
Comment 10•12 years ago
|
||
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•12 years ago
|
||
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+
Comment 12•12 years ago
|
||
+1 for "getNotInstalled".
Assignee | ||
Comment 13•12 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
Comment 14•12 years ago
|
||
(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•12 years ago
|
Priority: -- → P1
Whiteboard: [blocking-webrtdesktop1+]
Target Milestone: --- → Firefox 16
Updated•12 years ago
|
blocking-kilimanjaro: ? → +
Updated•12 years ago
|
Attachment #636633 -
Flags: review?(fabrice) → review+
Comment 15•12 years ago
|
||
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+
Comment 16•12 years ago
|
||
(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.
Assignee | ||
Comment 17•12 years ago
|
||
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)
Updated•12 years ago
|
Attachment #636636 -
Flags: review?(fabrice) → review+
Comment 18•12 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•12 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•12 years ago
|
Whiteboard: [blocking-webrtdesktop1+] → [blocking-webrtdesktop1+], [qa+]
Comment 24•12 years ago
|
||
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
Closed: 12 years ago
Resolution: --- → FIXED
Comment 25•12 years ago
|
||
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•12 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..
Comment 27•12 years ago
|
||
(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.
Updated•12 years ago
|
Keywords: dev-doc-needed
Comment 28•12 years ago
|
||
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•12 years ago
|
QA Contact: jsmith
Updated•12 years ago
|
Summary: Implement new behavior for getInstalled/getSelf/getAll/getNonInstalled → Implement new behavior for getInstalled/getSelf/getAll/getNotInstalled
Comment 29•12 years ago
|
||
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•12 years ago
|
Blocks: Apps-Dev-Doc-Needed
Updated•9 years ago
|
Product: Firefox → Firefox Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•