Move out action initializing code from listener.js
Categories
(Remote Protocol :: Marionette, task, P2)
Tracking
(firefox85 fixed)
Tracking | Status | |
---|---|---|
firefox85 | --- | fixed |
People
(Reporter: whimboo, Assigned: impossibus)
References
Details
(Keywords: memory-footprint, Whiteboard: [marionette-fission-reserve])
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
There the the following initialization code for actions in listener.js:
action.inputStateMap = new Map();
action.inputsToCancel = [];
It clearly needs a new place to run once we always use the JSWindowActors and are not running the listener.js framescript code.
Assignee | ||
Comment 1•4 years ago
|
||
In response to https://phabricator.services.mozilla.com/D90068?id=347885#inline-522328
I wonder why we initialize it in listener.js and not directly in actions.js. @maja_zf can you remember?
No, I don't remember, sorry. But I just ran the wdspec tests with the initialization in action.js and the tests pass.
--- a/testing/marionette/action.js
+++ b/testing/marionette/action.js
@@ -405,26 +405,26 @@ action.PointerType.get = function(str) {
/**
* Input state associated with current session. This is a map between
* input ID and the device state for that input source, with one entry
* for each active input source.
*
* Initialized in listener.js.
*/
-action.inputStateMap = undefined;
+action.inputStateMap = new Map();
/**
* List of {@link action.Action} associated with current session. Used to
* manage dispatching events when resetting the state of the input sources.
* Reset operations are assumed to be idempotent.
*
* Initialized in listener.js
*/
-action.inputsToCancel = undefined;
+action.inputsToCancel = [];
/**
* Represents device state for an input source.
*/
class InputState {
constructor() {
this.type = this.constructor.name.toLowerCase();
}
--- a/testing/marionette/listener.js
+++ b/testing/marionette/listener.js
@@ -1352,18 +1352,18 @@ const eventDispatcher = {
*/
function registerSelf() {
logger.trace("Frame script loaded");
curContainer.frame = content;
sandboxes.clear();
legacyactions.mouseEventsOnly = false;
- action.inputStateMap = new Map();
- action.inputsToCancel = [];
Maybe something has changed in the past 4 years about the timing of module load?
Reporter | ||
Comment 2•4 years ago
|
||
Yes, that is the patch that I also used. See:
https://phabricator.services.mozilla.com/D92372
But using that web-platform tests are busted:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0b786438087b9f6b77defcdba2be9eddf48f42b
InvalidArgumentException: Expected 3 to be mapped to [object NullInputState], got [object PointerInputState]
That means we actually do not reset the state, and that most likely happens for a remoteness change.
Reporter | ||
Comment 4•4 years ago
|
||
Maja, is that actually still needed? Given your changes from bug 1365886 we have that code in action.js
now, and as such for the framescript fallback case, we won't have to anything. It could remain until we remove the whole JS module.
Assignee | ||
Comment 5•4 years ago
|
||
We still have inputStateMap
initialization code in listener.js, and I think removing it breaks wpt with actors enabled. (I'll make a new try push when I get the chance.)
Reporter | ||
Comment 6•4 years ago
|
||
Right but given that we also initialize in action.js for the actor architecture, there doesn't seem to be any remaining work here. When we stop loading the framescript I assume it still just works?
Assignee | ||
Comment 7•4 years ago
|
||
Yeah, but when I was testing with AWSY with spec-compliant actions and JSWindowActors, I needed to add initialization to action.js and keep the initialization in listener registration for it to work.
Anyway, here's are try pushes with the listener init removed:
Marionette, wpt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d7e0a1cd694ba32e8b65746d9d35702f812f668
AWSY: https://treeherder.mozilla.org/#/jobs?repo=try&revision=10fd0f80138818535862327f2721f5e014586329
Reporter | ||
Comment 8•4 years ago
|
||
Thanks. As it looks like wpt tests are still failing. So we need a fix for that to be able to stop loading the framescript on bug 1669174.
Assignee | ||
Updated•4 years ago
|
Assignee | ||
Comment 9•4 years ago
|
||
Comment 10•4 years ago
|
||
Pushed by mjzffr@gmail.com: https://hg.mozilla.org/integration/autoland/rev/edcfe4ed963d Move out action initializing code from listener.js r=marionette-reviewers,whimboo
Comment 11•4 years ago
|
||
bugherder |
Reporter | ||
Updated•4 years ago
|
Comment 12•4 years ago
|
||
== Change summary for alert #28091 (as of Tue, 15 Dec 2020 13:23:42 GMT) ==
Improvements:
Ratio | Suite | Test | Platform | Options | Absolute values (old vs new) |
---|---|---|---|---|---|
3% | Base Content JS | macosx1014-64-shippable-qr | 2,765,997.67 -> 2,686,913.33 | ||
3% | Base Content JS | linux1804-64-shippable | 2,761,541.67 -> 2,684,212.00 | ||
3% | Base Content JS | linux1804-64-shippable-qr | 2,761,143.33 -> 2,684,054.67 | ||
3% | Base Content JS | windows10-64-shippable-qr | 2,765,646.67 -> 2,688,570.67 | ||
3% | Base Content JS | windows10-64-shippable | 2,765,468.00 -> 2,688,514.67 | ||
3% | Base Content JS | windows10-64-shippable-qr | 2,766,894.67 -> 2,691,896.00 |
For up to date results, see: https://treeherder.mozilla.org/perfherder/alerts?id=28091
Reporter | ||
Updated•4 years ago
|
Updated•2 years ago
|
Description
•