Send a ChromeEvent when the web activities is completed

RESOLVED FIXED

Status

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: timdream, Assigned: fabrice)

Tracking

unspecified
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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.
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.
blocking-basecamp: ? → +
Assignee

Comment 2

7 years ago
Posted patch patch (obsolete) — Splinter Review
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.
Assignee: nobody → fabrice
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.
blocking-basecamp: + → ?
Assignee

Comment 4

7 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Tim, can you try this one? It sends a single "activity-done" chrome event with an additional "success" boolean.
Attachment #668727 - Attachment is obsolete: true
(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)
Assignee

Comment 6

7 years ago
Tim, do we still want this? if yes, do we need manifestURL + pageURL to solve the entry points issue?
(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.
Assignee

Comment 8

7 years ago
Posted patch patch v3Splinter Review
Now with a pageURL property added to the activity-done event.
Attachment #669025 - Attachment is obsolete: true
Attachment #670949 - Flags: review?(21)
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).
Attachment #670949 - Flags: review?(21) → review+
https://hg.mozilla.org/mozilla-central/rev/683148127c83
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.