779982 - Change behaviour of getSelf and add amInstalled function

Status: RESOLVED FIXED

(Core Graveyard :: DOM: Apps, defect)

Description

[Anant Narayanan [:anant]]

Bug 775847 will add support for multiple apps per origin, but a pre-requisite for that is to change the behaviour of getSelf and create a new function.

- getSelf should take 1 argument: the path to the full manifest path representing the app that is trying to be retrieved. The callback will be invoked with an app record if the user is currently using the app, and a null object if not.

- A new function amInstalled will be introduced, the callback to which will be invoked with true if the application has been installed by the user, irrespective of whether the app is currently active or not, and false if the app isn't installed.

Examples:

- User launches App1 via a WebRT (icon on FirefoxOS, Android homescreen or Desktop app):
-- navigator.mozApps.getSelf("http://example.org/app1.manifest", cb); will result in cb(appObject) where appObject is the app record for App1.
-- navigator.mozApps.amInstalled("http://example.org/app1.manifest", cb); will result in cb(true).

- User visits http://example.org/app1/foo in the browser (on any platform):
-- getSelf("http://example.org/app1.manifest", cb); will result in cb(false).
-- getInstalled("http://example.org/app1.manifest", cb); will result in cb(true) if the app is "natively" installed, cb(false) if not.

Comment 1

[Anant Narayanan [:anant]]

This bug should block basecamp.

Comment 2

[Jason Smith [:jsmith]]

(In reply to Anant Narayanan [:anant] from comment #1)
> This bug should block basecamp.

Someone will probably ask this question in triage, but can provide the rationale for the nomination?

Comment 3

[:Felipe Gomes (needinfo for replies!)]

> - User visits http://example.org/app1/foo in the browser (on any platform):
> -- getSelf("http://example.org/app1.manifest", cb); will result in cb(false).

How can anyone develop or run a paid app in Firefox if getSelf in the browser will always return null, and they won't have access to the receipts?

This got really close to what I think can work, but I don't see the reason to have a different behavior in app or in the browser. With the parameter added to getSelf, the webpage is telling us what is the app, so we can always return it (if called from the same origin).
Or, saying the same thing in other words: if getSelf will only return something when the app is being run, there's no need for it to have a parameter.

Comment 4

[Anant Narayanan [:anant]]

For development purposes, the thinking is that we might add a development mode in Firefox where it is possible to load a given URL in "app mode". This will be a developer-facing feature, and web pages will always load as regular pages in the browser and will only be treated as part of an app when launched in the web runtime.

Does this sound reasonable?

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Yeah, kinda.. I dislike the fact that the proposal is to make it impossible to run an app in Firefox unless we introduce an app mode for users, but from what I understand, this is intentional [1].


But going back to my second point: if this is the way forward, there's no need to change the API. If getSelf only works in app mode, we already know which app is running, so there's no need to pass the manifest as the parameter. Jonas, is there another reason to pass the manifest? Is it for future-proofing in case we want to change the stance about [1] later?

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, I don't think getSelf needs to take a manifest URL.

Comment 7

[Andrew Overholt [:overholt]]

We want to fix this before we ship.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch 1

I need just a feedback because I still have 1 issue with android: a test fails :/

Comment 9

[Andrea Marchesini [:baku]]

Attachment: patch 1

Comment 10

[Andrea Marchesini [:baku]]

Attachment: patch 1

title updated

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think you might need to add a same-origin check for amInstalled somewhere. It should not be possible for http://foo.com to call amInstalled("http://bar.com"). We should probably throw an exception if that's called.

Comment 12

[Andrea Marchesini [:baku]]

amInstalled() doesn't have any parameters. It checks if the current origin has been installed.
Is this what we want? amInstalled() => true/false.
Otherwise we should change the name to: isItInstalled/isInstalled('http://foo.com')

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 663437 [details] [diff] [review]
patch 1

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

::: dom/apps/src/Webapps.jsm
@@ +440,5 @@
>        case "Webapps:Launch":
>          Services.obs.notifyObservers(mm, "webapps-launch", JSON.stringify(msg));
>          break;
> +      case "Webapps:AmInstalled":
> +        this.amInstalled(msg);

pass the mm as a second parameter to the function like the other cases do

@@ +870,5 @@
> +
> +    // Invalid appId:
> +    if (aData.appId == Ci.nsIScriptSecurityManager.NO_APP_ID ||
> +        aData.appId == Ci.nsIScriptSecurityManager.UNKNOWN_APP_ID) {
> +      ppmm.broadcastAsyncMessage("Webapps:GetSelf:Return:OK", aData);

why the usage of broadcastAsyncMessage instead of using the proper message manager (aMm) and calling sendAsyncMessage?.

Same comment applies to the other two usages below

::: dom/interfaces/apps/nsIDOMApplicationRegistry.idl
@@ +96,5 @@
>  
>    /**
> +   * the request will return true if the app from that origin is installed
> +   */
> +  nsIDOMDOMRequest amInstalled();

I thought that the proposal for amInstalled was to take one parameter (while getSelf remains parameter-less), so that it can be called from outside of an app context and support multiple apps per origin (meaning that just using the origin of the caller is not enough)


(the change in the .idl file requires updating the uuid of the interface)

Comment 14

[Andrea Marchesini [:baku]]

Attachment: patch 2

We should not throw an exception if http://foo.com calls amInstalled("http://bar.com"). Otherwise http://foo.com can catch it and knows that http://bar.com is installed.

If we have multiple apps per origin, we should change: getManifestFor too and return an array of 'manifests'.

Comment 15

[Marco Castelluccio [:marco]]

(In reply to Andrea Marchesini (:baku) from comment #14)
> We should not throw an exception if http://foo.com calls
> amInstalled("http://bar.com"). Otherwise http://foo.com can catch it and
> knows that http://bar.com is installed.

If you checked the origin and threw an exception before doing anything else, it would be impossible to know whether the application is installed or not.

Comment 16

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

See comment 0 which is right regarding amInstalled, (but wrong about getSelf)

The idea is that amInstalled (or isInstalled) takes a full URL. It then returns true if and only if an app is installed with that URL as manifest URL. I.e. its not enough that the origin matches, the full URL needs to match.

The goal of this is to make the API work once we support multiple apps per origin. At that point developers nees a way to check which specific app or apps have been installed.

And yes, the idea would be to throw immediately if someone tries to check if an app from another origin has been installed. Before you do any checks regarding what is installed. That way no information is leaked.

Comment 17

[:Felipe Gomes (needinfo for replies!)]

yeah, the latest patch implements the proposed behavior. The patch looks fine as it is but it would be better to throw the exception on out-of-origin as Marco and Jonas suggested

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch 3

nodePrincipal.checkMayLoad throws the exception if needed.

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 663970 [details] [diff] [review]
patch 3

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

I just reviewed the amInstalled function. r=me on that with the below comment.

::: dom/apps/src/Webapps.js
@@ +154,5 @@
>      return request;
>    },
>  
> +  amInstalled: function(aManifestURL) {
> +    this._window.document.nodePrincipal.checkMayLoad(Services.io.newURI(aManifestURL, null, null),

Actually, you should interpret the passed in URI as a relative URI compared to the baseURI of the document. You can do that by doing

Services.io.newURI(aManifestURL, null, this._window.document.baseURIObject);

when creating the URI. And then using that both when calling checkMayLoad and when sending the URI through the message manager.

Comment 20

[Andrea Marchesini [:baku]]

Attachment: patch3b

let manifestURL = Services.io.newURI(aManifestURL, null, this._window.document.baseURIObject);
this._window.document.nodePrincipal.checkMayLoad(manifestURL, true, false);

...

manifestURL: manifestURL.spec,

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 663984 [details] [diff] [review]
patch3b

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

I just reviewed a small part, so you need Felipe's review too.

Comment 22

[Andrea Marchesini [:baku]]

Attachment: patch 4

amInstalled => isInstalled

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Andrea: Can you also rename 'amInstalled' to 'isInstalled'. I checked with Anant and he agreed that isInstalled made more sense now that the function takes an argument (an old version of amInstalled didn't take any arguments and had a slightly different functionality).

Comment 24

[Andrea Marchesini [:baku]]

hehe already done :)

> Andrea: Can you also rename 'amInstalled' to 'isInstalled'.

Comment 25

[Anant Narayanan [:anant]]

Andrea, Jonas,

We should also consider having isInstalled() return an app object instead of a boolean true/false. One use case might be for a web page to present a "Launch" button if the app is accessed, but not in app mode.

What do you think?

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think we should look at that. But let's land this patch as-is for now since it's ready to go.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

I don't see any Try results posted, so I've pushed this to be safe. I'll land it if the push is green.
https://tbpl.mozilla.org/?tree=Try&rev=7c6f0e0bae23

Comment 28

[Jason Smith [:jsmith]]

(In reply to Anant Narayanan [:anant] from comment #25)
> Andrea, Jonas,
> 
> We should also consider having isInstalled() return an app object instead of
> a boolean true/false. One use case might be for a web page to present a
> "Launch" button if the app is accessed, but not in app mode.
> 
> What do you think?

Can you file a followup bug for this?

Comment 29

[Ryan VanderMeulen [:RyanVM]]

(In reply to Ryan VanderMeulen from comment #27)
> https://tbpl.mozilla.org/?tree=Try&rev=7c6f0e0bae23

Green on Try.

https://hg.mozilla.org/integration/mozilla-inbound/rev/878af0c5f508

Comment 30

[Anant Narayanan [:anant]]

(In reply to Jason Smith [:jsmith] from comment #28) 
> Can you file a followup bug for this?

Bug 794102.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/878af0c5f508