1357878 - Setting window size to window’s current position hangs Marionette

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Andreas Tolfsen ❲:ato❳]

As described in https://github.com/mozilla/geckodriver/issues/643.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1357878 - Prevent hang on setting window size to existing size;

When the window's size is being set to the window's existing size,
Marionette unconditionally listens for the DOM resize event.  When a
window is not resized, no such event fires.

This patch skips setting the window size when the window's current size
is the requested size.  This bypasses the problem of listening for an
event that never occurs.

It also combiens the window position- and size tests into a
test_window_rect.py test, since they share many of the same
characteristics.

Fixes: https://github.com/mozilla/geckodriver/issues/643

Review commit: https://reviewboard.mozilla.org/r/131718/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/131718/

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

https://reviewboard.mozilla.org/r/131718/#review134666

::: commit-message-a3491:11
(Diff revision 1)
> +
> +This patch skips setting the window size when the window's current size
> +is the requested size.  This bypasses the problem of listening for an
> +event that never occurs.
> +
> +It also combiens the window position- and size tests into a

combines

::: commit-message-a3491:11
(Diff revision 1)
> +
> +This patch skips setting the window size when the window's current size
> +is the requested size.  This bypasses the problem of listening for an
> +event that never occurs.
> +
> +It also combiens the window position- and size tests into a

Since some test methods are being removed and since that isn't clear from the diff, could you document which tests (and why) in the commit message?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:10
(Diff revision 1)
> +from marionette_driver.errors import InvalidArgumentException
> +
> +from marionette_harness import MarionetteTestCase
> +
> +
> +class TestPosition(MarionetteTestCase):

Update the test manifest(s) with the added/removed test files.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:127
(Diff revision 1)
> +        if start_size["width"] == self.max_width and start_size["height"] == self.max_height:
> +            self.start_size["width"] -= 1
> +            self.start_size["height"] -= 1
> +        self.marionette.set_window_size(start_size["width"], start_size["height"])
> +
> +        self.original_size = self.marionette.window_size

`original_size` isn't used. Did you forget a tearDown?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:162
(Diff revision 1)
> +        self.assertNotEqual(old["width"], new["width"])
> +        self.assertNotEqual(old["height"], new["height"])

I realize this test is just being moved, but while we're here: I think it would be better to check equality. We expect that new width is old width + 10. Same for test_move_to_new_position.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

https://reviewboard.mozilla.org/r/131718/#review134666

> Since some test methods are being removed and since that isn't clear from the diff, could you document which tests (and why) in the commit message?

Examining the files I removed closer, I discovered that I had skipped a few relevant tests.  I’ve now taken those and worked them into the new test_window_rect.py.

I also discovered that the Maximize Window command was not synchronous, so I’ve submitted a further patch to fix that.

> Update the test manifest(s) with the added/removed test files.

Good catch.  I always forget to do this.

> `original_size` isn't used. Did you forget a tearDown?

Yes I did.

> I realize this test is just being moved, but while we're here: I think it would be better to check equality. We expect that new width is old width + 10. Same for test_move_to_new_position.

That should be easy enough to fix while I’m at it.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1357878 - Maximize window synchronously and restore when maximized;

When refactoring the tests for the Set Window Rect command, it was
discovered that the Maximize Window command was not synchronous.

This patch makes GeckoDriver#maximizeWindow synchronous by waiting for
the DOM resize event to fire before returning the window rect to the user.

It also aligns the command with the WebDriver standard by making it
restore the window to its original size when calling the command a
second time.

Review commit: https://reviewboard.mozilla.org/r/132100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132100/

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131718/diff/1-2/

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

https://reviewboard.mozilla.org/r/131718/#review135298

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131718/diff/2-3/

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8860086 [details]
Bug 1357878 - Maximize window synchronously and restore when maximized;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132100/diff/1-2/

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8860086 [details]
Bug 1357878 - Maximize window synchronously and restore when maximized;

https://reviewboard.mozilla.org/r/132100/#review135296

(bah, I think I forgot to publish this a few days ago.) This looks good, but see try: the maximized size on Windows is bigger than expected in the test.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py:43
(Diff revision 1)
> +                "current width {} and max width {}"
> +                .format(delta, actual["width"], self.max["width"]))
> +        self.assertAlmostEqual(
> +            actual["height"], self.max["height"],
> +            delta=8,
> +            msg="Window height i snot within {} px of availHeight, "

snot

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py:66
(Diff revision 1)
> +    def test_maximize(self):
> +        rect = self.marionette.maximize_window()
> +        self.assert_window_rect(rect)
> +        size = self.marionette.window_size
> +        self.assertEqual(size, rect)
> +        self.assert_window_maximized(size)

failing on windows

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8860086 [details]
Bug 1357878 - Maximize window synchronously and restore when maximized;

https://reviewboard.mozilla.org/r/132100/#review135296

> snot

Describes aptly how I feel about this.

> failing on windows

I fixed the tests on Windows before you published this review, but now they are failing on macOS.  I probably have to adjust the platform specific delta somewhat.  I think it’s better to be as specific as we can for each platform, than to be overly generic with the delta and potentially let through false positives.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131718/diff/3-4/

Comment 12

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8860086 [details]
Bug 1357878 - Maximize window synchronously and restore when maximized;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132100/diff/2-3/

Comment 13

[Andreas Tolfsen ❲:ato❳]

try looks green now in latest push.

Comment 14

[Andreas Tolfsen ❲:ato❳]

(https://treeherder.mozilla.org/#/jobs?repo=try&revision=eda8fd2b1788&group_state=expanded)

Comment 15

[Mozilla Autoland]

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 e3c9648bd53b -d 9c090ad2183e: rebasing 392047:e3c9648bd53b "Bug 1357878 - Prevent hang on setting window size to existing size; r=maja_zf"
merging testing/marionette/driver.js
warning: conflicts while merging testing/marionette/driver.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 16

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/131718/diff/4-5/

Comment 17

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8860086 [details]
Bug 1357878 - Maximize window synchronously and restore when maximized;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132100/diff/3-4/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8c95c46f521f
Prevent hang on setting window size to existing size; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/42efa1423067
Maximize window synchronously and restore when maximized; r=maja_zf

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8c95c46f521f
https://hg.mozilla.org/mozilla-central/rev/42efa1423067