Last Comment Bug 746213 - Support mozApps.getSelf/getInstalled in desktop runtime to allow apps to access receipts
: Support mozApps.getSelf/getInstalled in desktop runtime to allow apps to acce...
Status: VERIFIED FIXED
[qa!]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Webapp Runtime (show other bugs)
: unspecified
: All All
: -- normal
: ---
Assigned To: Ed Lee :Mardak
: Jason Smith [:jsmith]
Mentors:
Depends on: 746771
Blocks: 738816 738832
  Show dependency treegraph
 
Reported: 2012-04-17 10:17 PDT by Ed Lee :Mardak
Modified: 2016-03-21 12:39 PDT (History)
7 users (show)
jsmith: in‑moztrap+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (1.07 KB, patch)
2012-04-19 15:30 PDT, Ed Lee :Mardak
felipc: review+
Details | Diff | Splinter Review
for checkin (1.12 KB, patch)
2012-04-20 08:19 PDT, Ed Lee :Mardak
felipc: review+
Details | Diff | Splinter Review

Description Ed Lee :Mardak 2012-04-17 10:17:21 PDT
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 2012-04-17 10:18:03 PDT
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 2012-04-17 16:06:22 PDT
To be clear, getSelf() results in success with "null" while getInstalled results in success with "[]".
Comment 3 Ed Lee :Mardak 2012-04-19 15:30:55 PDT
Created attachment 616777 [details] [diff] [review]
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 me!) 2012-04-19 15:44:27 PDT
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 2012-04-19 15:59:50 PDT
(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] 2012-04-19 16:33:33 PDT
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 2012-04-19 16:57:36 PDT
(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é 2012-04-19 17:27:50 PDT
(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] 2012-04-19 18:03:59 PDT
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 2012-04-19 19:50:24 PDT
(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] 2012-04-20 08:07:19 PDT
(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 2012-04-20 08:19:48 PDT
Created attachment 616975 [details] [diff] [review]
for checkin
Comment 13 Ed Lee :Mardak 2012-04-20 08:47:25 PDT
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é 2012-04-20 10:01:17 PDT
That looks fine to me, but you need a proper review before landing...
Comment 15 Ed Lee :Mardak 2012-04-20 10:05:40 PDT
Is r+felipe not enough to land in webapprt/ ?
Comment 16 [:fabrice] Fabrice Desré 2012-04-20 10:20:30 PDT
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 2012-04-20 15:26:21 PDT
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 19 Phil Ringnalda (:philor) 2012-04-21 23:52:57 PDT
https://hg.mozilla.org/mozilla-central/rev/c141dcc8c829
Comment 20 Jason Smith [:jsmith] 2012-04-23 22:11:15 PDT
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 2012-04-24 09:53:45 PDT
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] 2012-05-16 23:57:33 PDT
Verified on Win 7 64-bit. Created my own localhost test app to verify this. Also verified it through Ed's app as well.

Note You need to log in before you can comment on or make changes to this bug.