927310 - Propagate activity-done event from activity frame

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

Currently we're guessing which activity is closing because mozChromeEvent:activity-done only tells us who is the activity caller(manifestURL + pageURL).
I would like to improve this by having activity-done event for each activity frame.

:fabrice said it's possible to do.

Could we implement this in mozBrowserAPI? It should be similar as mozbrowserclose event.

Comment 1

[(inactive after 6/18) Alive Kuo [:alive]]

Proposal:
In ActivityWrapper.wrapMessage, store the innerWindowID or aWindow in the handler,
In ActivityRequestHandler, send the innerWindowID or aWindow to ActivityService once postResult/postError is called.
In ActivityService use notifyObserver about activity-done to mozBrowserChildPreload with subject=innerWindowID,
In mozBrowserChildPreload send mozbrowseractivitydone event out.
At the same time remove the observer in shell.js and listen to mozbrowseractivitydone in system app.

Comment 2

[[:fabrice] Fabrice Desré]

That's almost it, Alive. The mistake you made is that ActivityService lives in the parent, but we need to notify from the child process (inner window IDs are only unique per-process). So we need to notify from ActivityRequestHandler.js and from ActivityWrapper.js

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> That's almost it, Alive. The mistake you made is that ActivityService lives
> in the parent, but we need to notify from the child process (inner window
> IDs are only unique per-process). So we need to notify from
> ActivityRequestHandler.js and from ActivityWrapper.js

Thanks, have a WIP!

Comment 4

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: 927310.patch WIP

Almost done? but the comparation in BrowserElementChildPreload doesn't work
doc != content.document :/

Don't know why yet.

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: 927310-gecko.patch

Add mozbrowseractivitydone event.
The event name is bad but I don't have a better idea.

Although I have no idea how to write mochitest for web activity...Fabrice, any clue?

Comment 6

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: 927310-gaia.patch

Listen to mozbrowser event instead of mozChromeEvent

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 818395 [details] [diff] [review]
927310-gecko.patch

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

For the tests, we have no activities mochitests yet. Maybe you can write some gaia integration tests?

::: dom/activities/src/ActivityWrapper.js
@@ +69,5 @@
> +              return;
> +            }
> +            let docshell = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIWebNavigation);;
> +            Services.obs.notifyObservers(docshell, "activity-done", null);

Instead of sending the docshell, why not send aWindow.document?

Also, you need to remove the two new observers.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Fabrice Desré [:fabrice] from comment #7)
> Comment on attachment 818395 [details] [diff] [review]
> 927310-gecko.patch
> 
> Review of attachment 818395 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> For the tests, we have no activities mochitests yet. Maybe you can write
> some gaia integration tests?

OK, but should be another bug. We have a meta bug for gaia system wide integration test and we plan to break down all what we need to write for all cross-app features. Activity definitely is a big part.

> 
> ::: dom/activities/src/ActivityWrapper.js
> @@ +69,5 @@
> > +              return;
> > +            }
> > +            let docshell = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> > +                                  .getInterface(Ci.nsIWebNavigation);;
> > +            Services.obs.notifyObservers(docshell, "activity-done", null);
> 
> Instead of sending the docshell, why not send aWindow.document?

It's because neither aWindow.document or aWindow.content.document matches content.document in BrowserElementChildPreload...but the process id is the same without doubt between ActivityWrapper and BrowserElementChildPreload.
After talking with Patrick(:kk1fff), he says Justin Lebar told him to compare using docShell.
See http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementChildPreload.js#545

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: 927310-gecko-v2.patch

v2

Comment 10

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

Comment on attachment 818475 [details]
927310-gaia.patch

I wonder if we should move all the mozChromeEvent events to mozbrowseractivity* events altogether? I know we only allow foreground app to make activity request currently through.

Comment 11

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #10)
> Comment on attachment 818475 [details]
> 927310-gaia.patch
> 
> I wonder if we should move all the mozChromeEvent events to
> mozbrowseractivity* events altogether? I know we only allow foreground app
> to make activity request currently through.

I have a plan to move some more events into mozBrowserAPI but may need to discuss with others.
including gUM recording notifications(recording-devices in mozChromeEvent), activitystart(open-app in mozChromeEvent)...
Fabrice, any concern about doing this? I think enhancing mozBrowser events this is must-have in haida.

Activitystart case doesn't block my work so that's why I don't do it in this bug,
but it may matter if one day we have more than one foreground apps at one time. Some day.

Comment 12

[[:fabrice] Fabrice Desré]

Comment on attachment 818869 [details] [diff] [review]
927310-gecko-v2.patch

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

r- because I don't think this really works. see the comment about notifyObservers().

::: dom/activities/src/ActivityWrapper.js
@@ +70,5 @@
> +            }
> +            Services.obs.removeObserver(observer, "activity-error");
> +            Services.obs.removeObserver(observer, "activity-success");
> +            let docshell = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIWebNavigation);;

nit: double ';' at the end of the line.

@@ +72,5 @@
> +            Services.obs.removeObserver(observer, "activity-success");
> +            let docshell = aWindow.QueryInterface(Ci.nsIInterfaceRequestor)
> +                                  .getInterface(Ci.nsIWebNavigation);;
> +            Services.obs.notifyObservers(docshell, "activity-done",
> +                                          { success: (aTopic == 'activity-success' ? true : false) });

the 3rd argument of notifyObservers is a string (see https://mxr.mozilla.org/mozilla-central/source/xpcom/ds/nsIObserverService.idl#61), not a JS object.

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: 927310-gecko-v3.patch

v3: correct success attr.

Comment 14

[[:fabrice] Fabrice Desré]

Comment on attachment 819093 [details] [diff] [review]
927310-gecko-v3.patch

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

r=me with nit addressed

::: dom/browser-element/BrowserElementChildPreload.js
@@ +288,5 @@
>        case 'ask-parent-to-rollback-fullscreen':
>          sendAsyncMsg('rollback-fullscreen');
>          break;
> +      case 'activity-done':
> +        sendAsyncMsg('activitydone', { success: (data == 'activity-success' ? true : false) });

nit: { success: (data == 'activity-success') }

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Attachment: 927310-gecko-v4.patch

checkin patch

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/integration/b2g-inbound/rev/846a7e7f4451

Comment 17

[(inactive after 6/18) Alive Kuo [:alive]]

master
https://github.com/mozilla-b2g/gaia/commit/f6dd2c5e2d1a1d79f0689a174227600028df2007

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/846a7e7f4451