1045103 - add support for setWindowSize

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Malini Das [:mdas] - Away, not checking bugmail]

This bug is to track the implementation of https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#resizing-and-positioning-windows

Comment 1

[Malini Das [:mdas] - Away, not checking bugmail]

Youu should be able to use https://developer.mozilla.org/en-US/docs/Web/API/window.resizeTo.

This call is context-agnostic, meaning it doesn't matter if you're in "chrome" or "content" mode, only marionette-server.js will call resizeTo in the current browser window, so you won't need to redirect the call to any marionette-listener.

Comment 2

[Robert Wood [:rwood]]

Attachment: bug1045103.patch

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 8481359 [details] [diff] [review]
bug1045103.patch

Review of attachment 8481359 [details] [diff] [review]:
-----------------------------------------------------------------

Cool, one more thing: test_set_window_size.py also has to be added to our manifest: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/unit-tests.ini

This is looking good!

::: testing/marionette/client/marionette/tests/unit/test_set_window_size.py
@@ +18,5 @@
> +        self.marionette.set_window_size(self.start_size['width'], self.start_size['height'])
> +        super(MarionetteTestCase, self).tearDown()
> +
> +    def test_set_window_size(self):
> +        max_width = self.marionette.execute_script("return window.screen.availWidth;") 

extra whitespace at end

@@ +24,5 @@
> +        # valid size
> +        width = max_width - 100
> +        height = max_height - 100
> +        self.marionette.set_window_size(width, height)
> +        sleep(1)

sleeps are really flaky on tbpl, so instead, we should try and see if any events are fired when windows are resized, and wait for that to fire. It looks like we can use https://developer.mozilla.org/en-US/docs/Web/API/Window.onresize.

@@ +26,5 @@
> +        height = max_height - 100
> +        self.marionette.set_window_size(width, height)
> +        sleep(1)
> +        size = self.marionette.window_size
> +        self.assertEqual(size['width'], width, "Window width should match what was set")

can you change this to "Window width should be: %s" % width, and same for height? When we get this error message, it's easier to know which value it was supposed to be.

@@ +33,5 @@
> +        # invalid width
> +        rcvd_error = False
> +        try:
> +          self.marionette.set_window_size(max_width + 100, height)
> +        except MarionetteException as e:

here's a cool trick: https://mxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/tests/unit/test_profile_management.py#31 which makes this kind of control flow a lot less verbose. You can use it on the line 46 block as well.

::: testing/marionette/marionette-server.js
@@ +2371,5 @@
> +   */
> +  setWindowSize: function MDA_setWindowSize(aRequest) {
> +    this.command_id = this.getCommandId();
> +    let width = parseInt(aRequest.parameters.width);
> +    let height = parseInt(aRequest.parameters.height);

if parseInt is called on something that can't be parsed into an int, it will throw an error and will stop the server from running. We don't want to just rely on client side input validation since clients may not even do validation, so we must verify that the client sends us correct parameters.

What we want to do here is to try to parse it in a try/catch and if it fails, then we send an error back to the client telling them that these values must be ints

Comment 4

[Robert Wood [:rwood]]

Attachment: bug1045103-take2.patch

Comment 5

[Robert Wood [:rwood]]

Comment on attachment 8482895 [details] [diff] [review]
bug1045103-take2.patch

Missed the last review comment, ignore this patch please

Comment 6

[Robert Wood [:rwood]]

Attachment: bug1045103-take3.patch

Comment 7

[Jonathan Griffin (:jgriffin)]

This patch looks good.  Pushing to try: https://tbpl.mozilla.org/?tree=Try&rev=2e3ed4f5a525

Comment 8

[Robert Wood [:rwood]]

Thanks Jonathan. Ah, the unit test fails on the b2g emulator, I will look into it.

Comment 9

[Jonathan Griffin (:jgriffin)]

I suspect we might not be able to resize the windows on B2G devices, including the emulator, so we might need to check whether we're in such an environment and return an error in that case.

Comment 10

[Robert Wood [:rwood]]

I was wondering the same but tried it out and the window resize does actually work on the emulator (didn't try on device though). However question is should it be supported?

Why it is failing on the emulator is because in the webdriver spec (link in description above) it says the 'unsupported operation' error must be returned if the resize will result in the window being maximized. On b2g the window is maximized at the start of this new set_window_size unit test; so in teardown the test attempts to set the window size back, which results in the error.

One thing is if the starting size is max then in teardown the test could set the size back to starting size -1 pixel to avoid the error; however this is probably not a good idea as it could effect following tests... Any suggestions? Or should set_window_size even be supported on b2g in the first place?

:AutomatedTester: I just realized my logic in setWindowSize for checking if the requested new window size will result in a maximized state is wrong; instead of checking width and height individually, would maximized actually be both width and height are >= the max width/height accordingly?

Comment 11

[Jonathan Griffin (:jgriffin)]

I think resizing the window isn't a use case that really makes sense on B2G, and we could just elect not to support it there.  Let's see what David says.

Comment 13

[David Burns :automatedtester]

(In reply to Robert Wood [:rwood] from comment #10)
> I was wondering the same but tried it out and the window resize does
> actually work on the emulator (didn't try on device though). However
> question is should it be supported?
> 
> Why it is failing on the emulator is because in the webdriver spec (link in
> description above) it says the 'unsupported operation' error must be
> returned if the resize will result in the window being maximized. On b2g the
> window is maximized at the start of this new set_window_size unit test; so
> in teardown the test attempts to set the window size back, which results in
> the error.
> 
> One thing is if the starting size is max then in teardown the test could set
> the size back to starting size -1 pixel to avoid the error; however this is
> probably not a good idea as it could effect following tests... Any
> suggestions? Or should set_window_size even be supported on b2g in the first
> place?

I don't think it has any place in B2G. If you are wanting to manipulate the emulator that should not be in marionette but in the emulator code (isn't fun we have a monolithic library that marionette means automation framework, test runner and emulator manipulator :))

> 
> :AutomatedTester: I just realized my logic in setWindowSize for checking if
> the requested new window size will result in a maximized state is wrong;
> instead of checking width and height individually, would maximized actually
> be both width and height are >= the max width/height accordingly?

I think that maximised will be the same as window.screen.avail* so have your test act accordingly really. The thing to remember is that setting the size and that action happening is going to be asynchronous so you'll need to have a poll in your test probably

Comment 14

[Robert Wood [:rwood]]

Attachment: bug1045103-take4.patch

Comment 15

[Jonathan Griffin (:jgriffin)]

Comment on attachment 8485414 [details] [diff] [review]
bug1045103-take4.patch

Review of attachment 8485414 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good except you'll need to add b2g = false for the test in unit-tests.ini, I believe.

Comment 16

[Robert Wood [:rwood]]

Attachment: bug1045103-take5.patch

Updated patch with addition of 'b2g = false' in the manifest, carrying fw the R+

Comment 17

[Robert Wood [:rwood]]

https://tbpl.mozilla.org/?tree=Try&rev=a95d743a9516

Comment 18

[Robert Wood [:rwood]]

Pushed inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/e2d358265126

Comment 19

[David Burns :automatedtester]

Comment on attachment 8485414 [details] [diff] [review]
bug1045103-take4.patch

Review of attachment 8485414 [details] [diff] [review]:
-----------------------------------------------------------------

I was originally set to review and had started it. Please can you raise a bug for us to fix the following issues

::: testing/marionette/client/marionette/tests/unit/test_set_window_size.py
@@ +21,5 @@
> +            self.start_size['width']-=1
> +        self.marionette.set_window_size(self.start_size['width'], self.start_size['height'])
> +        super(MarionetteTestCase, self).tearDown()
> +
> +    def test_set_window_size(self):

nit: can we have a slightly more descriptive name for the test. It doesn't suggest if it is a positive or negative test.

@@ +41,5 @@
> +                         "Window width is %s but should be %s" % (size['width'], width))
> +        self.assertEqual(size['height'], height,
> +                         "Window height is %s but should be %s" % (size['height'], height))
> +
> +        # invalid size (cannot maximize)

this looks like it would be different test so we should split it

::: testing/marionette/client/marionette/tests/unit/unit-tests.ini
@@ +124,1 @@
>  

this should be a Firefox only test since we can't update the window size on mobile

::: testing/marionette/marionette-server.js
@@ +2381,5 @@
> +   */
> +  setWindowSize: function MDA_setWindowSize(aRequest) {
> +    this.command_id = this.getCommandId();
> +
> +    if (appName == "B2G") {

It would be better if this was (appName !== "Firefox") that way we don't need to special case for fennec when we start supporting it

Comment 20

[Robert Wood [:rwood]]

Ugh, sorry :automatedtester, created Bug 1064421

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e2d358265126