From 8b39a8e258fd4eb4c286c9044095a16c88333d8a Mon Sep 17 00:00:00 2001 From: Karl Tomlinson Date: Thu, 9 Jun 2022 02:32:17 +0000 Subject: [PATCH] Bug 1745595 wait for expected geometry after move or resize r=whimboo,webdriver-reviewers The requestAnimationFrame() callback used in IdlePromise() may run sooner than 1/60 second, providing insufficient time for changes to be effected. https://searchfox.org/mozilla-central/rev/e567185fa464270f94430e7cf62d134f4df9a69f/layout/base/nsRefreshDriver.cpp#1730-1731 Waiting for the "resize" and "MozUpdateWindowPos" events should provide minimum wait in the common cases that the OS completes the changes requested. This change should also resolve https://bugzilla.mozilla.org/show_bug.cgi?id=1702255 Differential Revision: https://phabricator.services.mozilla.com/D147729 --- remote/marionette/driver.js | 68 +++++++++++++++++++++++++++++++++++---------- remote/shared/Sync.jsm | 49 ++++++++++++++++++++++++++++---- 2 files changed, 96 insertions(+), 21 deletions(-) diff --git a/remote/marionette/driver.js b/remote/marionette/driver.js index ac11b274184f..2ddf2d84ed74 100644 --- a/remote/marionette/driver.js +++ b/remote/marionette/driver.js @@ -33,6 +33,7 @@ XPCOMUtils.defineLazyModuleGetters(lazy, { enableEventsActor: "chrome://remote/content/marionette/actors/MarionetteEventsParent.jsm", error: "chrome://remote/content/shared/webdriver/Errors.jsm", + EventPromise: "chrome://remote/content/shared/Sync.jsm", getMarionetteCommandsActorProxy: "chrome://remote/content/marionette/actors/MarionetteCommandsParent.jsm", IdlePromise: "chrome://remote/content/marionette/sync.js", @@ -1099,7 +1100,19 @@ GeckoDriver.prototype.setWindowRect = async function(cmd) { lazy.assert.open(this.getBrowsingContext({ top: true })); await this._handleUserPrompts(); - let { x, y, width, height } = cmd.parameters; + const { x = null, y = null, width = null, height = null } = cmd.parameters; + if (x !== null) { + lazy.assert.integer(x); + } + if (y !== null) { + lazy.assert.integer(y); + } + if (height !== null) { + lazy.assert.positiveInteger(height); + } + if (width !== null) { + lazy.assert.positiveInteger(width); + } const win = this.getCurrentWindow(); switch (lazy.WindowState.from(win.windowState)) { @@ -1113,23 +1126,48 @@ GeckoDriver.prototype.setWindowRect = async function(cmd) { break; } - if (width != null && height != null) { - lazy.assert.positiveInteger(height); - lazy.assert.positiveInteger(width); - - if (win.outerWidth != width || win.outerHeight != height) { + function geometryMatches() { + if ( + width !== null && + height !== null && + (win.outerWidth !== width || win.outerHeight !== height) + ) { + return false; + } + if (x !== null && y !== null && (win.screenX !== x || win.screenY !== y)) { + return false; + } + lazy.logger.trace(`Requested window geometry matches`); + return true; + } + + if (!geometryMatches()) { + // There might be more than one resize or MozUpdateWindowPos event due + // to previous geometry changes, such as from restoreWindow(), so + // wait longer if window geometry does not match. + const options = { checkFn: geometryMatches, timeout: 500 }; + const promises = []; + if (width !== null && height !== null) { + promises.push(new lazy.EventPromise(win, "resize", options)); win.resizeTo(width, height); - await new lazy.IdlePromise(win); } - } - - if (x != null && y != null) { - lazy.assert.integer(x); - lazy.assert.integer(y); - - if (win.screenX != x || win.screenY != y) { + if (x !== null && y !== null) { + promises.push( + new lazy.EventPromise(win.windowRoot, "MozUpdateWindowPos", options) + ); win.moveTo(x, y); - await new lazy.IdlePromise(win); + } + try { + await Promise.race(promises); + } catch (e) { + if (e instanceof lazy.error.TimeoutError) { + // The operating system might not honor the move or resize, in which + // case assume that geometry will have been adjusted "as close as + // possible" to that requested. There may be no event received if the + // geometry is already as close as possible. + } else { + throw e; + } } } diff --git a/remote/shared/Sync.jsm b/remote/shared/Sync.jsm index 7d49d4f46438..b2be7c8b7013 100644 --- a/remote/shared/Sync.jsm +++ b/remote/shared/Sync.jsm @@ -20,10 +20,11 @@ const { XPCOMUtils } = ChromeUtils.import( const lazy = {}; XPCOMUtils.defineLazyModuleGetters(lazy, { + error: "chrome://remote/content/shared/webdriver/Errors.jsm", Log: "chrome://remote/content/shared/Log.jsm", }); -const { TYPE_REPEATING_SLACK } = Ci.nsITimer; +const { TYPE_ONE_SHOT, TYPE_REPEATING_SLACK } = Ci.nsITimer; XPCOMUtils.defineLazyGetter(lazy, "logger", () => lazy.Log.get(lazy.Log.TYPES.REMOTE_AGENT) @@ -91,7 +92,7 @@ function Deferred() { /** * Wait for an event to be fired on a specified element. * - * The returned promise is guaranteed to not be called before the + * The returned promise is guaranteed to not resolve before the * next event tick after the event listener is called, so that all * other event listeners for the element are executed before the * handler is executed. For example: @@ -110,41 +111,61 @@ function Deferred() { * Indicates the event will be despatched to this subject, * before it bubbles down to any EventTarget beneath it in the * DOM tree. - * @param {function=} checkFn + * @param {function=} [null] options.checkFn * Called with the Event object as argument, should return true if the * event is the expected one, or false if it should be ignored and * listening should continue. If not specified, the first event with * the specified name resolves the returned promise. + * @param {number=} [null] options.timeout + * Timeout duration in milliseconds, if provided. + * If specified, then the returned promise will be rejected with + * TimeoutError, if not already resolved, after this duration has elapsed. + * If not specified, then no timeout is used. * @param {boolean=} [false] options.mozSystemGroup * Determines whether to add listener to the system group. * @param {boolean=} [false] options.wantUntrusted * Receive synthetic events despatched by web content. * - * @return {Promise.} + * @return {Promise} + * Either fulfilled with the first described event, satisfying + * options.checkFn if specified, or rejected with TimeoutError after + * options.timeout milliseconds if specified. * * @throws {TypeError} + * @throws {RangeError} */ function EventPromise(subject, eventName, options = {}) { const { capture = false, checkFn = null, + timeout = null, mozSystemGroup = false, wantUntrusted = false, } = options; - if ( !subject || !("addEventListener" in subject) || typeof eventName != "string" || typeof capture != "boolean" || (checkFn && typeof checkFn != "function") || + (timeout !== null && typeof timeout != "number") || typeof mozSystemGroup != "boolean" || typeof wantUntrusted != "boolean" ) { throw new TypeError(); } + if (timeout < 0) { + throw new RangeError(); + } return new Promise((resolve, reject) => { + let timer; + + function cleanUp() { + subject.removeEventListener(eventName, listener, capture); + timer?.cancel(); + } + function listener(event) { lazy.logger.trace(`Received DOM event ${event.type} for ${event.target}`); try { @@ -156,7 +177,7 @@ function EventPromise(subject, eventName, options = {}) { lazy.logger.warn(`Event check failed: ${e.message}`); } - subject.removeEventListener(eventName, listener, capture); + cleanUp(); executeSoon(() => resolve(event)); } @@ -165,6 +186,22 @@ function EventPromise(subject, eventName, options = {}) { mozSystemGroup, wantUntrusted, }); + + if (timeout !== null) { + timer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer); + timer.init( + () => { + cleanUp(); + reject( + new lazy.error.TimeoutError( + `EventPromise timed out after ${timeout} ms` + ) + ); + }, + timeout, + TYPE_ONE_SHOT + ); + } }); } -- 2.11.4.GIT