Closed
Bug 1386696
Opened 7 years ago
Closed 7 years ago
Fix content event forwarding
Categories
(GeckoView :: General, enhancement)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: esawin, Assigned: esawin)
References
Details
Attachments
(3 files, 1 obsolete file)
1.19 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
4.86 KB,
patch
|
esawin
:
review+
|
Details | Diff | Splinter Review |
1.85 KB,
patch
|
jchen
:
review+
|
Details | Diff | Splinter Review |
Fix content event forwarding, as introduced in bug 1322586.
Assignee | ||
Comment 1•7 years ago
|
||
Attachment #8892966 -
Flags: review?(nchen)
Assignee | ||
Comment 2•7 years ago
|
||
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 3•7 years ago
|
||
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•7 years ago
|
||
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•7 years 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•7 years 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 7•7 years ago
|
||
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+
Comment 8•7 years ago
|
||
(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!
Updated•7 years ago
|
Attachment #8892966 -
Flags: review?(nchen) → review+
Assignee | ||
Comment 9•7 years ago
|
||
Addressed comments.
Attachment #8892970 -
Attachment is obsolete: true
Attachment #8893082 -
Flags: review+
Assignee | ||
Comment 10•7 years ago
|
||
I think this might be even clearer than passing around null arguments for windows.
Attachment #8893083 -
Flags: review?(nchen)
Updated•7 years ago
|
Attachment #8893083 -
Flags: review?(nchen) → review+
Comment 11•7 years 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•7 years 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
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 13•7 years ago
|
||
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
Updated•5 years ago
|
Product: Firefox for Android → GeckoView
Updated•5 years ago
|
Target Milestone: Firefox 57 → mozilla57
You need to log in
before you can comment on or make changes to this bug.
Description
•