David Burns :automatedtester Assignee
	Description • 7 years ago

We need to deprecate set window position and implement Set Window Rect.

	David Burns :automatedtester Assignee
	Comment 1 • 7 years ago

leaving set/get Window Position/Size commands to be removed at a later stage.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8848566 [details]
Bug 1347589: Implementation of Marionette Set Window Rect.

https://reviewboard.mozilla.org/r/121116/#review125380

::: testing/marionette/client/marionette_driver/marionette.py:1429
(Diff revision 2)
>      def get_window_position(self):
>          """Get the current window's position.
>  
>          :returns: a dictionary with x and y
>          """
>          return self._send_message(
>              "getWindowPosition", key="value" if self.protocol == 1 else None)

Mark as deprecated.

::: testing/marionette/client/marionette_driver/marionette.py:1437
(Diff revision 2)
>      def set_window_position(self, x, y):
>          """Set the position of the current window
>  
>          :param x: x coordinate for the top left of the window
>          :param y: y coordinate for the top left of the window
>          """
>          self._send_message("setWindowPosition", {"x": x, "y": y})

Mark as deprecated.

::: testing/marionette/client/marionette_driver/marionette.py:1446
(Diff revision 2)
> +        """
> +            Set the position and size of the current window.
> +
> +            The supplied width and height values refer to the window outerWidth
> +            and outerHeight values, which include scroll bars, title bars, etc.
> +
> +            An error will be returned if the requested window size would result
> +            in the window being in the maximised state.
> +
> +            :param x: x coordinate for the top left of the window
> +            :param y: y coordinate for the top left of the window
> +            :param width: The width to resize the window to.
> +            :param height: The height to resize the window to.
> +        """

Incorrectly indented docstring.  There appears to be four leading spaces that are unnecessary.

::: testing/marionette/client/marionette_driver/marionette.py:1449
(Diff revision 2)
> +            The supplied width and height values refer to the window outerWidth
> +            and outerHeight values, which include scroll bars, title bars, etc.

Use double backticks for width and height so it is clear from the generated documentation that it refers to the function arguments, and single backticks for outerWidth and outerHeight so it they appear in a monospaced font.

::: testing/marionette/driver.js:1258
(Diff revision 2)
> -      reject();
> +        reject();
> -    }
> +      }
> -  });
> +    });
> +  }
> +
> +  if (height != null && width != null) {

This should come before the x/y code path.

::: testing/marionette/driver.js:1280
(Diff revision 2)
> +    'width': win.outerWidth,
> +    'height': win.outerHeight,

Use double quotes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_position.py:34
(Diff revision 2)
>              with self.assertRaises(InvalidArgumentException):
>                  self.marionette.set_window_position(x, y)
>  
> +    def test_setting_window_rect_with_nulls_errors(self):
> +        with self.assertRaises(InvalidArgumentException):
> +            self.marionette.set_window_rect()

Can you be specific with the kwargs in case they were to change in the future?

> self.marionette.set_window_rect(width=None, height=None, …)

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_position.py:42
(Diff revision 2)
> +        self.assertNotEqual(old_position["x"], new_position["x"])
> +        self.assertNotEqual(old_position["y"], new_position["y"])

Any reason you are not asserting that new_position is the wanted_position?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_position.py:47
(Diff revision 2)
> +        width = actual['width'] - 50
> +        height = actual['height'] - 50

Double quotes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_position.py:51
(Diff revision 2)
> +        self.assertEqual(size['width'], width,
> +                         "New width is {0} but should be {1}".format(size['width'], width))
> +        self.assertEqual(size['height'], height,
> +                         "New height is {0} but should be {1}".format(size['height'], height))

Please use double quotes consistently.

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 7 years ago
mozreview-review

Comment on attachment 8848567 [details]
Bug 1347589: Implement Marionette Get Window Rect.

https://reviewboard.mozilla.org/r/121472/#review125388

::: commit-message-7fc48:3
(Diff revision 2)
> +Brings the getWindowPosition and getWindowSize calls into
> +one call.

Please explain motivation for this change.  I.e. we are aligning Marionette with the WebDriver standard.

::: testing/marionette/client/marionette_driver/marionette.py:1429
(Diff revision 2)
>      def get_window_position(self):
>          """Get the current window's position.
>  
>          :returns: a dictionary with x and y
>          """
>          return self._send_message(
>              "getWindowPosition", key="value" if self.protocol == 1 else None)

Mark as deprecated.

::: testing/marionette/driver.js:1212
(Diff revision 2)
> - *     Object with |x| and |y| coordinates.
> + *     Object with |x|, |y| coordinates and |width| and |height|
> + *     of browser window.

The use of commas is odd here.  I suggest employing the Oxford comma instead:

> Dictionary with |x| and |y| coordinates, and |width| and |height| dimensions of the browser window.

	David Burns :automatedtester Assignee
	Comment 8 • 7 years ago
mozreview-review-reply

Comment on attachment 8848566 [details]
Bug 1347589: Implementation of Marionette Set Window Rect.

https://reviewboard.mozilla.org/r/121116/#review125380

> Any reason you are not asserting that new_position is the wanted_position?

We can't always guarantee that will will get that position so could lead to issues especially on automation.

	David Burns :automatedtester Assignee
	Comment 9 • 7 years ago
mozreview-review

Comment on attachment 8848567 [details]
Bug 1347589: Implement Marionette Get Window Rect.

https://reviewboard.mozilla.org/r/121472/#review125884

::: testing/marionette/driver.js:1212
(Diff revision 2)
> - *     Object with |x| and |y| coordinates.
> + *     Object with |x|, |y| coordinates and |width| and |height|
> + *     of browser window.

I have corrected the punctuation. Since JS doesnt have that concept of a dictionary, I have left it as `Object`

	Andreas Tolfsen ❲:ato❳
	Comment 13 • 7 years ago
mozreview-review

Comment on attachment 8848566 [details]
Bug 1347589: Implementation of Marionette Set Window Rect.

https://reviewboard.mozilla.org/r/121116/#review126242

	Andreas Tolfsen ❲:ato❳
	Comment 14 • 7 years ago
mozreview-review-reply

Comment on attachment 8848566 [details]
Bug 1347589: Implementation of Marionette Set Window Rect.

https://reviewboard.mozilla.org/r/121116/#review125380

> We can't always guarantee that will will get that position so could lead to issues especially on automation.

OK, fair enough.

	Pulsebot
	Comment 17 • 7 years ago

Pushed by dburns@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d367390a0129
Implementation of Marionette Set Window Rect. r=ato
https://hg.mozilla.org/integration/autoland/rev/ffe3412eba40
Implement Marionette Get Window Rect. r=ato

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/d367390a0129
https://hg.mozilla.org/mozilla-central/rev/ffe3412eba40

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/e02e832e948d
https://hg.mozilla.org/releases/mozilla-aurora/rev/678507a09317

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 7 years ago

Needs rebasing (or some other patch approvals) for Beta/ESR52 uplift.

	Ryan VanderMeulen [:RyanVM]
	Comment 21 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/93078ef18399
https://hg.mozilla.org/releases/mozilla-beta/rev/d55842d308ea