In bug 779982, isInstalled was implemented and returns true or false in the callback. One could make a case for having it return the actual app object instead so that the website may call launch() (with the appropriate popup blocking mechanisms in place).

I kind of disagree with this change. The name isInstalled() clearly leads to returning a boolean value. But actually the worst part is that isInstalled() should not be async - I missed it in the first patch.

I don't think that we should make isInstalled sync. All other parts of the API are async, and most callers of the API are running in a separate process, so making it async seems more consistent with everything else and also simpler.

Comment on attachment 664523 [details] [diff] [review] patch 1 Review of attachment 664523 [details] [diff] [review]: ----------------------------------------------------------------- Obs.: this is just reviewing the patch without consideration if this API change should happen or not. I'll let Jonas/Anant/Fabrice figure that out ::: dom/apps/src/Webapps.jsm @@ +904,4 @@ > > for (let appId in this.webapps) { > if (this.webapps[appId].manifestURL == aData.manifestURL) { > + aData.app = AppsUtils.cloneAppObject(this.webapps[appId]); Fabrice should be able to tell for sure, but I believe you'll also want the _readManifests part like the other functions do to include the JSON manifest content in the app object (unless you wanted on purpose to not include it here)

Just added _readManifests. Waiting for comments from sicking

Jonas can you give a feedback about this bug?

I'm ok with doing this. I do agree with Fabrice that the name gets somewhat awkward. Maybe rename it to checkInstalled? Or getInstalled? I assume that there's a agreement that if the function returns an App object that it needs to be async? (Sync is of course almost always better if we can make it work, but I was under the impression that that was hard on for example desktop where it would require checking a app registry which might live in a different process, or even just on disk)

It's always harder to make these sync since they are sending/receiving async messages from the content to the parent. I guess we could spin the event loop in the child...

Never spin the event loop! In B2G it'd be easy to make a sync message to the parent process. On desktop we couldn't really do that though. So I suggest we keep the function async.

(In reply to Jonas Sicking (:sicking) from comment #9) > In B2G it'd be easy to make a sync message to the parent process. That doesn't help much since the parent is doing async i/o to get the manifest.

We should definitely keep it async then.

Comment on attachment 665291 [details] [diff] [review] patch 2 Review of attachment 665291 [details] [diff] [review]: ----------------------------------------------------------------- Patch looks good, but let's change the name to checkInstalled as Jonas suggested. The name getInstalled already exists so we can't use that. Although with these changes to getSelf/checkInstalled we should probably think if the getInstalled function still makes sense to exist. But apps out there might already be using it so changing it now is probably not safe

isInstalled has been renamed to checkInstalled

Comment on attachment 665828 [details] [diff] [review] patch 2b Review of attachment 665828 [details] [diff] [review]: ----------------------------------------------------------------- It doesn't matter in practice, but for correctness please update the uuid in the idl before landing

I don't see any Try results here, so I've gone ahead and done so. I'll land it on inbound if it's green. https://tbpl.mozilla.org/?tree=Try&rev=d434f0c44563

(In reply to Ryan VanderMeulen from comment #16) > https://tbpl.mozilla.org/?tree=Try&rev=d434f0c44563 Green on Try. https://hg.mozilla.org/integration/mozilla-inbound/rev/dfe50b46ef8c