This might help with bug 1172660 and certainly obsoletes bug 1131818. The idea is to make it possible to have EventUtils.js loaded as a JS module (i.e. `Components.utils.import("EventUtils.js");`) After that we can import it into content-task.js and expose it to the content script that will be evaluated as a global. Example: ```js ContentTask.spawn(function* () { let el = content.document.getElementById("SomeElID"); EventUtils.syntesizeMouseAtCenter(el, {}, content.window/* will be optional */); // AND EventUtils.synthesizeTouchAtCenter(el, {}); // AND all the others. }); ``` This should make porting other tests that depend on these APIs a lot easier. I have a patch for this, will post it in a few.

Sounds good to me!

Comment on attachment 8711046 [details] [diff] [review] Patch v1: make all EventUtils methods globally available for use in ContentTasks Review of attachment 8711046 [details] [diff] [review]: ----------------------------------------------------------------- This looks okay to me, but I'm not 100% confident in my understanding of all of the places where EventUtils.js is used, so I don't have a high degree of certainty that the changes being made in that module make sense in all of those places. Also, why the changes to doc_frame_script.js, test_audio_wakelock.html and test_video_wakelock.html? ::: testing/mochitest/BrowserTestUtils/content/content-task.js @@ +10,5 @@ > Cu.import("resource://testing-common/ContentTaskUtils.jsm", this); > +this.EventUtils = {}; > +Cu.import("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", this.EventUtils); > +EventUtils.setGlobals(content.window, Components); > + Nit - extra newline. ::: testing/mochitest/tests/SimpleTest/EventUtils.js @@ +18,5 @@ > * synthesizeNativeClick > * > * When adding methods to this file, please add a performance test for it. > */ > +var loadAsJSM = String(this).indexOf("BackstagePass") > -1; According to the comment on line 37, EventUtils takes great care in ensuring it doesn't create naming collisions. Do we worry at all about things that imports EventUtils having things named "exports" for example? Perhaps "exports" should only ever be defined or exposed if this is loaded via a JSM. I suggest smaug looks at the change to this file to make sure EventUtils is still behaving by the rules it needs to follow with this change.

Comment on attachment 8711046 [details] [diff] [review] Patch v1: make all EventUtils methods globally available for use in ContentTasks Hey smaug, If you have a second (and if you don't - got any other reviewer suggestions?), are the changes to EventUtils.js kosher with how EventUtils.js is expected to work in multiple types of environments? I'm particularly worried about the globals that are being defined here - but I'm not sure if we care or not.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #6) > Also, why the changes to doc_frame_script.js, This devtools test suite is trying to the exact same thing as the new `setGlobals(window, Components)` method does, only in a more hacky way, since it imports the library with `loadSubScript`. I suspect it'll suffer from "Can't access dead object" intermittents, just like bug 1238065. > test_audio_wakelock.html and > test_video_wakelock.html? The global variable changes I made here probably triggered some kind of initialization code that made 'mozPower' to be initialized to `null|undefined`. I have no clue as to _why_, but seeing that no EventUtils are needed for those tests at all, I removed the script elements. Good riddance, prolly makes them more efficient too. > According to the comment on line 37, EventUtils takes great care in ensuring > it doesn't create naming collisions. Do we worry at all about things that > imports EventUtils having things named "exports" for example? > > Perhaps "exports" should only ever be defined or exposed if this is loaded > via a JSM. I agree and have thought about naming these variables similar to _EU_Ci and friends... so they would become _EU_window, _EU_navigator, etc. I'm interested in smaug's opinion too! My try pushes - up until now - have shown that these changes are not causing issues. Yet. > I suggest smaug looks at the change to this file to make sure EventUtils is > still behaving by the rules it needs to follow with this change. Great idea!

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #7) > Comment on attachment 8711046 [details] [diff] [review] > Patch v1: make all EventUtils methods globally available for use in > ContentTasks > > Hey smaug, > > If you have a second (and if you don't - got any other reviewer > suggestions?), are the changes to EventUtils.js kosher with how > EventUtils.js is expected to work in multiple types of environments? I'm > particularly worried about the globals that are being defined here - but I'm > not sure if we care or not. So in general fine, but this breaks many functions in non-window contexts. +var gWindow = typeof window != "undefined" ? window : null; +var gNavigator = typeof navigator != "undefined" ? navigator : null; +var gKeyboardEvent = typeof KeyboardEvent != "undefined" ? KeyboardEvent : null; all that feels odd. I would hide those behind some getter which returns either relevant object from the current global, or if we're not in window context, using content.<propertyname>

Though, I don't know what +var exports = { + setGlobals is doing. Feels like all that should happen automatically.

Comment on attachment 8711046 [details] [diff] [review] Patch v1: make all EventUtils methods globally available for use in ContentTasks So either explain how the setup should work and why this doesn't default to some getters trying to access interfaces from 'content', or just fix the part I commented earlier. (I don't know why some of setGlobals use content.window. How is that different to plain content?)

(In reply to Olli Pettay [:smaug] from comment #11) > So either explain how the setup should work and why this doesn't default > to some getters trying to access interfaces from 'content', or just > fix the part I commented earlier. I want to import the .js file as JSM and avoid using the scriptLoader. This means that `content` is not a global, since JSM scopes are isolated. TBH: I like the explicit passing of globals to a JSM, because it's super obvious and easy to reason about. Apart from that - but I can't support this statement with facts, just experience - occasionally my test runs fail with a message "can't access dead object", as documented in bug 1238065. When I load EventUtils.js as a JSM using this patch, it never happens again. Perhaps a bit irrational, but I'd like to refrain from mixing too many script loading mechanisms. > (I don't know why some of setGlobals use content.window. How is that > different to plain content?) I'll refrain doing that from now on.

While in many cases this can be useful, we actually want to come up with a way for synthesizeMouse/synthesizeKey/etc to be called from the parent and have it redirect to the child and resolve when it has finished, as a real mouse click would do. Synthesizing the events in the child can cause subtle differences with how the child is focused. There are a couple of tests that fail due to this. Bug 1240052 covers this for mouse events. So I'm not convinced that encouraging this approach is a good idea. Note though that I don't know how to do the better way.

DOMWindowUtils has a way to request some events to be dispatched in the parent process. That is what we might want to use for certain cases. But there are also cases when one really doesn't want the outer world to disturb some test.

(In reply to Neil Deakin from comment #13) > While in many cases this can be useful, we actually want to come up with a > way for synthesizeMouse/synthesizeKey/etc to be called from the parent and > have it redirect to the child and resolve when it has finished, as a real > mouse click would do. If that's the case, we shouldn't want to make our own special IPC protocol that eventually reaches out to DOMWindowUtils in the frame script according to parameters as directed from the parent. Right? I mean, instead we should be able to hijack this native event flow in test-mode. > Synthesizing the events in the child can cause subtle differences with how > the child is focused. There are a couple of tests that fail due to this. But my sense of logic tells me that our tests ought to be simulating real-world-usage as much as possible, right? What I'm seeing in our tests adapted to use `ContentTask` is that for each and every action, event or simulation of any sort that needs to happen in the scope of the content window we do chrome -> message -> content -> eval -> message -> chrome -> etc. A lot of messaging and event loop spins to ensure asynchrous behavior of the messageManager, I gather. So what I can see is one `ContentTask.spawn` per unit test in which we test the world to be we expect it to be in one go (bug 1241930 should make this even easier) and nothing else. Just like how our user load a page, leave the URL bar and interact with the page to no end.

(In reply to Mike de Boer [:mikedeboer] from comment #15) s/synchrous/synchronous/ and s/user load/users load/ /facepalm.

(In reply to Mike de Boer [:mikedeboer] from comment #15) > (In reply to Neil Deakin from comment #13) > > While in many cases this can be useful, we actually want to come up with a > > way for synthesizeMouse/synthesizeKey/etc to be called from the parent and > > have it redirect to the child and resolve when it has finished, as a real > > mouse click would do. > > If that's the case, we shouldn't want to make our own special IPC protocol > that eventually reaches out to DOMWindowUtils in the frame script according > to parameters as directed from the parent. Right? > I mean, instead we should be able to hijack this native event flow in > test-mode. > If I understand correctly, there are some problems with attempting to synthesize native events like this. Particularly, I _think_ there can be problems with the OS preventing us from dispatching events if, for example, the window isn't focused correctly or something. I believe our synth event tools here are creating "gecko" events and dispatching those, which is cross-platform and (I believe) sidesteps this restriction. > But my sense of logic tells me that our tests ought to be simulating > real-world-usage as much as possible, right? > > What I'm seeing in our tests adapted to use `ContentTask` is that for each > and every action, event or simulation of any sort that needs to happen in > the scope of the content window we do chrome -> message -> content -> eval > -> message -> chrome -> etc. A lot of messaging and event loop spins to > ensure asynchrous behavior of the messageManager, I gather. > So what I can see is one `ContentTask.spawn` per unit test in which we test > the world to be we expect it to be in one go (bug 1241930 should make this > even easier) and nothing else. Just like how our user load a page, leave the > URL bar and interact with the page to no end. Thinking about it, dispatching the events from the parent might be more realistic, since the events from the user will be consumed once we return to the event loop, which the async stuff simulates. I'm being swayed here by Enn's argument. Are there any other advantages to putting EventUtils into ContentTask that haven't been mentioned?

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #17) > Thinking about it, dispatching the events from the parent might be more > realistic, since the events from the user will be consumed once we return to > the event loop, which the async stuff simulates. > > I'm being swayed here by Enn's argument. Me too... except that it saddens me quite a bit that we can't extend this clean code model of making `ContentTask` rule our universe and let everything in there happen without all the indirection that we see all over the place with BrowserTestUtils. I'm not convinced about the async stuff simulating the flow of actual events. I'd actually expect that keydown and mousedown events, for example, would be passed down synchronously, NOT asynchronously. APZ I get, but that's a different ballgame. Who could tell us a good story regarding this? I'm _very_ eager to learn! :-)

(In reply to Mike de Boer [:mikedeboer] from comment #18) > > I'm not convinced about the async stuff simulating the flow of actual > events. I'd actually expect that keydown and mousedown events, for example, > would be passed down synchronously, NOT asynchronously. APZ I get, but > that's a different ballgame. > Who could tell us a good story regarding this? I'm _very_ eager to learn! :-) Here's what I know: 1) With e10s, events are dispatched asynchronously across IPC, and therefore can be consumed at an arbitrarily later time in the content process 2) In order for UI events to be consumed, a process must return to its event loop. So, with (2) in mind, suppose the following: yield ContentTask.spawn(browser, null, function*() { content.doSyncThing(); content.doAnotherSyncThing(); EventUtils.synthesizeMouseEvent(...); content.doThirdSyncThing(); }); If you follow EventUtils.synthesizeMouseEvent down the rabbit hole, we eventually get here: https://dxr.mozilla.org/mozilla-central/rev/aa90f482e16db77cdb7dea84564ea1cbd8f7f6b3/dom/base/nsContentUtils.cpp#7913 Which, I believe (unless my mental model is off here) will call HandleEvent on the nsPresShell synchronously, so any event handlers will be called synchronously (assuming that test.events.async.enabled is false, which is the default). This violates (2) from above. I argue that this is not as realistic as dispatching the event from the parent process, since the content process _must_ return to the event loop in order to consume the IPC message to synthesize the mouse event. The above only works out assuming I have my mental model correct, but I'm about 78% confident on that.

(In reply to Mike Conley (:mconley) - Needinfo me! from comment #19) > The above only works out assuming I have my mental model correct, but I'm > about 78% confident on that. I LOL'ed when I read that ;) I'm going to keep teasing you with that, like 78% of the time. But the main thing here is that we can already get our things done with the tools at hand and the shared belief that adding the IPC overhead is simulating native events as close as we possibly can. Sounds like a WONTFIX to me! Thanks all, for chiming in.

I'm not entirely sure what is being talked about here. I'm not referring to synthesizing native events.

(In reply to Neil Deakin from comment #21) > I'm not entirely sure what is being talked about here. I'm not referring to > synthesizing native events. Yeah, the term 'native' is a bit overloaded here. We're talking more along the lines of 'triggering the event from the parent process is closest to simulating the way gecko events are dispatched we can get'.