Closed Bug 1357878 Opened 6 years ago Closed 6 years ago

Setting window size to window’s current position hangs Marionette

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(2 files)

Bug 1357878 - Prevent hang on setting window size to existing size;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1357878 - Maximize window synchronously and restore when maximized;
59 bytes, text/x-review-board-request
impossibus
: review+
Details
Assignee: nobody → ato
Status: NEW → ASSIGNED
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.
Attachment #8859718 - Flags: review?(mjzffr) → review-
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 on attachment 8859718 [details]
Bug 1357878 - Prevent hang on setting window size to existing size;

https://reviewboard.mozilla.org/r/131718/#review135298
Attachment #8859718 - Flags: review?(mjzffr) → review+
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
Attachment #8860086 - Flags: review?(mjzffr) → review+
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.
try looks green now in latest push.
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)
Pushed by atolfsen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/8c95c46f521f
https://hg.mozilla.org/mozilla-central/rev/42efa1423067
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.