Closed Bug 1045103 Opened 5 years ago Closed 5 years ago

add support for setWindowSize

Categories

(Testing :: Marionette, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla35

People

(Reporter: mdas, Assigned: rwood)

References

Details

(Keywords: pi-marionette-spec)

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → rwood
Attached patch bug1045103.patch (obsolete) — Splinter Review
Attachment #8481359 - Flags: review?(mdas)
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
Attachment #8481359 - Flags: review?(mdas) → review-
Attached patch bug1045103-take2.patch (obsolete) — Splinter Review
Attachment #8481359 - Attachment is obsolete: true
Attachment #8482895 - Flags: review?(jgriffin)
Comment on attachment 8482895 [details] [diff] [review]
bug1045103-take2.patch

Missed the last review comment, ignore this patch please
Attachment #8482895 - Flags: review?(jgriffin)
Attached patch bug1045103-take3.patch (obsolete) — Splinter Review
Attachment #8482895 - Attachment is obsolete: true
Attachment #8482940 - Flags: review?(jgriffin)
Thanks Jonathan. Ah, the unit test fails on the b2g emulator, I will look into it.
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.
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?
Flags: needinfo?(jgriffin)
Flags: needinfo?(dburns)
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.
Flags: needinfo?(jgriffin)
Attachment #8482940 - Flags: review?(jgriffin)
Duplicate of this bug: 1062928
(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
Flags: needinfo?(dburns)
Attached patch bug1045103-take4.patch (obsolete) — Splinter Review
Attachment #8482940 - Attachment is obsolete: true
Attachment #8485414 - Flags: review?(jgriffin)
Attachment #8485414 - Flags: review?(dburns)
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.
Attachment #8485414 - Flags: review?(jgriffin) → review+
Updated patch with addition of 'b2g = false' in the manifest, carrying fw the R+
Attachment #8485414 - Attachment is obsolete: true
Attachment #8485414 - Flags: review?(dburns)
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
Ugh, sorry :automatedtester, created Bug 1064421
https://hg.mozilla.org/mozilla-central/rev/e2d358265126
Status: NEW → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.