Closed
Bug 1381876
Opened 7 years ago
Closed 7 years ago
Ensure Maximize Window command is synchronous
Categories
(Remote Protocol :: Marionette, enhancement)
Tracking
(firefox57 fixed)
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
(Blocks 1 open bug)
Details
Attachments
(12 files, 1 obsolete file)
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
automatedtester
:
review+
|
Details |
It has recently been discovered through various intermittents (1, 2) that the Maximize Window command in Marionette is not as synchronous as we first thought. This will also affect the Minimize Window command (3) once it lands. This bug is to identify and fix the cause of the fragility. 1. https://bugzilla.mozilla.org/show_bug.cgi?id=1380309 2. https://bugzilla.mozilla.org/show_bug.cgi?id=1380513 3. https://bugzilla.mozilla.org/show_bug.cgi?id=1378121
Assignee | ||
Updated•7 years ago
|
Comment 1•7 years ago
|
||
Could this be again because the `resize` event is getting fired with a high rate as described here? https://developer.mozilla.org/en-US/docs/Web/Events/resize#Examples
Comment 2•7 years ago
|
||
Ok, I think I found what we have to do here: https://developer.mozilla.org/en-US/docs/Web/Events/sizemodechange > Note: This event may fire several times or while resizing the window (for example, see bug 715867), so always check if the window state changed by inspecting window.windowState in the event.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 29•7 years ago
|
||
mozreview-review |
Comment on attachment 8892527 [details] Bug 1381876 - Return window rect from Context#rect consistently. https://reviewboard.mozilla.org/r/163514/#review168990
Attachment #8892527 -
Flags: review?(dburns) → review+
Comment 30•7 years ago
|
||
mozreview-review |
Comment on attachment 8892526 [details] Bug 1381876 - Rename Maximize Window test to match command name. https://reviewboard.mozilla.org/r/163512/#review168992
Attachment #8892526 -
Flags: review?(dburns) → review+
Comment 31•7 years ago
|
||
mozreview-review |
Comment on attachment 8892528 [details] Bug 1381876 - Include window state in window rect. https://reviewboard.mozilla.org/r/163516/#review168994
Attachment #8892528 -
Flags: review?(dburns) → review+
Comment 32•7 years ago
|
||
mozreview-review |
Comment on attachment 8892529 [details] Bug 1381876 - Fix window maximised assertions. https://reviewboard.mozilla.org/r/163518/#review169006
Attachment #8892529 -
Flags: review?(dburns) → review+
Comment 33•7 years ago
|
||
mozreview-review |
Comment on attachment 8892530 [details] Bug 1381876 - Remove unused import from test_window_maximize.py. https://reviewboard.mozilla.org/r/163520/#review169008
Attachment #8892530 -
Flags: review?(dburns) → review+
Comment 34•7 years ago
|
||
mozreview-review |
Comment on attachment 8892531 [details] Bug 1381876 - Compare window rect, not window size. https://reviewboard.mozilla.org/r/163522/#review169010
Attachment #8892531 -
Flags: review?(dburns) → review+
Comment 35•7 years ago
|
||
mozreview-review |
Comment on attachment 8892532 [details] Bug 1381876 - Allow assert_success to be used without an expected value. https://reviewboard.mozilla.org/r/163524/#review169012
Attachment #8892532 -
Flags: review?(dburns) → review+
Comment 36•7 years ago
|
||
mozreview-review |
Comment on attachment 8892533 [details] Bug 1381876 - Make client.Session#maximize a method. https://reviewboard.mozilla.org/r/163526/#review169016
Attachment #8892533 -
Flags: review?(dburns) → review+
Comment 37•7 years ago
|
||
mozreview-review |
Comment on attachment 8892534 [details] Bug 1381876 - Skip resetting namespaced interfaces in client.Session. https://reviewboard.mozilla.org/r/163528/#review169022
Attachment #8892534 -
Flags: review?(dburns) → review+
Comment 38•7 years ago
|
||
mozreview-review |
Comment on attachment 8892535 [details] Bug 1381876 - Correct WPT maximize window tests. https://reviewboard.mozilla.org/r/163530/#review169024 ::: testing/web-platform/tests/webdriver/tests/maximize_window.py:62 (Diff revision 2) > + # Determine the largest available window size by first maximising > + # the window and getting the window rect dimensions. > + # > + # Then resize the window to the maximum available size. > + session.end() > + available = session.window.maximize() How can you `end()` a session and still use it? This looks really weird
Attachment #8892535 -
Flags: review?(dburns) → review-
Comment 39•7 years ago
|
||
mozreview-review |
Comment on attachment 8892536 [details] Bug 1381876 - Rename WaitCondition to Condition. https://reviewboard.mozilla.org/r/163532/#review169026
Attachment #8892536 -
Flags: review?(dburns) → review+
Comment 40•7 years ago
|
||
mozreview-review |
Comment on attachment 8892537 [details] Bug 1381876 - Introduce TimedPromise. https://reviewboard.mozilla.org/r/163534/#review169030 ::: commit-message-a3ff1:7 (Diff revision 2) > + > +This introduces a specialisation of the well-known Promise that can > +time out after a set limit, causing the promises' "reject" callback to > +be invoked. > + > +The TimedPromise object represents the tiemd, eventual completion s/tiemd/timed/
Attachment #8892537 -
Flags: review?(dburns) → review+
Comment 41•7 years ago
|
||
mozreview-review |
Comment on attachment 8892538 [details] Bug 1381876 - Ensure Maximize Window is synchronous. https://reviewboard.mozilla.org/r/163536/#review169036
Attachment #8892538 -
Flags: review?(dburns) → review+
Assignee | ||
Comment 42•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8892535 [details] Bug 1381876 - Correct WPT maximize window tests. https://reviewboard.mozilla.org/r/163530/#review169024 > How can you `end()` a session and still use it? This looks really weird API methods on the WPT WebDriver test client are decorated with @command, which implicitly creates a session if one does not exist. So when session.end() ends the session, session.window.maximize()’s decorator creates a new one for us.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 56•7 years ago
|
||
mozreview-review |
Comment on attachment 8892535 [details] Bug 1381876 - Correct WPT maximize window tests. https://reviewboard.mozilla.org/r/163530/#review169356
Attachment #8892535 -
Flags: review?(dburns) → review+
Comment 57•7 years ago
|
||
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again. hg error in cmd: hg rebase -s 949bb36f8db1 -d f034b3a5d6a8: rebasing 411068:949bb36f8db1 "Bug 1381876 - Rename Maximize Window test to match command name. r=automatedtester" rebasing 411069:8bb14677a89c "Bug 1381876 - Return window rect from Context#rect consistently. r=automatedtester" rebasing 411070:427852655e74 "Bug 1381876 - Include window state in window rect. r=automatedtester" rebasing 411071:4f3711d64153 "Bug 1381876 - Fix window maximised assertions. r=automatedtester" rebasing 411072:83f0fac35b63 "Bug 1381876 - Remove unused import from test_window_maximize.py. r=automatedtester" rebasing 411073:61f05633511e "Bug 1381876 - Compare window rect, not window size. r=automatedtester" rebasing 411074:2b7a435fc36c "Bug 1381876 - Allow assert_success to be used without an expected value. r=automatedtester" rebasing 411075:c4c0c2b74582 "Bug 1381876 - Make client.Session#maximize a method. r=automatedtester" merging testing/web-platform/tests/tools/webdriver/webdriver/client.py rebasing 411076:244142f43561 "Bug 1381876 - Skip resetting namespaced interfaces in client.Session. r=automatedtester" merging testing/web-platform/tests/tools/webdriver/webdriver/client.py rebasing 411077:b5a95d7f9587 "Bug 1381876 - Correct WPT maximize window tests. r=automatedtester" merging testing/web-platform/meta/MANIFEST.json warning: conflicts while merging testing/web-platform/meta/MANIFEST.json! (edit, then use 'hg resolve --mark') unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8892535 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 92•7 years ago
|
||
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/86cb93e08b08 Rename Maximize Window test to match command name. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/92a8be47bbd7 Return window rect from Context#rect consistently. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/03c1eabbcbc6 Include window state in window rect. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/e194d3ec9c1a Fix window maximised assertions. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/f17acd4a5bbb Remove unused import from test_window_maximize.py. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/d72b664302bc Compare window rect, not window size. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/2e2a7ad88909 Allow assert_success to be used without an expected value. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/e013a69621be Make client.Session#maximize a method. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/ea49436af9d9 Skip resetting namespaced interfaces in client.Session. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/66ce6c8cd053 Rename WaitCondition to Condition. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/4f1c4c9fd451 Introduce TimedPromise. r=automatedtester https://hg.mozilla.org/integration/autoland/rev/ec8c6742603a Ensure Maximize Window is synchronous. r=automatedtester
Comment 93•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/86cb93e08b08 https://hg.mozilla.org/mozilla-central/rev/92a8be47bbd7 https://hg.mozilla.org/mozilla-central/rev/03c1eabbcbc6 https://hg.mozilla.org/mozilla-central/rev/e194d3ec9c1a https://hg.mozilla.org/mozilla-central/rev/f17acd4a5bbb https://hg.mozilla.org/mozilla-central/rev/d72b664302bc https://hg.mozilla.org/mozilla-central/rev/2e2a7ad88909 https://hg.mozilla.org/mozilla-central/rev/e013a69621be https://hg.mozilla.org/mozilla-central/rev/ea49436af9d9 https://hg.mozilla.org/mozilla-central/rev/66ce6c8cd053 https://hg.mozilla.org/mozilla-central/rev/4f1c4c9fd451 https://hg.mozilla.org/mozilla-central/rev/ec8c6742603a
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 94•7 years ago
|
||
mozreview-review |
Comment on attachment 8892537 [details] Bug 1381876 - Introduce TimedPromise. https://reviewboard.mozilla.org/r/163534/#review178558 ::: testing/marionette/wait.js:147 (Diff revision 7) > + * instead resolved on timeout. > + * > + * @return {Promise.<*>} > + * Timed promise. > + */ > +function TimedPromise(fn, {timeout = 1500, throws = TimeoutError} = {}) { Sorry to come back to this that late, but what is the reason to use a different timeout here compared to wait. It would have made more sense to really use the same duration.
Comment 95•7 years ago
|
||
mozreview-review |
Comment on attachment 8892538 [details] Bug 1381876 - Ensure Maximize Window is synchronous. https://reviewboard.mozilla.org/r/163536/#review178566 ::: testing/marionette/driver.js:2982 (Diff revision 7) > if (win.windowState == win.STATE_MAXIMIZED) { > win.restore(); > } else { > win.maximize(); > } > - }); > + }, {throws: null}); I feel there is a case when a slowly changing window state can fail... Or I read this wrong? Why do we ignore the reject case case here and don't do anything in case the TimedPromise times out? By default this is 1.5s, and especially for debug builds I could imaging that window size changes can take a while and exceed this duration. When will there be cases when we don't expect a `sizemodechange` to happen?
Assignee | ||
Comment 96•7 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #94) > ::: testing/marionette/wait.js:147 (Diff revision 7) > > > + * instead resolved on timeout. * * @return {Promise.<*>} * > > +Timed promise. */ function TimedPromise(fn, {timeout = 1500, > > +throws = TimeoutError} = {}) { > > Sorry to come back to this that late, but what is the reason to > use a different timeout here compared to wait. It would have made > more sense to really use the same duration. They are two distinct computing ideas. (In reply to Henrik Skupin (:whimboo) from comment #95) > ::: testing/marionette/driver.js:2982 > (Diff revision 7) > > if (win.windowState == win.STATE_MAXIMIZED) { > > win.restore(); > > } else { > > win.maximize(); > > } > > - }); > > + }, {throws: null}); > > I feel there is a case when a slowly changing window state can > fail... Or I read this wrong? > > Why do we ignore the reject case case here and don't do anything > in case the TimedPromise times out? By default this is 1.5s, and > especially for debug builds I could imaging that window size > changes can take a while and exceed this duration. > > When will there be cases when we don't expect a `sizemodechange` > to happen? This is already described in detail in the commit message, but we don’t expect a sizemodechange on Linux when the window manager doesn’t support the concept of window maximisation. In those cases we time out and hope we have given the window enough time to transition into whatever state the WM has commanded.
Updated•1 year ago
|
Product: Testing → Remote Protocol
You need to log in
before you can comment on or make changes to this bug.
Description
•