Implement getting/setting window rect endpoints

RESOLVED FIXED in Firefox 53

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: automatedtester, Assigned: automatedtester)

Tracking

(Blocks 1 bug)

Version 3
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

()

Attachments

(2 attachments)

Assignee

Description

2 years ago
We need to deprecate set window position and implement Set Window Rect.
Blocks: webdriver
Summary: Implement SetWindowRect endpoint → Implement getting/setting window rect endpoints
Assignee

Updated

2 years ago
Assignee: nobody → dburns
Assignee

Updated

2 years ago
Blocks: 1348145
Assignee

Comment 1

2 years ago
leaving set/get Window Position/Size commands to be removed at a later stage.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 6

2 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.
Attachment #8848566 - Flags: review?(ato) → review-

Comment 7

2 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.
Attachment #8848567 - Flags: review?(ato) → review+
Assignee

Comment 8

2 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.
Assignee

Comment 9

2 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`
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
Comment on attachment 8848566 [details]
Bug 1347589: Implementation of Marionette Set Window Rect.

https://reviewboard.mozilla.org/r/121116/#review126242
Attachment #8848566 - Flags: review?(ato) → review+

Comment 14

2 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.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

2 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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d367390a0129
https://hg.mozilla.org/mozilla-central/rev/ffe3412eba40
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Assignee

Updated

2 years ago
Whiteboard: [checkin-needed-beta][checkin-needed-aurora][checkin-needed-esr52]

Comment 19

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e02e832e948d
https://hg.mozilla.org/releases/mozilla-aurora/rev/678507a09317
Whiteboard: [checkin-needed-beta][checkin-needed-aurora][checkin-needed-esr52] → [checkin-needed-beta][checkin-needed-esr52]
Needs rebasing (or some other patch approvals) for Beta/ESR52 uplift.
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]
Assignee

Updated

2 years ago
Flags: needinfo?(dburns)
Whiteboard: [checkin-needed-beta][checkin-needed-esr52]

Comment 21

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/93078ef18399
https://hg.mozilla.org/releases/mozilla-beta/rev/d55842d308ea
Flags: in-testsuite+
Whiteboard: [checkin-needed-beta][checkin-needed-esr52] → [checkin-needed-esr52]
Whiteboard: [checkin-needed-esr52]
You need to log in before you can comment on or make changes to this bug.