Closed Bug 779982 Opened 7 years ago Closed 7 years ago

Change behaviour of getSelf and add amInstalled function

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(blocking-basecamp:+)

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: anant, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed, feature, Whiteboard: [LOE:M][WebAPI:P3], [qa-])

Attachments

(1 file, 6 obsolete files)

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.
This bug should block basecamp.
Blocks: 775847
blocking-basecamp: --- → ?
(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?
Keywords: dev-doc-needed
> - 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.
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?
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?
No, I don't think getSelf needs to take a manifest URL.
We want to fix this before we ship.
blocking-basecamp: ? → +
Assignee: nobody → jonas
Whiteboard: [WebAPI:P3]
Keywords: feature
Whiteboard: [WebAPI:P3] → [LOE:M][WebAPI:P3]
Assignee: jonas → amarchesini
Attached patch patch 1 (obsolete) — Splinter Review
I need just a feedback because I still have 1 issue with android: a test fails :/
Attachment #663346 - Flags: feedback?(felipc)
Depends on: 793211
Attached patch patch 1 (obsolete) — Splinter Review
Attachment #663346 - Attachment is obsolete: true
Attachment #663346 - Flags: feedback?(felipc)
Attachment #663435 - Flags: review?(felipc)
Attached patch patch 1 (obsolete) — Splinter Review
title updated
Attachment #663435 - Attachment is obsolete: true
Attachment #663435 - Flags: review?(felipc)
Status: NEW → ASSIGNED
Attachment #663437 - Flags: review?(felipc)
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.
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 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)
Attachment #663437 - Flags: review?(felipc)
Attached patch patch 2 (obsolete) — Splinter Review
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'.
Attachment #663437 - Attachment is obsolete: true
Attachment #663808 - Flags: review?(felipc)
(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.
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.
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
Attachment #663808 - Flags: review?(felipc) → feedback+
Attached patch patch 3 (obsolete) — Splinter Review
nodePrincipal.checkMayLoad throws the exception if needed.
Attachment #663808 - Attachment is obsolete: true
Attachment #663970 - Flags: review?(jonas)
Attachment #663970 - Flags: feedback?(felipc)
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.
Attachment #663970 - Flags: review?(jonas) → review+
Attached patch patch3b (obsolete) — Splinter Review
let manifestURL = Services.io.newURI(aManifestURL, null, this._window.document.baseURIObject);
this._window.document.nodePrincipal.checkMayLoad(manifestURL, true, false);

...

manifestURL: manifestURL.spec,
Attachment #663970 - Attachment is obsolete: true
Attachment #663970 - Flags: feedback?(felipc)
Attachment #663984 - Flags: review+
Attachment #663984 - Flags: feedback?(felipc)
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.
Attachment #663984 - Flags: feedback?(felipc) → review?(felipc)
Attached patch patch 4Splinter Review
amInstalled => isInstalled
Attachment #663984 - Attachment is obsolete: true
Attachment #663984 - Flags: review?(felipc)
Attachment #664128 - Flags: review?(felipc)
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).
Attachment #664128 - Flags: review?(felipc) → review+
hehe already done :)

> Andrea: Can you also rename 'amInstalled' to 'isInstalled'.
Keywords: checkin-needed
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?
I think we should look at that. But let's land this patch as-is for now since it's ready to go.
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
(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?
(In reply to Jason Smith [:jsmith] from comment #28) 
> Can you file a followup bug for this?

Bug 794102.
Whiteboard: [LOE:M][WebAPI:P3] → [LOE:M][WebAPI:P3], [qa-]
https://hg.mozilla.org/mozilla-central/rev/878af0c5f508
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
No longer blocks: 775847
Blocks: 778277
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.