Propagate activity-done event from activity frame

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: alive, Assigned: alive)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

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.
Blocks: 907013
Blocks: 911053
No longer blocks: 907013
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.
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
(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!
Created attachment 818327 [details] [diff] [review]
927310.patch WIP

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

Don't know why yet.
Created attachment 818395 [details] [diff] [review]
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?
Attachment #818395 - Flags: review?(fabrice)
Attachment #818327 - Attachment is obsolete: true
Created attachment 818475 [details]
927310-gaia.patch

Listen to mozbrowser event instead of mozChromeEvent
Attachment #818475 - Flags: review?(timdream)
Attachment #818475 - Attachment description: 927310.patch → 927310-gaia.patch
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.
Attachment #818395 - Flags: review?(fabrice) → feedback+
(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
Created attachment 818869 [details] [diff] [review]
927310-gecko-v2.patch

v2
Attachment #818395 - Attachment is obsolete: true
Attachment #818869 - Flags: review?(fabrice)
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.
Attachment #818475 - Flags: review?(timdream) → review+
(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.
Blocks: 928433
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.
Attachment #818869 - Flags: review?(fabrice) → review-
Created attachment 819093 [details] [diff] [review]
927310-gecko-v3.patch

v3: correct success attr.
Attachment #819093 - Flags: review?(fabrice)
Attachment #818869 - Attachment is obsolete: true
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') }
Attachment #819093 - Flags: review?(fabrice) → review+
Created attachment 820095 [details] [diff] [review]
927310-gecko-v4.patch

checkin patch
Attachment #819093 - Attachment is obsolete: true
Keywords: checkin-needed
master
https://github.com/mozilla-b2g/gaia/commit/f6dd2c5e2d1a1d79f0689a174227600028df2007
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.