Fix content event forwarding

RESOLVED FIXED in Firefox 57

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: esawin, Assigned: esawin)

Tracking

51 Branch
Firefox 57
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

a year ago
Fix content event forwarding, as introduced in bug 1322586.
(Assignee)

Updated

a year ago
Blocks: 1386697
(Assignee)

Comment 1

a year ago
Created attachment 8892966 [details] [diff] [review]
0001-Bug-1386696-1.0-Provide-event-dispatcher-to-GeckoVie.patch
Attachment #8892966 - Flags: review?(nchen)
(Assignee)

Comment 2

a year ago
Created attachment 8892970 [details] [diff] [review]
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.
Attachment #8892970 - Flags: review?(nchen)
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 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.
(Assignee)

Comment 5

a year ago
(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.
(Assignee)

Comment 6

a year ago
(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 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 || {}`).
Attachment #8892970 - Flags: review?(nchen) → review+
(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!
Attachment #8892966 - Flags: review?(nchen) → review+
(Assignee)

Comment 9

a year ago
Created attachment 8893082 [details] [diff] [review]
0002-Bug-1386696-2.1-Ensure-content-event-forwarding-is-h.patch

Addressed comments.
Attachment #8892970 - Attachment is obsolete: true
Attachment #8893082 - Flags: review+
(Assignee)

Comment 10

a year ago
Created attachment 8893083 [details] [diff] [review]
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.
Attachment #8893083 - Flags: review?(nchen)
Attachment #8893083 - Flags: review?(nchen) → review+

Comment 11

a year ago
Pushed by esawin@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/21585c4acb10
https://hg.mozilla.org/mozilla-central/rev/bd0d041387e4
https://hg.mozilla.org/mozilla-central/rev/c5633839c80f
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
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
You need to log in before you can comment on or make changes to this bug.