Closed Bug 794102 Opened 7 years ago Closed 7 years ago

isInstalled should return an app object

Categories

(Core Graveyard :: DOM: Apps, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: anant, Assigned: baku)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 3 obsolete files)

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).
Assignee: nobody → amarchesini
OS: Linux → All
Hardware: x86_64 → All
Attached patch patch 1 (obsolete) — Splinter Review
Attachment #664523 - Flags: review?(felipc)
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)
Attachment #664523 - Flags: review?(felipc)
Attached patch patch 2 (obsolete) — Splinter Review
Just added _readManifests. Waiting for comments from sicking
Attachment #664523 - Attachment is obsolete: true
Attachment #665291 - Flags: review?(felipc)
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
Attachment #665291 - Flags: review?(felipc) → feedback+
Attached patch patch 2b (obsolete) — Splinter Review
isInstalled has been renamed to checkInstalled
Attachment #665291 - Attachment is obsolete: true
Attachment #665828 - Flags: review?(felipc)
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
Attachment #665828 - Flags: review?(felipc) → review+
Attached patch patch 2cSplinter Review
Attachment #665828 - Attachment is obsolete: true
Attachment #666475 - Flags: review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/dfe50b46ef8c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Whiteboard: [qa-]
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.