798445 - Send a ChromeEvent when the web activities is completed

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

According to bug 789392 comment 10, apps handling web activities should not be required to use |window.close()| to close itself, nor apps requiring activities are required to use |reopenApp()| to bring itself to foreground when the activities transaction is competed.

Window manager in System app should handle all that on app's behave; we should not leak implementation detail this way. 

Enabling System app to do so requires Gecko to send a ChromeEvents to notify window manager that the web activity is completed, and we should switch to the original requester app.

I am not sure what information is needed to contain in the ChromeEvent. If possible, the identifier (manifestURL) of the requester app should be included.

Require blocking cause we should not expose these improper API requirement to 3rd party apps.

Comment 1

[David Flanagan [:djf]]

I agree with Tim that this should be blocking. If we don't fix it, we expose a broken API to third party developers and apps will be written containing the hacky workarounds that we currently use in Gaia. Fixing it in v2 will be much harder when we have third party apps already written to the broken model.

Comment 2

[[:fabrice] Fabrice Desré]

Attachment: patch

Tim, with this patch the system app can listen to 2 new chromeEvents:
"activity-success" and "activity-error", and get the manifestURL of the app that started the activity in the manifestURL property.

I'm not sure if gaia needs to know if the action was completed by a postError or a postSuccess. Maybe a single "activity-done" is enough.

Also, when |returnValue == false|, we fire a postResult(null) but I'm not sending a chrome event in this case, since we would immediately switch back to the caller. Let me know if that makes sense for you.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Thanks!

(In reply to Fabrice Desré [:fabrice] from comment #2)
> Created attachment 668727 [details] [diff] [review]
> patch
> 
> Tim, with this patch the system app can listen to 2 new chromeEvents:
> "activity-success" and "activity-error", and get the manifestURL of the app
> that started the activity in the manifestURL property.
> 
> I'm not sure if gaia needs to know if the action was completed by a
> postError or a postSuccess. Maybe a single "activity-done" is enough.

'activity-done' or 'activity-complete' would be enough. I don't see any use case currently that System app needs to know the success/error, but it's no harm to expose that in another chromeEvent detail property like |detail.success|.
 
> Also, when |returnValue == false|, we fire a postResult(null) but I'm not
> sending a chrome event in this case, since we would immediately switch back
> to the caller. Let me know if that makes sense for you.

That make sense I guess. That will take away the ability for handling app to switching back to the caller app (for the |returnValue == false| case), but I guess that shouldn't matter.

I am testing the patch against Gaia patch right now. Once the chromeEvent format finalized let's fix Gaia in another bug that dependent on this one.

Comment 4

[[:fabrice] Fabrice Desré]

Attachment: patch v2

Tim, can you try this one? It sends a single "activity-done" chrome event with an additional "success" boolean.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Fabrice Desré [:fabrice] from comment #4)
> Created attachment 669025 [details] [diff] [review]
> patch v2
> 
> Tim, can you try this one? It sends a single "activity-done" chrome event
> with an additional "success" boolean.

That indeed works. However as mentioned on IRC, manifestURL is not enough to specify which app we should be bring back because we got entry points. Together with situation described in bug 796629 that just make things very complex.

I am just going to save the "origin" of the displayed app when the activity starts, and bring the displayed app back when activity is done. WIP patch is coming (to another bug)

Comment 6

[[:fabrice] Fabrice Desré]

Tim, do we still want this? if yes, do we need manifestURL + pageURL to solve the entry points issue?

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Fabrice Desré [:fabrice] from comment #6)
> Tim, do we still want this? if yes, do we need manifestURL + pageURL to
> solve the entry points issue?

Yes we still need this chromeEvent. Just that we probably don't need the information from it's detail object.

In my patch for Gaia in bug 799039, I simply record identifier of the currently displayed app. Let's hope there are no concurrent web activities ... thanks.

Comment 8

[[:fabrice] Fabrice Desré]

Attachment: patch v3

Now with a pageURL property added to the activity-done event.

Comment 9

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 670949 [details] [diff] [review]
patch v3

Review of attachment 670949 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the nits addressed.

::: b2g/chrome/content/shell.js
@@ +857,5 @@
> +      manifestURL: data.manifestURL,
> +      pageURL: data.pageURL
> +    });
> +  }, 'activity-done', false);
> +})();

nit: I know you don't like the ' so you have polluted this file with ". Which is fine for me as long as you don't mix them :)

Also I don't really see the need to have a closure 'activityReturn' here? There is nothing to leak and pollute the global scope.

::: dom/activities/src/ActivitiesService.jsm
@@ +198,4 @@
>            "id": aMsg.id,
>            "error": "NO_PROVIDER"
>          });
> +        // XXX No need to notifiy observers since we don't open any other app.

Do you want to keep the XXX since you said there is nothing to do?

@@ +212,4 @@
>              "id": aMsg.id,
>              "error": "USER_ABORT"
>            });
> +          // XXX No need to notifiy observers since we don't open any other app.

Same comment as above.

::: dom/activities/src/ActivityProxy.js
@@ +47,5 @@
> +    let manifestURL = DOMApplicationRegistry.getManifestURLByLocalId(principal.appId);
> +    cpmm.sendAsyncMessage("Activity:Start", { id: this.id,
> +                                              options: aOptions,
> +                                              manifestURL: manifestURL,
> +                                              pageURL: aWindow.document.location.href });

It seems like you can avoid the call do DOMApplicationRegistry if there is no appId (because the activity is started from a web page).

Comment 10

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/integration/mozilla-inbound/rev/683148127c83

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/683148127c83

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/10a4db1a1d8e