746213 - Support mozApps.getSelf/getInstalled in desktop runtime to allow apps to access receipts

Status: VERIFIED FIXED

(Firefox Graveyard :: Webapp Runtime, defect)

Description

[Ed Lee :Mardak]

Marketplace relies on getInstalled to determine if an app should be installable or just show "installed". The Marketplace app run natively currently would always show apps as installable because getSelf and getInstalled both run onsuccess with a null result.

Comment 1

[Ed Lee :Mardak]

Note, probably similar to bug 738832 but that one is about changing state (installing/uninstalling) where as this bug is just reading out state.

Comment 2

[Ed Lee :Mardak]

To be clear, getSelf() results in success with "null" while getInstalled results in success with "[]".

Comment 3

[Ed Lee :Mardak]

Attachment: v1

Not exactly how it worked before though in previous tryserver builds. But all this does is..

Import Webapps.jsm to initialize DOMApplicationRegistry, which handles Webapps:GetSelf, etc. messages.

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 616777 [details] [diff] [review]
v1

cool, is this all that is needed for it to work? I'm giving the review here on the simple change (importing Webapps.jsm), but I don't know offhand if Webapps.jsm was prepared to be used in the runtime. It'd be great if Fabrice or someone could comment on that.

Comment 5

[Ed Lee :Mardak]

(In reply to Felipe Gomes (:felipe) from comment #4)
> cool, is this all that is needed for it to work?
At least on mac, it needs bug 746771 to correctly set registryDir.

I noticed that the DOMRequest was being returned but no onsuccess/onerror. So I walked through the Firefox handler and saw that nothing was receiving the Webapps:GetSelf message which is usually handled by DOMApplicationRegistry.

Just importing the file will make sure DOMApplicationRegistry is inited and ready to receive and send messages back to WebappsRegistry of Webapps.js.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Felipe: I modified Webapps.jsm to be usable by the runtime as part of the initial implementation of the launcher/shell in my fix for bug 725408:

http://hg.mozilla.org/mozilla-central/diff/ef55c163a23a/dom/base/Webapps.jsm

So it's ok to import it, and it's also necessary to do so, as Mardak notes.  Ideally, we'd have a way to register modules to get loaded on startup, as we do components.  But this is the way to do it in the meantime.

The only suggestions I'd made for this patch are:

1. add a comment explaining why the module is being imported even though it isn't being used (otherwise someone might try to remove it later, thinking it cruft);

2. import it in the command-line handler, which is the initial entry point into the app and as close as you can get to "on startup" (although on the other hand it might be better to load it as lazily as possible to reduce impact on startup).

Comment 7

[Ed Lee :Mardak]

(In reply to Myk Melez [:myk] [@mykmelez] from comment #6)
> 2. import it in the command-line handler
Thinking a bit more about where to import Webapps.jsm..

Fabrice, any particular reason why Webapps.jsm shouldn't be imported by Webapp.js? The former, a js module, handles messages sent by the latter, a js component. But they don't work without the other.

We run into this bug with the native runtime because the js module wasn't getting loaded.

Comment 8

[[:fabrice] Fabrice Desré]

(In reply to Edward Lee :Mardak from comment #7)
 
> Fabrice, any particular reason why Webapps.jsm shouldn't be imported by
> Webapp.js? The former, a js module, handles messages sent by the latter, a
> js component. But they don't work without the other.

Webapps.js is the content part of the API, and the .jsm is running in the parent process when using e10s. So you can't load the .jsm from the .js.

The .jsm must indeed be loaded by the "chrome" to get and send messages.

> We run into this bug with the native runtime because the js module wasn't
> getting loaded.

Expected. this is why we load the .jsm in nsBrowserGlue.js in fx desktop.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

nsBrowserGlue.js is a component that loads on app-startup.  It doesn't explicitly load Webapps.jsm, but it does define a lazy loader for webappsUI.jsm and then calls webappsUI.init() on final-ui-startup; webappsUI.jsm then loads Webapps.jsm.

So Webapps.jsm gets loaded on final-ui-startup in Firefox.  And perhaps we should do the same.  But it seems sufficient for now to load it either when opening the first window or when handling the command-line (although it isn't clear when this happens relative to that notification).

Aside: I'm concerned about the way Webapps.jsm loads the main registry file (<profile>/webapps/webapps.json) asynchronously.  If the first thing a webapp does when it starts up is ask for its receipt, can it race the loading of that file?  Seems like we should make sure it doesn't perhaps by queuing up getSelf etc. calls while that file is loading.  But in any case that's a separate bug.

Comment 10

[Ed Lee :Mardak]

(In reply to Fabrice Desré [:fabrice] from comment #8)
> The .jsm must indeed be loaded by the "chrome" to get and send messages.
So then we should keep the import in content/webapp.js as opposed to in CommandLineHandler.js which would be running in a different context (?)

Comment 11

[Myk Melez [:myk] [@mykmelez]]

(In reply to Edward Lee :Mardak from comment #10)
> (In reply to Fabrice Desré [:fabrice] from comment #8)
> > The .jsm must indeed be loaded by the "chrome" to get and send messages.
> So then we should keep the import in content/webapp.js as opposed to in
> CommandLineHandler.js which would be running in a different context (?)

By "chrome", Fabrice apparently means the chrome process, as opposed to the content process, where Webapps.js is executed when e10s is enabled (i.e. in Fennec).  In the runtime, as in desktop Firefox, e10s is disabled, and there is only one process, so any script can load Webapps.jsm, even if it's in a different context from the browser/runtime chrome code.

Comment 12

[Ed Lee :Mardak]

Attachment: for checkin

Comment 13

[Ed Lee :Mardak]

Comment on attachment 616975 [details] [diff] [review]
for checkin

Could almost fall into a=desktop-only but webapprt/ isn't in the auto-approved list.

Comment 14

[[:fabrice] Fabrice Desré]

That looks fine to me, but you need a proper review before landing...

Comment 15

[Ed Lee :Mardak]

Is r+felipe not enough to land in webapprt/ ?

Comment 16

[[:fabrice] Fabrice Desré]

ha ok, he r+'d the previous one. As far as landing rules goes, that looks really safe for fennec indeed.

Comment 17

[Ed Lee :Mardak]

Returning to Web Apps product as we don't need separate mozilla-central approval:

felipe: just got clearance from akeybl to land webapprt/ changes under the a=desktop-only rule

Comment 18

[Ed Lee :Mardak]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c141dcc8c829

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c141dcc8c829

Comment 20

[Jason Smith [:jsmith]]

Ed - Trying to figure out a good way to verify this. Shall I verify that I can integrate correctly with getSelf and getInstalled within the webrt for desktop with David's smoke tests for the mozapps API (the behavior on firefox should be the same as the webrt for desktop)?

Comment 21

[Ed Lee :Mardak]

I tested it by calling mozApps.install with the receipts object and checking it if it exists with mozApps.getSelf (in both Firefox and WebRT).

Comment 22

[Jason Smith [:jsmith]]

Verified on Win 7 64-bit. Created my own localhost test app to verify this. Also verified it through Ed's app as well.