Closed Bug 756306 Opened 12 years ago Closed 12 years ago

`mozApps.getInstalled()` should return only the apps that are natively installed - Windows

Categories

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

Product:
Firefox Graveyard
Component:
Web Apps
Platform:
x86
Windows 7
Type:
defect

Tracking

(blocking-kilimanjaro:+)

Status:
RESOLVED DUPLICATE of bug 768276
Milestone:
Firefox 16
Project Flags:
blocking-kilimanjaro +

People

(Reporter: TimAbraldes, Assigned: TimAbraldes)

References

Details

(Whiteboard: [blocking-webrtdesktop1+])

Attachments

(1 file, 3 obsolete files)

Patch v4 - Update to work with patch in bug 758044; don't append "-1" for default port
1.68 KB, patch
fabrice
: review-
Details | Diff | Splinter Review 
See discussion in bug 749033.

Marketplaces need to be able to distinguish between 3 states:
  App has not been purchased/acquired by this user
  App has been purchased/acquired by this user but it is not installed on this machine
  App is natively installed on the device the user is currently using

Marketplaces are aware of whether an app has been purchased/acquired by a particular user because they can look at the user's purchase history.  The only missing piece is the ability to tell whether an app is currently installed on the user's machine.

The approach being considered on Windows is to check for the app's uninstall registry key; if that exists we can be reasonably sure that the app is still installed (and at least the Marketplace will show consistency with Windows' uninstall widget)
blocking-kilimanjaro: --- → ?
blocking-kilimanjaro: ? → +
Attached patch Patch v1 - Initial WIP patch (obsolete) — — Splinter Review 
This patch has the intended effect of making `navigator.mozApps.getInstalled()` return only the apps that are natively installed on a Windows machine.

This is a WIP, so there is obvious cleanup to be done:
  1) Use a #ifdef instead of a runtime check for Windows-only codepath
  2) Reuse the same `nsIWindowsRegKey` instead of creating it on every loop iteration
  ...and probably more

One issue came up during development that may cause bigger headaches: It looks like the `App` objects [https://developer.mozilla.org/en/Apps/Apps_JavaScript_API/app_object] do not have complete origins for apps; they are missing port numbers.  Other code relies on port numbers being present, including the code that writes the Windows uninstall registry keys, so we'll have to make sure we keep port info along with origins wherever we store them.
Assignee: nobody → tabraldes
Status: NEW → ASSIGNED
Spoke with Fabrice who let me know that origins will have port information whenever the port is not the default for the scheme.

Cleaned up the patch and pushed to try:
  https://tbpl.mozilla.org/?tree=Try&rev=7965cb27ac9f

Fabrice, I believe you wrote most of webapps.jsm, are you willing to do this review?
Attachment #626302 - Attachment is obsolete: true
Attachment #626339 - Flags: review?(fabrice)
Comment on attachment 626339 [details] [diff] [review]
Patch v2 - Deal with port-less origins. Reuse single nsIWindowsRegKey. Switch to #ifdefs.

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

Not that the code is bad, but I'd like to understand it better

::: dom/base/Webapps.jsm
@@ +301,5 @@
> +        // We're on Windows, so check for native installation
> +        try {
> +          let origin = this.webapps[id].origin;
> +          if (!/:-?\d{1,5}$/.test(origin)) {
> +            origin += ":-1";

-1 is not a valid port number. Can you explain me why you add that?

@@ +311,5 @@
> +                            uninstallKey.ACCESS_READ);
> +        } catch (e) {
> +          continue;
> +        } finally {
> +          if(uninstallKey) {

uninstallKey is never null in the #ifdef XP_WIN block.
Attachment #626339 - Flags: review?(fabrice) → review-
(In reply to Fabrice Desré [:fabrice] from comment #3)
> Comment on attachment 626339 [details] [diff] [review]
> Patch v2 - Deal with port-less origins. Reuse single nsIWindowsRegKey.
> Switch to #ifdefs.
> 
> Review of attachment 626339 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not that the code is bad, but I'd like to understand it better
> 
> ::: dom/base/Webapps.jsm
> @@ +301,5 @@
> > +        // We're on Windows, so check for native installation
> > +        try {
> > +          let origin = this.webapps[id].origin;
> > +          if (!/:-?\d{1,5}$/.test(origin)) {
> > +            origin += ":-1";
> 
> -1 is not a valid port number. Can you explain me why you add that?

The uninstall registry key that we write is of the form scheme://host:port and if the port is the default value for the scheme we use -1 instead.  This is done simply because that is what `nsIURI` does [https://developer.mozilla.org/en/NsIURI].

> @@ +311,5 @@
> > +                            uninstallKey.ACCESS_READ);
> > +        } catch (e) {
> > +          continue;
> > +        } finally {
> > +          if(uninstallKey) {
> 
> uninstallKey is never null in the #ifdef XP_WIN block.

Good catch. Fixed this in the new patch.


This try run reveals that this patch breaks the webapps mochitests:
  https://tbpl.mozilla.org/?tree=Try&rev=7965cb27ac9f

I'll update the tests and post an update patch soon.
Attachment #626339 - Attachment is obsolete: true
(In reply to Tim Abraldes from comment #4)

> The uninstall registry key that we write is of the form scheme://host:port
> and if the port is the default value for the scheme we use -1 instead.  This
> is done simply because that is what `nsIURI` does
> [https://developer.mozilla.org/en/NsIURI].

nsIURI is an internal interface. The correct thing to do is to use the default port number for the scheme used by the origin. You can get it using the protocol handler (see https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsIProtocolHandler.idl#27)
(In reply to Fabrice Desré [:fabrice] from comment #5)
> (In reply to Tim Abraldes from comment #4)
> 
> > The uninstall registry key that we write is of the form scheme://host:port
> > and if the port is the default value for the scheme we use -1 instead.  This
> > is done simply because that is what `nsIURI` does
> > [https://developer.mozilla.org/en/NsIURI].
> 
> nsIURI is an internal interface. The correct thing to do is to use the
> default port number for the scheme used by the origin. You can get it using
> the protocol handler (see
> https://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/
> nsIProtocolHandler.idl#27)

It probably makes more sense to omit the port number if it is the default.  Filed bug 758044 for this.
Another WIP patch.  This is what this patch might look like when bug 758044 lands.
Attachment #626499 - Attachment is obsolete: true
Opinion on Priority and Milestone:

Priority: P1, blocker
Milestone: Firefox 15

Reasoning:

This part of the blocker bug attached - which is already a blocker for the release. Windows is a primary operating system to be supported. Therefore, this blocks.
Priority: -- → P1
Whiteboard: [blocking-webrtdesktop1+]
Target Milestone: --- → Firefox 15
Comment on attachment 626667 [details] [diff] [review]
Patch v4 - Update to work with patch in bug 758044; don't append "-1" for default port

I'm running this through try along with the patch in bug 758044:
  https://tbpl.mozilla.org/?tree=Try&rev=d250c8fe81b9

Fabrice; assuming the try run goes well, what do you think of this patch?
Attachment #626667 - Flags: review?(fabrice)
Comment on attachment 626667 [details] [diff] [review]
Patch v4 - Update to work with patch in bug 758044; don't append "-1" for default port

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

That does not make sense to do that check just for getInstalled() but not for getSelf() and getAll(). With the current patch, a dashboard using getAll() will show results that are hidden by getInstalled().

I'm seeing additional related issues:
- What is the expected behavior for uninstall() ? We should probably fail to uninstall an app that is not natively installed.
- Why don't we try to keep the native apps installed list in sync with the UA registry?
- Why is this windows only? Don't we want the same behavior on Mac (and linux in the future)?
Attachment #626667 - Flags: review?(fabrice) → review-
There are 3 distinct states that apps can be in:
  1) Unacquired
  2) Acquired but not installed
  3) Installed

On platforms that support native installation, states 2 and 3 are distinct.  On platforms that do not support native installation, states 2 and 3 are the same.


(In reply to Fabrice Desré [:fabrice] from comment #10)
> Comment on attachment 626667 [details] [diff] [review]
> Patch v4 - Update to work with patch in bug 758044; don't append "-1" for
> default port
> 
> Review of attachment 626667 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> That does not make sense to do that check just for getInstalled() but not
> for getSelf() and getAll(). With the current patch, a dashboard using
> getAll() will show results that are hidden by getInstalled().

I think it makes sense to have `getInstalled()` return only natively installed (state 3 above) apps while `getAll()` returns all "acquired" (states 2 and 3) apps.  The hypothetical dashboard you mention would want to cross-reference the `getInstalled()` results with the `getAll()` results so it could show different buttons for each app (e.g. "buy this app," "purchased," and "installed").  A similar argument applies for `getSelf()`.

> I'm seeing additional related issues:
> - What is the expected behavior for uninstall() ? We should probably fail to
> uninstall an app that is not natively installed.

CCing Anant to be sure, but I believe that `uninstall()` has additional implications for AitC: Uninstalling an app using the mozApps API will cause the app to be "unacquired" and this will propagate to your other devices.

> - Why don't we try to keep the native apps installed list in sync with the
> UA registry?

The UA registry keeps track of whether apps are "acquired" and `getInstalled` is now keeping track of whether apps are "installed (natively)."  Thus, the two shouldn't be kept in sync.

> - Why is this windows only? Don't we want the same behavior on Mac (and
> linux in the future)?

This is the Window-only bug.  See bug 756307 for Linux and bug 756308 for Mac.


One possible alternative to the implementation in this patch is to add a "state" flag to the App object.  The flag would have values "unacquired," "acquired," and "installed."  With that implementation, sites and stores would not have to cross-reference `getInstalled()` against `getSelf()` or `getAll()`; they could just check the flag.
(In reply to Tim Abraldes from comment #11)
> There are 3 distinct states that apps can be in:
>   1) Unacquired
>   2) Acquired but not installed
>   3) Installed
> 
> On platforms that support native installation, states 2 and 3 are distinct. 
> On platforms that do not support native installation, states 2 and 3 are the
> same.
> 
> 
> (In reply to Fabrice Desré [:fabrice] from comment #10)
> > Comment on attachment 626667 [details] [diff] [review]
> > Patch v4 - Update to work with patch in bug 758044; don't append "-1" for
> > default port
> > 
> > Review of attachment 626667 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > That does not make sense to do that check just for getInstalled() but not
> > for getSelf() and getAll(). With the current patch, a dashboard using
> > getAll() will show results that are hidden by getInstalled().
> 
> I think it makes sense to have `getInstalled()` return only natively
> installed (state 3 above) apps while `getAll()` returns all "acquired"
> (states 2 and 3) apps.  The hypothetical dashboard you mention would want to
> cross-reference the `getInstalled()` results with the `getAll()` results so
> it could show different buttons for each app (e.g. "buy this app,"
> "purchased," and "installed").  A similar argument applies for `getSelf()`.

Makes sense to me, but let's make sure that we update docs to reflect that (tracking bug flagged dev-doc-needed) and that any users of getInstalled know of the implications of this change (persona.org maybe?). We should talk about this in Tuesday's apps meeting about clarifying acquired vs. installed implications in the mozapps API.

> 
> > I'm seeing additional related issues:
> > - What is the expected behavior for uninstall() ? We should probably fail to
> > uninstall an app that is not natively installed.
> 
> CCing Anant to be sure, but I believe that `uninstall()` has additional
> implications for AitC: Uninstalling an app using the mozApps API will cause
> the app to be "unacquired" and this will propagate to your other devices.

Uninstall might also need modification to handle acquired vs. installed, as an uninstall may imply only one (if the app is only acquired) or both (if the app is acquired and installed). This needs to be thought out carefully though to avoid unexpected integration issues - let's talk about this on Tuesday.

> 
> > - Why don't we try to keep the native apps installed list in sync with the
> > UA registry?
> 
> The UA registry keeps track of whether apps are "acquired" and
> `getInstalled` is now keeping track of whether apps are "installed
> (natively)."  Thus, the two shouldn't be kept in sync.

Right. I believe the tracking bug comments in bug 749033 give a rationale for why the two are distinct. See that bug more information.

> 
> > - Why is this windows only? Don't we want the same behavior on Mac (and
> > linux in the future)?
> 
> This is the Window-only bug.  See bug 756307 for Linux and bug 756308 for
> Mac.
> 
> 
> One possible alternative to the implementation in this patch is to add a
> "state" flag to the App object.  The flag would have values "unacquired,"
> "acquired," and "installed."  With that implementation, sites and stores
> would not have to cross-reference `getInstalled()` against `getSelf()` or
> `getAll()`; they could just check the flag.

That's a good idea. Another idea would be modifying the API to distinguish between acquired vs. native installed explicitly, although I'm not sure how easy it would be to make these changes now. Meaning:

- navigator.mozApps.getNativeInstalled() - get all of the apps natively installed on the user's machine
- navigator.mozApps.getAcquired() - get all of the apps acquired by the user
- App object's launch method (already planned to be implemented in bug 745924) - launch the app natively
- App object's uninstall method - Uninstall the app natively from the user's machine
- App object's unacquire method - Unacquire the app from the user's machine
So, if I understand correctly, what you mean by "acquired" is "installed in the DOM registry", and what you mean by "installed" in "natively installed" ? This is confusing.

A dashboard may not be a store (for instance, Gaia's homescreen is a dashboard), so I don't really understand why you expect dashboards in general to cross-reference getAll() and getInstalled().

About adding a flag and additional calls that expose the "native" bits: what you propose is to leak an implementation detail into the API abstraction. This is a bad idea in general. This is pointless in b2g for instance, and potentially for other runtimes that would want to implement the API.
(In reply to Fabrice Desré [:fabrice] from comment #13)
> So, if I understand correctly, what you mean by "acquired" is "installed in
> the DOM registry", and what you mean by "installed" in "natively installed"
> ? This is confusing.
> 
> A dashboard may not be a store (for instance, Gaia's homescreen is a
> dashboard), so I don't really understand why you expect dashboards in
> general to cross-reference getAll() and getInstalled().
> 
> About adding a flag and additional calls that expose the "native" bits: what
> you propose is to leak an implementation detail into the API abstraction.
> This is a bad idea in general. This is pointless in b2g for instance, and
> potentially for other runtimes that would want to implement the API.

Let's talk about this at Tuesday's apps meeting. We need to balance API concerns with the UX feedback we've received about the desktop implementation for the mozapps API. I do note that this isn't a pointless change - a top pain point identified by user feedback in the mozillian preview was the following:

"I do not understand the difference between apps installed in the DOM Registry vs. natively installed and the interactions that follow it"

There's also the need for AITC to distinguish what's apps are acquired for a BID account.

Note - Technically the install functionality is doing native operations per desktop OS, so there are OS-dependent issues that may need to be reflected in other mozapps API interactions. We'll to think about this carefully though.
(In reply to Fabrice Desré [:fabrice] from comment #13)
> So, if I understand correctly, what you mean by "acquired" is "installed in
> the DOM registry", and what you mean by "installed" in "natively installed"
> ? This is confusing.

It's only confusing if you consider apps in the DOM registry as "installed". It might be better to think of apps simply "present" in the DOM registry, they are apps that the user has acquired but not necessarily installed or use actively.

As an analogy, I have acquired 100s of apps on my Android phone but I only have 15 apps I actively use on my dashboard. In that particular case, my DOM registry would consists of 100s of apps, but the dashboard would only show 15. The dashboard needs a way to distinguish between the ones I use and the ones I simply have. I think the word "installed" is a good way to make this distinction, which is why the proposal was to use getInstalled() to only return a subset of the getAll() set.

I agree that adding a flag is not the right way to go. But I think we should have  getInstalled() and getAll() return different things.
(In reply to Jason Smith [:jsmith] from comment #14)
> Note - Technically the install functionality is doing native operations per
> desktop OS, so there are OS-dependent issues that may need to be reflected
> in other mozapps API interactions. We'll to think about this carefully
> though.

This is not necessary. Installing an app from a marketplace is simply a two step process:
1. Acquire the app on behalf of the user (thereby making an entry in the DOM registry)
2. Install the app in the current environment (this is OS-dependent)

These two steps are universal on all platforms (including B2G). Fabrice, I think the B2G dashboard should only show apps I have "installed" as opposed to everything that's in the DOM registry. There could be another dashboard that shows me everything in the registry of course, but it's probably not the primary one I see on unlocking the home screen. Thoughts?
(In reply to Anant Narayanan [:anant] from comment #16)
> (In reply to Jason Smith [:jsmith] from comment #14)
> > Note - Technically the install functionality is doing native operations per
> > desktop OS, so there are OS-dependent issues that may need to be reflected
> > in other mozapps API interactions. We'll to think about this carefully
> > though.
> 
> This is not necessary. Installing an app from a marketplace is simply a two
> step process:
> 1. Acquire the app on behalf of the user (thereby making an entry in the DOM
> registry)
> 2. Install the app in the current environment (this is OS-dependent)
> 
> These two steps are universal on all platforms (including B2G). Fabrice, I
> think the B2G dashboard should only show apps I have "installed" as opposed
> to everything that's in the DOM registry. There could be another dashboard
> that shows me everything in the registry of course, but it's probably not
> the primary one I see on unlocking the home screen. Thoughts?

I don't agree - there is necessity to re-evaluate native vs. acquired in the mozapps API. There's considerations for k9o features coming down the pipeline that require distinguishing both of the states (e.g new tab, awesome bar, AITC), including one bug already planned that's a k9o blocker to modify the API on the behavior for launching an app - bug 745924. Re-evaluating the other API areas could capture problems before a user gets exposed to the problem, although we already know that users are exposed to the problem (there's been a lot of feedback in this area).
(In reply to Anant Narayanan [:anant] from comment #15)

> It's only confusing if you consider apps in the DOM registry as "installed".
> It might be better to think of apps simply "present" in the DOM registry,
> they are apps that the user has acquired but not necessarily installed or
> use actively.

Sure there's a difference between installed apps and apps I use actively. But I don't see how it relates to the DOM registry vs. native installation.
 
> As an analogy, I have acquired 100s of apps on my Android phone but I only
> have 15 apps I actively use on my dashboard. In that particular case, my DOM
> registry would consists of 100s of apps, but the dashboard would only show
> 15. The dashboard needs a way to distinguish between the ones I use and the
> ones I simply have. I think the word "installed" is a good way to make this
> distinction, which is why the proposal was to use getInstalled() to only
> return a subset of the getAll() set.

What you ask here should be implemented by the dashboard, not part of the API. This is a UI/UX feature. For instance, the homescreen can store its status (icons order, folders, etc.) in indexedDB.

> I agree that adding a flag is not the right way to go. But I think we should
> have  getInstalled() and getAll() return different things.

This is already the case. getInstalled() returns apps installed from the calling origin, getAll() apps installed from anywhere.
(In reply to Fabrice Desré [:fabrice] from comment #18) 
> > As an analogy, I have acquired 100s of apps on my Android phone but I only
> > have 15 apps I actively use on my dashboard. In that particular case, my DOM
> > registry would consists of 100s of apps, but the dashboard would only show
> > 15. The dashboard needs a way to distinguish between the ones I use and the
> > ones I simply have. I think the word "installed" is a good way to make this
> > distinction, which is why the proposal was to use getInstalled() to only
> > return a subset of the getAll() set.
> 
> What you ask here should be implemented by the dashboard, not part of the
> API. This is a UI/UX feature. For instance, the homescreen can store its
> status (icons order, folders, etc.) in indexedDB.

Dashboards can only implement it correctly if they are provided that information by the API. We want to allow users to "remotely" install or uninstall an app on any device from any of their other devices. Thus, the proposal is to use getInstalled/getAll as a way of providing this information.

> > I agree that adding a flag is not the right way to go. But I think we should
> > have  getInstalled() and getAll() return different things.
> 
> This is already the case. getInstalled() returns apps installed from the
> calling origin, getAll() apps installed from anywhere.

Yes, but in addition, what is the behavior of getInstalled when called from a dashboard? The proposal is to return applications acquired from anywhere but only installed on the current device. There is no other way for the dashboard to know this information.
(In reply to Jason Smith [:jsmith] from comment #17)

> I don't agree - there is necessity to re-evaluate native vs. acquired in the
> mozapps API. There's considerations for k9o features coming down the
> pipeline that require distinguishing both of the states (e.g new tab,
> awesome bar, AITC), including one bug already planned that's a k9o blocker
> to modify the API on the behavior for launching an app - bug 745924.
> Re-evaluating the other API areas could capture problems before a user gets
> exposed to the problem, although we already know that users are exposed to
> the problem (there's been a lot of feedback in this area).

Fixing bug 745924 doesn't imply any changes to the API. New tab/awesome bar/aitc are chrome land features, so don't warrant a change in a web facing API either.
(In reply to Jason Smith [:jsmith] from comment #17)
> I don't agree - there is necessity to re-evaluate native vs. acquired in the
> mozapps API. There's considerations for k9o features coming down the
> pipeline that require distinguishing both of the states (e.g new tab,
> awesome bar, AITC), including one bug already planned that's a k9o blocker
> to modify the API on the behavior for launching an app - bug 745924.
> Re-evaluating the other API areas could capture problems before a user gets
> exposed to the problem, although we already know that users are exposed to
> the problem (there's been a lot of feedback in this area).

Are you implying we need further changes to the API than what has already been discussed in bug 749033? If so, I'd be interested in hearing your reasoning.
(In reply to Fabrice Desré [:fabrice] from comment #20)
> (In reply to Jason Smith [:jsmith] from comment #17)
> 
> > I don't agree - there is necessity to re-evaluate native vs. acquired in the
> > mozapps API. There's considerations for k9o features coming down the
> > pipeline that require distinguishing both of the states (e.g new tab,
> > awesome bar, AITC), including one bug already planned that's a k9o blocker
> > to modify the API on the behavior for launching an app - bug 745924.
> > Re-evaluating the other API areas could capture problems before a user gets
> > exposed to the problem, although we already know that users are exposed to
> > the problem (there's been a lot of feedback in this area).
> 
> Fixing bug 745924 doesn't imply any changes to the API. New tab/awesome
> bar/aitc are chrome land features, so don't warrant a change in a web facing
> API either.

Let me clarify my wording - I'm meaning the implementation of an exposed API method would change (launch). The bugs in sequence bug 740922 and bug 745924 would change the app object's launch method to launch an app natively, not as a pinned app tab in firefox.

(In reply to Anant Narayanan [:anant] from comment #21)
> Are you implying we need further changes to the API than what has already
> been discussed in bug 749033? If so, I'd be interested in hearing your
> reasoning.

I think I should clarify my wording (might be causing some confusion) - I'm implying that there might be changes to the "implementation" of the functionality of the API, not the API exposed itself - it's more that it might worth thinking about it at the minimum now that we've got a bunch of feedback from the Mozillian Preview. Marco I know was looking for an exposed way in firefox to uninstall an app natively, as his linux implementation does not provide an explicit way to uninstall an app (unless you do it manually yourself by deleting the directory). There's also changes to the app object's launch method needed to launch app natively, not as an app tab (bug 745924 and bug 740922). This is required for the awesome bar and new tab implementation so that if a user clicks an app that they have installed, then it will launch the chromeless shell, not in firefox. Then there's this bug and AITC - the need to distinguish getting apps that are installed on a user's machine vs. acquired. 

I was just generally noting we need to think about these requirements and how best to go about them - if an API change isn't a good idea, then what other options do we have?

I'd prefer to hold this discussion going on in this bug though for now, as Tuesday's meeting would be a perfect time to talk about this in-person. Sound good?
(In reply to Jason Smith [:jsmith] from comment #22)
> I'd prefer to hold this discussion going on in this bug though for now, as
> Tuesday's meeting would be a perfect time to talk about this in-person.
> Sound good?

As long as we can make progress online, doing so is better than having a meeting.  And if we do need a meeting to resolve the issues being raised here, we should call a special one for it, as the Tuesday meeting doesn't have the right set of attendees, and this subject could easily take up an entire meeting slot.
(In reply to Jason Smith [:jsmith] from comment #22)
> I think I should clarify my wording (might be causing some confusion) - I'm
> implying that there might be changes to the "implementation" of the
> functionality of the API, not the API exposed itself - it's more that it
> might worth thinking about it at the minimum now that we've got a bunch of
> feedback from the Mozillian Preview. Marco I know was looking for an exposed
> way in firefox to uninstall an app natively, as his linux implementation
> does not provide an explicit way to uninstall an app (unless you do it
> manually yourself by deleting the directory). There's also changes to the
> app object's launch method needed to launch app natively, not as an app tab
> (bug 745924 and bug 740922). This is required for the awesome bar and new
> tab implementation so that if a user clicks an app that they have installed,
> then it will launch the chromeless shell, not in firefox. Then there's this
> bug and AITC - the need to distinguish getting apps that are installed on a
> user's machine vs. acquired.

Sure, I don't think anyone is debating that the implementations of some API methods may have to change (or if there is any debate, it is restricted to the bug assigned to the implementation of that particular method).

> I was just generally noting we need to think about these requirements and
> how best to go about them - if an API change isn't a good idea, then what
> other options do we have?

The best way to go about them is to change the implementation of the API as necessary on each platform, as we are doing so now in various bugs. There is currently no necessity for changing the mozApps API definition itself, beyond what we discussed in bug 749033; i.e. making getInstalled return only installed apps, while making getAll return all acquired apps.

Fabrice has some concerns about that change which we should discuss in this bug (technically, bug 749033 would be a better place to do so since this API change is not platform specific, but discussing it here is fine with me).
(In reply to Anant Narayanan [:anant] from comment #19)

> Dashboards can only implement it correctly if they are provided that
> information by the API. We want to allow users to "remotely" install or
> uninstall an app on any device from any of their other devices. Thus, the
> proposal is to use getInstalled/getAll as a way of providing this
> information.

Ha, that's a different use case. But the API to use for this must be provided by the "remote install" service, not by mozApps.

> > > I agree that adding a flag is not the right way to go. But I think we should
> > > have  getInstalled() and getAll() return different things.
> > 
> > This is already the case. getInstalled() returns apps installed from the
> > calling origin, getAll() apps installed from anywhere.
> 
> Yes, but in addition, what is the behavior of getInstalled when called from
> a dashboard? The proposal is to return applications acquired from anywhere
> but only installed on the current device. There is no other way for the
> dashboard to know this information.

getInstalled() from a dashboard will return apps that are installed from this dashboard if it's also a store. getAll() is intended to be used by dashboards. Once again, I object to adding in the mozApps API functionnality from what could be a 3rd party service.
(In reply to Fabrice Desré [:fabrice] from comment #25)
> (In reply to Anant Narayanan [:anant] from comment #19)
> 
> > Dashboards can only implement it correctly if they are provided that
> > information by the API. We want to allow users to "remotely" install or
> > uninstall an app on any device from any of their other devices. Thus, the
> > proposal is to use getInstalled/getAll as a way of providing this
> > information.
> 
> Ha, that's a different use case. But the API to use for this must be
> provided by the "remote install" service, not by mozApps.
> 
> > > > I agree that adding a flag is not the right way to go. But I think we should
> > > > have  getInstalled() and getAll() return different things.
> > > 
> > > This is already the case. getInstalled() returns apps installed from the
> > > calling origin, getAll() apps installed from anywhere.
> > 
> > Yes, but in addition, what is the behavior of getInstalled when called from
> > a dashboard? The proposal is to return applications acquired from anywhere
> > but only installed on the current device. There is no other way for the
> > dashboard to know this information.
> 
> getInstalled() from a dashboard will return apps that are installed from
> this dashboard if it's also a store. getAll() is intended to be used by
> dashboards. Once again, I object to adding in the mozApps API functionnality
> from what could be a 3rd party service.

Alright, so let's backup and rethink this. Are there alternative ways outside of the mozapps API we could use to know that an app is or isn't natively installed on a user's machine?
(In reply to Fabrice Desré [:fabrice] from comment #25)
> (In reply to Anant Narayanan [:anant] from comment #19)
> > Dashboards can only implement it correctly if they are provided that
> > information by the API. We want to allow users to "remotely" install or
> > uninstall an app on any device from any of their other devices. Thus, the
> > proposal is to use getInstalled/getAll as a way of providing this
> > information.
> 
> Ha, that's a different use case. But the API to use for this must be
> provided by the "remote install" service, not by mozApps.

I disagree. 1 API is better than 2 APIs. There is no need for some dashboards to know whether an app was installed from a remote device or this one, all it needs to know is that the user installed an app (somehow). Others may care about this distinction, at which point 2 APIs become necessary.

But ignoring that specific use-case, you are making the assumption that the dashboard is always able to tell which apps the user has installed on their current device. This may be true on B2G (you can store this info in localStorage/indexedDB etc.), but this is not true either on Android or the iPhone. Specifically, on Android, we are unable to query for which icons have been placed on the home-screen. Therefore, the list of apps returned by getAll() is insufficient to build a dashboard that will provide all the features we need.
(In reply to Anant Narayanan [:anant] from comment #27)
> (In reply to Fabrice Desré [:fabrice] from comment #25)
> > (In reply to Anant Narayanan [:anant] from comment #19)
> > > Dashboards can only implement it correctly if they are provided that
> > > information by the API. We want to allow users to "remotely" install or
> > > uninstall an app on any device from any of their other devices. Thus, the
> > > proposal is to use getInstalled/getAll as a way of providing this
> > > information.
> > 
> > Ha, that's a different use case. But the API to use for this must be
> > provided by the "remote install" service, not by mozApps.
> 
> I disagree. 1 API is better than 2 APIs. There is no need for some
> dashboards to know whether an app was installed from a remote device or this
> one, all it needs to know is that the user installed an app (somehow).
> Others may care about this distinction, at which point 2 APIs become
> necessary.
> 
> But ignoring that specific use-case, you are making the assumption that the
> dashboard is always able to tell which apps the user has installed on their
> current device. This may be true on B2G (you can store this info in
> localStorage/indexedDB etc.), but this is not true either on Android or the
> iPhone. Specifically, on Android, we are unable to query for which icons
> have been placed on the home-screen. Therefore, the list of apps returned by
> getAll() is insufficient to build a dashboard that will provide all the
> features we need.

IPhone? Did you mean to say desktop? Cause we haven't talked about iOS support yet.

Anyways, for Android implementation, the uninstall flow as you've noted is also problematic due to the issue you've identified. I'll track that issue in bug 761048.

And I agree with the argument you are making - the current implementation of mozapps API just isn't sufficient for k9o needs coming down the pipeline. Fabrice - Are there alternative approaches you would suggest outside of touching the mozapps API?
I think this discussion should happen in dev-webapps. This is really sad to find that some plan changes to an existing API without making a proposal for review first.
(In reply to Fabrice Desré [:fabrice] from comment #29)
> I think this discussion should happen in dev-webapps. This is really sad to
> find that some plan changes to an existing API without making a proposal for
> review first.

We already started this discussion and reviewed ideas on bug 749033. We could add a post to dev-webapps to indicate that if you are interested to contribute ideas or feedback that they could add information to bug 749033. But I don't think duplicating a proposal already discussed in bug 749033 is appropriate, given that there's been a large discussion on that bug with a lot of people that are applicable to this discussion.

I'd move this discussion off this bug - this is supposed to be an implementation bug, not the overarching meta bug where the discussion was happening.
To be clear, I'm not working on this currently.  Has discussion moved to another forum?
(In reply to Tim Abraldes from comment #31)
> To be clear, I'm not working on this currently.  Has discussion moved to
> another forum?

no (at least not in dev-webapps).
(In reply to Fabrice Desré [:fabrice] from comment #32)
> (In reply to Tim Abraldes from comment #31)
> > To be clear, I'm not working on this currently.  Has discussion moved to
> > another forum?
> 
> no (at least not in dev-webapps).

I dropped a post on dev-webapps to advertise discussing this on the meta bug. Let's move the discussion there and try to figure out to resolve the concerns you have expressed.
Blocks: 763786
Target Milestone: Firefox 15 → Firefox 16
Felipe: given the work you recently landed, what additional work do we need to do here in order to identify natively installed apps on Windows?
I spoke with Felipe over PM. He's rolled the patch for this bug into the patch for bug 768276 so we'll close this bug when that one lands.
See Also: → 768276
Depends on: 768276
This bug and patch was included as part of bug 768276
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → DUPLICATE
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: