1386696 - Fix content event forwarding

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

[Eugen Sawin [:esawin]]

Fix content event forwarding, as introduced in bug 1322586.

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1386696-1.0-Provide-event-dispatcher-to-GeckoVie.patch

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1386696-2.0-Ensure-content-event-forwarding-is-h.patch

With e10s enabled, this patch fixes variable shadowing in DispatcherDelegate.dispatch (data).

With e10s disabled, this patch should ensure that we differentiate between the frame script environment correctly since IS_PARENT_PROCESS is not adequate for it that in this case.
We also make sure to pass the window holding the message manager when creating the dispatcher in EventDispatcher.receiveMessage.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8892970 [details] [diff] [review]
0002-Bug-1386696-2.0-Ensure-content-event-forwarding-is-h.patch

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

::: mobile/android/modules/geckoview/Messaging.jsm
@@ +254,4 @@
>        return;
>      }
>  
> +    let win = aMsg.target.ownerGlobal || aMsg.target.contentWindow || {};

What's this change for?

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8892966 [details] [diff] [review]
0001-Bug-1386696-1.0-Provide-event-dispatcher-to-GeckoVie.patch

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

::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm
@@ +24,4 @@
>    constructor(aModuleName, aMessageManager) {
>      this.moduleName = aModuleName;
>      this.messageManager = aMessageManager;
> +    this.eventDispatcher = EventDispatcher.for(this);

We should only pass a window to `EventDispatcher.for`, and `this` here is not a window. I think you can pass in the message manager here, and make `EventDispatcher.for` detect if the argument is a window or a message manager.

Comment 5

[Eugen Sawin [:esawin]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #3)
> Comment on attachment 8892970 [details] [diff] [review]
> 0002-Bug-1386696-2.0-Ensure-content-event-forwarding-is-h.patch
> 
> Review of attachment 8892970 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/geckoview/Messaging.jsm
> @@ +254,4 @@
> >        return;
> >      }
> >  
> > +    let win = aMsg.target.ownerGlobal || aMsg.target.contentWindow || {};
> 
> What's this change for?

I think we want the ChromeWindow in this case, because that will hold the ChromeMessageBroadcaster.

Comment 6

[Eugen Sawin [:esawin]]

(In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> Comment on attachment 8892966 [details] [diff] [review]
> 0001-Bug-1386696-1.0-Provide-event-dispatcher-to-GeckoVie.patch
> 
> Review of attachment 8892966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mobile/android/modules/geckoview/GeckoViewContentModule.jsm
> @@ +24,4 @@
> >    constructor(aModuleName, aMessageManager) {
> >      this.moduleName = aModuleName;
> >      this.messageManager = aMessageManager;
> > +    this.eventDispatcher = EventDispatcher.for(this);
> 
> We should only pass a window to `EventDispatcher.for`, and `this` here is
> not a window. I think you can pass in the message manager here, and make
> `EventDispatcher.for` detect if the argument is a window or a message
> manager.

To improve readability I would prefer to do it explicitly and pass two arguments to EventDispatcher.for(aWindow, aMessageManager) instead, where either can be null.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8892970 [details] [diff] [review]
0002-Bug-1386696-2.0-Ensure-content-event-forwarding-is-h.patch

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

(In reply to Eugen Sawin [:esawin] from comment #5)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #3)
> > Comment on attachment 8892970 [details] [diff] [review]
> > 0002-Bug-1386696-2.0-Ensure-content-event-forwarding-is-h.patch
> > 
> > Review of attachment 8892970 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: mobile/android/modules/geckoview/Messaging.jsm
> > @@ +254,4 @@
> > >        return;
> > >      }
> > >  
> > > +    let win = aMsg.target.ownerGlobal || aMsg.target.contentWindow || {};
> > 
> > What's this change for?
> 
> I think we want the ChromeWindow in this case, because that will hold the
> ChromeMessageBroadcaster.

I think you just want `let win = aMsg.target.ownerGlobal;` in this case (don't need ` || aMsg.target.contentWindow || {}`).

Comment 8

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Eugen Sawin [:esawin] from comment #6)
> (In reply to Jim Chen [:jchen] [:darchons] from comment #4)
> > Comment on attachment 8892966 [details] [diff] [review]
> > 0001-Bug-1386696-1.0-Provide-event-dispatcher-to-GeckoVie.patch
> > 
> > We should only pass a window to `EventDispatcher.for`, and `this` here is
> > not a window. I think you can pass in the message manager here, and make
> > `EventDispatcher.for` detect if the argument is a window or a message
> > manager.
> 
> To improve readability I would prefer to do it explicitly and pass two
> arguments to EventDispatcher.for(aWindow, aMessageManager) instead, where
> either can be null.

Sounds good!

Comment 9

[Eugen Sawin [:esawin]]

Attachment: 0002-Bug-1386696-2.1-Ensure-content-event-forwarding-is-h.patch

Addressed comments.

Comment 10

[Eugen Sawin [:esawin]]

Attachment: 0003-Bug-1386696-3.0-Add-explicit-way-to-construct-an-eve.patch

I think this might be even clearer than passing around null arguments for windows.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/21585c4acb10
[1.0] Provide event dispatcher to GeckoView content modules to allow for event forwarding. r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0d041387e4
[2.1] Ensure content event forwarding is handling e10s and non-e10s cases correctly. r=jchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5633839c80f
[3.0] Add explicit way to construct an event dispatcher based on a message manager. r=jchen

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/21585c4acb10
https://hg.mozilla.org/mozilla-central/rev/bd0d041387e4
https://hg.mozilla.org/mozilla-central/rev/c5633839c80f

Comment 13

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/21585c4acb1096722bc51883486ccc110efac60d
Bug 1386696 - [1.0] Provide event dispatcher to GeckoView content modules to allow for event forwarding. r=jchen

https://hg.mozilla.org/projects/date/rev/bd0d041387e43857f7d8d155479776c9cd6eb732
Bug 1386696 - [2.1] Ensure content event forwarding is handling e10s and non-e10s cases correctly. r=jchen

https://hg.mozilla.org/projects/date/rev/c5633839c80fab59b9af031f22b9cdaa0f4b9954
Bug 1386696 - [3.0] Add explicit way to construct an event dispatcher based on a message manager. r=jchen