Closed Bug 1348145 Opened 8 years ago Closed 6 years ago

Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize

Categories

(Remote Protocol :: Marionette, enhancement, P3)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement

Tracking

(firefox-esr60 fixed, firefox61 fixed, firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla62
Tracking Flags:
Tracking Status
firefox-esr60 --- fixed
firefox61 --- fixed
firefox62 --- fixed

People

(Reporter: automatedtester, Assigned: vpit3833, Mentored)

References

Details

(Whiteboard: [lang=py][lang=js])

Attachments

(1 file, 7 obsolete files)

Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.
(deleted), text/x-review-board-request
whimboo
: review+
Details 
Since there needs to be some period where we don't break people's selenium tests, I am leaving this bug to remove those methods at some stage (probably 56 or 57).
Priority: -- → P3
I like to work on this.  How to find out which methods qualify to be removed?
Flags: needinfo?(dburns)
Summary: Remove setWIndowPosition and setWindowSize → Remove setWindowPosition and setWindowSize
Mentor: hskupin
status-firefox60: --- → affected
status-firefox61: --- → affected
Whiteboard: [lang=py][lang=js]
I am looking for a way to replace set_window_position calls in test_window_rect.py with set_window_rect before removing the definition of set_window_position.

What values of height and width do I use in setup method of test_window_rect.py?  There is no get_window_rect defined in marionette.py.  The closest I can find is in /testing/web-platform/tests/webdriver/tests/get_window_rect.py and doesn't look like what can be used here.

So, for the moment, I am thinking between setting up height and width with hand written constants or define get_window_rect in marionette.py.
Flags: needinfo?(hskupin)
Attached patch bug-1348145-attempt-01.patch (obsolete) (deleted) — — Splinter Review 
I might have answered my own question here.

Please comment on this and once everything is ironed out, I will upload a full patch removing setWindowSize as well.
Flags: needinfo?(hskupin)
Attachment #8965538 - Flags: feedback?(hskupin)
I will have a look in a moment. But beside that please note that we also have to remove the getters. I updated the summary accordingly.
Assignee: nobody → venkateshpitta
Status: NEW → ASSIGNED
Summary: Remove setWindowPosition and setWindowSize → Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize
Comment on attachment 8965538 [details] [diff] [review]
bug-1348145-attempt-01.patch

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

The changes mostly touch the right code, but please see my inline comments in what would need to be improved here.

::: testing/marionette/client/marionette_driver/marionette.py
@@ +1383,4 @@
>                        DeprecationWarning)
>          return self._send_message("getWindowPosition")
>  
> +    def get_window_rect(self):

There is no need to add another method here. Support for getting the window rect is already present by the `window_rect` property.

@@ +1393,1 @@
>      def set_window_position(self, x, y):

You want to update this method. I think it's better to keep it to ease the setting of the window position. That way you don't necessarily have to use the `set_window_rect` method directly.

For example see our webdriver client:

https://dxr.mozilla.org/mozilla-central/rev/2f5ffe4fa2153a798ed8b310a597ea92abd1b868/testing/web-platform/tests/tools/webdriver/webdriver/client.py#260-273

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py
@@ -12,4 @@
>  
>      def setUp(self):
>          MarionetteTestCase.setUp(self)
> -        self.original_position = self.marionette.get_window_position()

In driver.js you removed `setWindowPosition`, so this is not necessary to change unless you plan to also remove `getWindowPosition` in the same commit.

Maybe you do two commits, one for window position and the other for window size.

@@ +75,5 @@
> +            self.marionette.get_window_rect()))
> +        self.marionette.set_window_rect(-8, -8, 8, 8)
> +        position = self.marionette.get_window_rect()
> +        print("Position after requesting move to negative coordinates and dimensions: {}"
> +              .format(position))

This test should only test moving the window but not resizing.
Attachment #8965538 - Flags: feedback?(hskupin) → feedback+
(In reply to Henrik Skupin (:whimboo) from comment #6)

> ::: testing/marionette/client/marionette_driver/marionette.py
> @@ +1383,4 @@
> >                        DeprecationWarning)
> >          return self._send_message("getWindowPosition")
> >  
> > +    def get_window_rect(self):
> 
> There is no need to add another method here. Support for getting the window
> rect is already present by the `window_rect` property.

Done.  Removed the new method and started using the property.

> 
> @@ +1393,1 @@
> >      def set_window_position(self, x, y):
> 
> You want to update this method. I think it's better to keep it to ease the
> setting of the window position. That way you don't necessarily have to use
> the `set_window_rect` method directly.
> 
> For example see our webdriver client:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 2f5ffe4fa2153a798ed8b310a597ea92abd1b868/testing/web-platform/tests/tools/
> webdriver/webdriver/client.py#260-273
> 

Sorry, I am still not clear.  Do I use @command pattern on set_window_position to have it call set_window_rect?

> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py
> @@ -12,4 @@
> >  
> >      def setUp(self):
> >          MarionetteTestCase.setUp(self)
> > -        self.original_position = self.marionette.get_window_position()
> 
> In driver.js you removed `setWindowPosition`, so this is not necessary to
> change unless you plan to also remove `getWindowPosition` in the same commit.
> 
> Maybe you do two commits, one for window position and the other for window
> size.

I am attaching the edits to remove set/get window_position in the next comment/attachment.

> 
> @@ +75,5 @@
> > +            self.marionette.get_window_rect()))
> > +        self.marionette.set_window_rect(-8, -8, 8, 8)
> > +        position = self.marionette.get_window_rect()
> > +        print("Position after requesting move to negative coordinates and dimensions: {}"
> > +              .format(position))
> 
> This test should only test moving the window but not resizing.

ok, now passing the original height/width along and ignoring the two.  How can this be done better?
Attached patch bug-1348145-attempt-02.patch (obsolete) (deleted) — — Splinter Review 

  

  



        Attachment #8965538 -
        Attachment is obsolete: true

        Attachment #8965926 -
        Flags: feedback?(hskupin)

    
    

    
  
(In reply to Venkatesh from comment #7)
> > @@ +1393,1 @@
> > >      def set_window_position(self, x, y):
> > 
> > You want to update this method. I think it's better to keep it to ease the
> > setting of the window position. That way you don't necessarily have to use
> > the `set_window_rect` method directly.
> > 
> > For example see our webdriver client:
> > 
> > https://dxr.mozilla.org/mozilla-central/rev/
> > 2f5ffe4fa2153a798ed8b310a597ea92abd1b868/testing/web-platform/tests/tools/
> > webdriver/webdriver/client.py#260-273
> 
> Sorry, I am still not clear.  Do I use @command pattern on
> set_window_position to have it call set_window_rect?

No, I proposed to just use a call to `set_window_rect` with only the `x` and `y` coordinates specified. But now I had another look at the Python client code and as it looks like we are free to specify whatever we want. So I wonder if we should keep the old methods and enforce the appropriate parameters, or remove them and loosely handle sizing and positioning via `set_window_rect`. Personally I would prefer the former. Andreas, what do you think?

> > > +            self.marionette.get_window_rect()))
> > > +        self.marionette.set_window_rect(-8, -8, 8, 8)
> > > +        position = self.marionette.get_window_rect()
> > > +        print("Position after requesting move to negative coordinates and dimensions: {}"
> > > +              .format(position))
> > 
> > This test should only test moving the window but not resizing.
> 
> ok, now passing the original height/width along and ignoring the two.  How
> can this be done better?

`set_window_rect` currently accepts None for both width and heigh, so you could leave them out. But lets wait until the above question has been answered.

  

  


Flags: needinfo?(ato)

    
    

    
  
I have in the past raised concern about removing set_window_position
and set_window_size from the client API we offer to end-users.  The
WebDriver:SetWindowRect command is meant as a performance improvement
to reduce the number of driver requests, and unless you’re testing
the WebDriver protocol (as the WPT client does) it is more user
friendly to have the client API match what the user wants to do:
whilst it is possible to atomically reposition and resize the window
I don’t know how many users that is actually a useful primitive
for.

Now I don’t care about this so strongly that I want to discourage
the attached patched, but at the same time I don’t think the WPT
WebDriver is a good source of inspiration here since it is geared
more towards testing the low-level protocol than it is towards
providing an idiomatic and user-friendly API.

In any case, please feel free to move forward with the current plans.

  

  


Flags: needinfo?(ato)

    
    

    
  
(In reply to Andreas Tolfsen ‹:ato› from comment #10)
> I have in the past raised concern about removing set_window_position
> and set_window_size from the client API we offer to end-users.  The
> WebDriver:SetWindowRect command is meant as a performance improvement
> to reduce the number of driver requests, and unless you’re testing
> the WebDriver protocol (as the WPT client does) it is more user
> friendly to have the client API match what the user wants to do:
> whilst it is possible to atomically reposition and resize the window
> I don’t know how many users that is actually a useful primitive
> for.

Do you mean these functions are or could be useful primitives for users outside of Mozilla code base?  If that is the case, the functions carry a warning message informing of being deprecated and direct the user to other functions.  Could the messages be updated to mention a hard deadline for removal and be left untouched until then?  As the final step, the definitions can be removed on the day after the deadline, no?

> 
> Now I don’t care about this so strongly that I want to discourage
> the attached patched, but at the same time I don’t think the WPT
> WebDriver is a good source of inspiration here since it is geared
> more towards testing the low-level protocol than it is towards
> providing an idiomatic and user-friendly API.
> 
> In any case, please feel free to move forward with the current plans.

  

  


Flags: needinfo?(ato)

    
    

    
  
(In reply to Venkatesh from comment #11)

> Do you mean these functions are or could be useful primitives for
> users outside of Mozilla code base?  If that is the case, the
> functions carry a warning message informing of being deprecated
> and direct the user to other functions.  Could the messages
> be updated to mention a hard deadline for removal and be left
> untouched until then?  As the final step, the definitions can be
> removed on the day after the deadline, no?

I think probably the deprecation warnings have been there for long
enough that it is fine to remove them now.

  

  


Flags: needinfo?(ato)

    
    

    
  
Comment on attachment 8965926 [details] [diff] [review]
bug-1348145-attempt-02.patch

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

Removing feedback request given that Andreas gave his input.

  

  



        Attachment #8965926 -
        Flags: feedback?(hskupin)

    
    

    
  
Vankesh, if you can submit your patch via mozreview that would be fantastic. It needs a bit of setup, and you can find everything here: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#mozreview-commits

  

  



    
    

    
  

      
      
      
      

        Attached patch
          
          
        bug-1348145-attempt-03.patch (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
setWindowPosition and getWindowPosition removed.  Please comment.

  

  



        Attachment #8965926 -
        Attachment is obsolete: true

        Attachment #8967237 -
        Flags: review?(hskupin)

    
    

    
  
Hi Henrik, I get abort: authorisation failed on entering password.  Resetting/changing password did not help too.  I am trying `hg push -c 450059 review`.  Where can I look to find out what is going on and how to fix?

  

  



    
    

    
  
(In reply to Venkatesh from comment #16)
> Hi Henrik, I get abort: authorisation failed on entering password. 
> Resetting/changing password did not help too.  I am trying `hg push -c
> 450059 review`.  Where can I look to find out what is going on and how to
> fix?

Did you setup the authentication key in Bugzilla, and added it to your hg config? That shouldn't force you to enter a password when trying to push.

  

  



    
    

    
      
  


  

    
    

    
  
Comment on attachment 8967237 [details] [diff] [review]
bug-1348145-attempt-03.patch

Looks like it worked now. mozreview is clearly better, so let me remove this obsolete attachment.

  

  



        Attachment #8967237 -
        Attachment is obsolete: true

        Attachment #8967237 -
        Flags: review?(hskupin)

    
    

    
  
Comment on attachment 8967273 [details]
Bug 1348145 - remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/235964/#review241934

Feedback wise this would be a f+ because that looks already kinda good. Thank you for that update! For a full review a couple of pieces are still missing. Please have a look at my inline review comments. Let me know if something is unclear.

::: commit-message-14217:1
(Diff revision 1)
> +Bug 1348145 - remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.  r?whimboo

Note this only removes `getWindowPosition/setWindowPosition` yet. Are you planning to make the `getWindowSize/setWindowSize` removal part of this or another comment? You would have to update the commit message then.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:28
(Diff revision 1)
>      def test_get_types(self):
> -        position = self.marionette.get_window_position()
> +        position = self.marionette.window_rect
>          self.assertIn("x", position)
>          self.assertIn("y", position)
> +        self.assertIn("height", position)
> +        self.assertIn("width", position)

You better may want to combine the two test classes for TestPosition and TestSize in that file.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:37
(Diff revision 1)
> +        self.assertIsInstance(position["width"], int)
>  
>      def test_set_types(self):
> -        for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]):
> +        for x, y, h, w in (["a", "b", "h", "w"], [1.2, 3.4, 4.5, 5.6],
> +                           [True, False, True, False], [[], [], [], []],
> +                           [{}, {}, {}, {}]):

For better readability please define that list already before the for loop, and then each entry in its own line.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:61
(Diff revision 1)
>      def test_move_to_new_position(self):
> -        old_position = self.marionette.get_window_position()
> +        old_position = self.marionette.window_rect
>          new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
> -        self.marionette.set_window_position(new_position["x"], new_position["y"])
> +        self.marionette.set_window_rect(new_position["x"], new_position["y"])
>          self.assertNotEqual(old_position["x"], new_position["x"])
>          self.assertNotEqual(old_position["y"], new_position["y"])

Also check the height and width that both haven't been changed.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:74
(Diff revision 1)
>          self.assertEqual(old_position["y"], new_position["y"])
> +        self.assertEqual(old_position["height"], new_position["height"])
> +        self.assertEqual(old_position["width"], new_position["width"])
>  
>      def test_move_to_negative_coordinates(self):
> +        dims = self.marionette.window_rect

Please continue to use `old_position` here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:80
(Diff revision 1)
>          print("Current position: {}".format(
> -            self.marionette.get_window_position()))
> -        self.marionette.set_window_position(-8, -8)
> -        position = self.marionette.get_window_position()
> -        print("Position after requesting move to negative coordinates: {}".format(position))
> +            dims["x"], dims["y"]))
> +        self.marionette.set_window_rect(-8, -8)
> +        position = self.marionette.window_rect
> +        print("Position after requesting move to negative coordinates: {}, {}"
> +              .format(position))

Only print x and y here.

  

  



        Attachment #8967273 -
        Flags: review?(hskupin) → review-

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #20)
> Comment on attachment 8967273 [details]

> ::: commit-message-14217:1
> (Diff revision 1)
> > +Bug 1348145 - remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.  r?whimboo
> 
> Note this only removes `getWindowPosition/setWindowPosition` yet. Are you
> planning to make the `getWindowSize/setWindowSize` removal part of this or
> another comment? You would have to update the commit message then.
> 
I was typing/copying from the title.  I am thinking removing getWindowPosition/setWindowPosition can be worked on separately from getWindowSize/setWindowSize.  Changed the commit message accordingly.  Will it be OK?

> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:
> 28
> (Diff revision 1)
> >      def test_get_types(self):
> > -        position = self.marionette.get_window_position()
> > +        position = self.marionette.window_rect
> >          self.assertIn("x", position)
> >          self.assertIn("y", position)
> > +        self.assertIn("height", position)
> > +        self.assertIn("width", position)
> 
> You better may want to combine the two test classes for TestPosition and
> TestSize in that file.
> 

Will it be OK to do this as a last step toward concluding work on this bug?

> :::
> testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:
> 74
> (Diff revision 1)
> >          self.assertEqual(old_position["y"], new_position["y"])
> > +        self.assertEqual(old_position["height"], new_position["height"])
> > +        self.assertEqual(old_position["width"], new_position["width"])
> >  
> >      def test_move_to_negative_coordinates(self):
> > +        dims = self.marionette.window_rect
> 
> Please continue to use `old_position` here.
Calling it `position` here so that the rest of the method can remain unedited.

  

  



    
    

    
      
  


  

    
  

        Attachment #8967273 -
        Attachment is obsolete: true

    
    

    
  
(In reply to Venkatesh from comment #21)
> > Note this only removes `getWindowPosition/setWindowPosition` yet. Are you
> > planning to make the `getWindowSize/setWindowSize` removal part of this or
> > another comment? You would have to update the commit message then.
> > 
> I was typing/copying from the title.  I am thinking removing
> getWindowPosition/setWindowPosition can be worked on separately from
> getWindowSize/setWindowSize.  Changed the commit message accordingly.  Will
> it be OK?

The patch itself is small enough to fit the removal of all those methods. It will also make it easier for you to combine the tests, without having an interim change. I would propose to do all in a single commit.

Please note that for the commit you pushed earlier today the bug number in the commit message is wrong, and that it got attached to a different bug. Best always is to run `hg amend` to squash the latest changes in the previous patch. In your case run `hg histedit` to combine them.

> > You better may want to combine the two test classes for TestPosition and
> > TestSize in that file.
> 
> Will it be OK to do this as a last step toward concluding work on this bug?

The above proposal also answers this bug.

> > Please continue to use `old_position` here.
>
> Calling it `position` here so that the rest of the method can remain
> unedited.

As best use what other tests are using. It makes it easier to understand.

  

  



    
    

    
  
A summary of what is in this patch:

1. remove setWindowSize/getWindowSize and all their usages
2. remove set_window_size/get_window_size and all their usages
3. bring the body of methods that had a large overlap of code into one (and renamed them to indicate this)
4. combined the two classes into one

  

  



    
    

    
      
  


  

    
  

        Attachment #8967606 -
        Attachment is obsolete: true

        Attachment #8967606 -
        Flags: review?(hskupin)

    
    

    
  
Comment on attachment 8967606 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition.

https://reviewboard.mozilla.org/r/236308/#review242658

I don't think that you wanted to discard this commit?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:35
(Diff revision 1)
>          self.assertIsInstance(position["y"], int)
>          self.assertIsInstance(position["height"], int)
>          self.assertIsInstance(position["width"], int)
>  
>      def test_set_types(self):
> -        for x, y, h, w in (["a", "b", "h", "w"], [1.2, 3.4, 4.5, 5.6],
> +        illegal_rects = (["a", "b", "h", "w"], 

Please note the trailing space at the end. The linter will mark this as failure. Please fix it.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:64
(Diff revision 1)
>      def test_move_to_new_position(self):
>          old_position = self.marionette.window_rect
>          new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
>          self.marionette.set_window_rect(new_position["x"], new_position["y"])
>          self.assertNotEqual(old_position["x"], new_position["x"])
>          self.assertNotEqual(old_position["y"], new_position["y"])

Oh, I didn't notice before... Why does this test not check for the new coordinates, but does a not equal check for the old position? It means we only test half of it. Please change it to `assertEqual()`.

  

  



        Attachment #8967606 -
        Flags: review?(hskupin)

    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review242668

::: testing/marionette/client/marionette_driver/marionette.py
(Diff revision 1)
>          :param orientation: The orientation to lock the screen in.
>          """
>          body = {"orientation": orientation}
>          self._send_message("setScreenOrientation", body)
>  
> -    @property

The removal is fine, but getWindowPosition/setWindowPosition is no longer removed. I think you accidentally got rid of all your former commits?

You can avoid that by checking the patches on mozreview before publishing.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:11
(Diff revision 1)
>  
>  from marionette_driver.errors import InvalidArgumentException
>  from marionette_harness import MarionetteTestCase
>  
>  
> -class TestPosition(MarionetteTestCase):
> +class TestPositionAndSize(MarionetteTestCase):

Best call it `TestWindowRect`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:14
(Diff revision 1)
>  
>  
> -class TestPosition(MarionetteTestCase):
> +class TestPositionAndSize(MarionetteTestCase):
>  
>      def setUp(self):
> +        super(MarionetteTestCase, self).setUp()

When you use `super` then it has to the current class, but not the base class.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:16
(Diff revision 1)
> -class TestPosition(MarionetteTestCase):
> +class TestPositionAndSize(MarionetteTestCase):
>  
>      def setUp(self):
> +        super(MarionetteTestCase, self).setUp()
> +
>          MarionetteTestCase.setUp(self)

You already called `setUp` of the base class above.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:17
(Diff revision 1)
>  
>      def setUp(self):
> +        super(MarionetteTestCase, self).setUp()
> +
>          MarionetteTestCase.setUp(self)
>          self.original_position = self.marionette.window_rect

This should be `original_rect` and at the end of `setUp`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:44
(Diff revision 1)
>          self.marionette.set_window_rect(x, y, height, width)
> +        is_fullscreen = self.marionette.execute_script("return document.fullscreenElement;", sandbox=None)
> +        if is_fullscreen:
> +            self.marionette.fullscreen()
>          MarionetteTestCase.tearDown(self)
> +        super(MarionetteTestCase, self).tearDown()

Same here for the usage of `super`, and the double call to `tearDown`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:73
(Diff revision 1)
>      def test_setting_window_rect_with_nulls_errors(self):
>          with self.assertRaises(InvalidArgumentException):
>              self.marionette.set_window_rect(height=None, width=None,
>                                              x=None, y=None)
>  
> -    def test_set_position_with_rect(self):
> +    def test_set_position_and_size_with_rect(self):

Make sure that we test:

1) Set position only
2) Set size only
3) Set both position and size

  

  



        Attachment #8967922 -
        Flags: review?(hskupin) → review-

    
    

    
      
  

    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review242886


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118
(Diff revision 2)
> +        self.assertNotEqual(old["x"], new["x"])
> +        self.assertNotEqual(old["y"], new["y"])
> +
> +    def test_move_to_new_size(self):
> +        old = self.marionette.window_rect
> +        new = {"height": old["height"] + 10, "widht": old["width"])

Error: Invalid syntax [py3: is-parseable]

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118
(Diff revision 2)
> +        self.assertNotEqual(old["x"], new["x"])
> +        self.assertNotEqual(old["y"], new["y"])
> +
> +    def test_move_to_new_size(self):
> +        old = self.marionette.window_rect
> +        new = {"height": old["height"] + 10, "widht": old["width"])

Error: Invalid syntax [py3: is-parseable]

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118
(Diff revision 2)
> +        self.assertNotEqual(old["x"], new["x"])
> +        self.assertNotEqual(old["y"], new["y"])
> +
> +    def test_move_to_new_size(self):
> +        old = self.marionette.window_rect
> +        new = {"height": old["height"] + 10, "widht": old["width"])

Error: Invalid syntax [py2: is-parseable]

  

  



    
    

    
      
  

    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review242900


Code analysis found 3 defects in this patch:
 - 3 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118
(Diff revision 3)
> +        self.assertNotEqual(old["x"], new["x"])
> +        self.assertNotEqual(old["y"], new["y"])
> +
> +    def test_move_to_new_size(self):
> +        old = self.marionette.window_rect
> +        new = {"height": old["height"] + 10, "width": old["width"])

Error: Invalid syntax [py2: is-parseable]

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118
(Diff revision 3)
> +        self.assertNotEqual(old["x"], new["x"])
> +        self.assertNotEqual(old["y"], new["y"])
> +
> +    def test_move_to_new_size(self):
> +        old = self.marionette.window_rect
> +        new = {"height": old["height"] + 10, "width": old["width"])

Error: Invalid syntax [py2: is-parseable]

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:118
(Diff revision 3)
> +        self.assertNotEqual(old["x"], new["x"])
> +        self.assertNotEqual(old["y"], new["y"])
> +
> +    def test_move_to_new_size(self):
> +        old = self.marionette.window_rect
> +        new = {"height": old["height"] + 10, "width": old["width"])

Error: Invalid syntax [py3: is-parseable]

  

  



    
    

    
      
  

    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review243334

This patch look way better an includes everything we need. Thanks! Nevertheless I had to point out a couple of comments. Please have a look at those, and make sure to `hg commit --amend` the changes before pushing. Also please run all the tests, and also the linter (mach lint testing/marionette).

::: testing/firefox-ui/tests/functional/sessionstore/test_tabbar_session_restore_button.py:76
(Diff revision 4)
>          # Ensure the window is large enough to show the button.
> -        self.marionette.set_window_size(1200, 1200)
> +        self.marionette.set_window_rect(height=1200, width=1200)
>          self.assertEqual(wrapper.value_of_css_property('visibility'), 'visible')
>  
>          # Set the window small enough that the button disappears.
> -        self.marionette.set_window_size(335, 335)
> +        self.marionette.set_window_rect(height=335, width=335)

Good catch!

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:16
(Diff revision 4)
> -class TestPosition(MarionetteTestCase):
> +class TestWindowRect(MarionetteTestCase):
>  
>      def setUp(self):
> -        MarionetteTestCase.setUp(self)
> -        self.original_position = self.marionette.get_window_position()
> +        self.original_rect = self.marionette.window_rect
> +
> +        super(TestWindowRect, self).setUp()

You should call the super __init__ method first before doing any special initialization for that class.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:42
(Diff revision 4)
> -        self.marionette.set_window_position(x, y)
> -        MarionetteTestCase.tearDown(self)
> +        height, width = self.original_rect["height"], self.original_rect["width"]
> +        self.marionette.set_window_rect(x, y, height, width)
> +        is_fullscreen = self.marionette.execute_script("return document.fullscreenElement;", sandbox=None)
> +        if is_fullscreen:
> +            self.marionette.fullscreen()
> +        super(TestWindowRect, self).tearDown()

nit: please add a blank line before.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:45
(Diff revision 4)
> +        if is_fullscreen:
> +            self.marionette.fullscreen()
> +        super(TestWindowRect, self).tearDown()
>  
>      def test_get_types(self):
> -        position = self.marionette.get_window_position()
> +        position = self.marionette.window_rect

it's not the position only anymore.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:56
(Diff revision 4)
>          self.assertIsInstance(position["y"], int)
> +        self.assertIsInstance(position["height"], int)
> +        self.assertIsInstance(position["width"], int)
>  
>      def test_set_types(self):
> -        for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]):
> +        illegal_rects = (["a", "b", "h", "w"], 

nit: there is still the trailing white-space.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:75
(Diff revision 4)
>  
> -    def test_set_position_with_rect(self):
> +    def test_set_position(self):
>          old_position = self.marionette.window_rect
>          wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
>  
> -        new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
> +        new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])

You can keep `new_position` here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:78
(Diff revision 4)
>          wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
>  
> -        new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
> +        new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
> +
> +        self.assertNotEqual(old_position["x"], new["x"])
> +        self.assertNotEqual(old_position["y"], new["y"])

You didn't reply to one of my last review comments here...

Why not checking for the new position? We would miss half of the test here. Or would that not work?

Also make sure to add the expected state as second argument.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:80
(Diff revision 4)
> -        new_position = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
> +        new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
> +
> +        self.assertNotEqual(old_position["x"], new["x"])
> +        self.assertNotEqual(old_position["y"], new["y"])
> +
> +    def test_set_size(self):

Note, for an easier reviewing it would have been good to not move test methods around in that file at the same time  when you make changes.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:84
(Diff revision 4)
> +
> +    def test_set_size(self):
> +        old_size = self.marionette.window_rect
> +        wanted_size = {"height": old_size["height"] - 50, "width": old_size["width"] - 50}
>  
> -        self.assertNotEqual(old_position["x"], new_position["x"])
> +        new = self.marionette.set_window_rect(height=wanted_size["height"], weight=wanted_size["width"])

`new_size` please

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:91
(Diff revision 4)
> +        self.assertEqual(new["width"], wanted_size["width"],
> +                         "New width is {0} but should be {1}".format(new["width"], wanted_size["width"]))
> +        self.assertEqual(new["height"], wanted_size["height"],
> +                         "New height is {0} but should be {1}".format(new["height"], wanted_dize["height"]))
> +
> +    def test_set_position_and_size_with_rect(self):

Remove the `with_rect` suffix from the test name.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:92
(Diff revision 4)
> +                         "New width is {0} but should be {1}".format(new["width"], wanted_size["width"]))
> +        self.assertEqual(new["height"], wanted_size["height"],
> +                         "New height is {0} but should be {1}".format(new["height"], wanted_dize["height"]))
> +
> +    def test_set_position_and_size_with_rect(self):
> +        old_position = self.marionette.window_rect

`old_rect` given that you cover position and size.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:95
(Diff revision 4)
> +
> +    def test_set_position_and_size_with_rect(self):
> +        old_position = self.marionette.window_rect
> +        wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
> +        width = old_position["width"] - 50
> +        height = old_position["height"] - 50

Include those two lines into the `wanted_rect`, similar to x and y.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:101
(Diff revision 4)
> +
> +        new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"],
> +                                              width=width, height=height)
> +
> +        self.assertNotEqual(old_position["x"], new["x"])
> +        self.assertNotEqual(old_position["y"], new["y"])

the expected value has to be the second argument.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:107
(Diff revision 4)
> +        self.assertEqual(new["width"], width,
> +                         "New width is {0} but should be {1}".format(new["width"], width))
> +        self.assertEqual(new["height"], height,
> +                         "New height is {0} but should be {1}".format(new["height"], height))
>  
>      def test_move_to_new_position(self):

What's the difference to `test_set_position`?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:116
(Diff revision 4)
> -        self.assertNotEqual(old_position["x"], new_position["x"])
> -        self.assertNotEqual(old_position["y"], new_position["y"])
> +        self.marionette.set_window_rect(x=new["x"], y=new["y"])
> +
> +        self.assertNotEqual(old["x"], new["x"])
> +        self.assertNotEqual(old["y"], new["y"])
> +
> +    def test_move_to_new_size(self):

What's the difference to `test_set_size`?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:126
(Diff revision 4)
> +        actual = self.marionette.window_rect
> +
> +        self.assertEqual(actual["width"], new["width"])
> +        self.assertEqual(actual["height"], new["height"])
> +
> +    def test_move_to_new_position_and_size(self):

What's the difference to `test_set_position_and_size_with_rect`?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:148
(Diff revision 4)
> -        self.marionette.set_window_position(old_position["x"], old_position["y"])
> -        new_position = self.marionette.get_window_position()
> -        self.assertEqual(old_position["x"], new_position["x"])
> -        self.assertEqual(old_position["y"], new_position["y"])
> +        self.marionette.set_window_rect(x=old["x"], y=old["y"])
> +
> +        new = self.marionette.window_rect
> +
> +        self.assertEqual(old["x"], new["x"])
> +        self.assertEqual(old["y"], new["y"])

The expected value has to be the second argument. The same applies to all the following asserts.

  

  



        Attachment #8967922 -
        Flags: review?(hskupin) → review-

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #33)

> This patch look way better an includes everything we need. Thanks!
> Nevertheless I had to point out a couple of comments. Please have a look at
> those, and make sure to `hg commit --amend` the changes before pushing. Also
> please run all the tests, and also the linter (mach lint testing/marionette).
> 
I got the ./mach lint ./path/to/file[s].  How do I run tests?


> Good catch!
> 
Thank you :)

I am not clear on whether to move this https://searchfox.org/mozilla-central/source/testing/web-platform/tests/tools/wptrunner/wptrunner/executors/executorselenium.py#341 to set_window_rect as well.  Because there is no marionette imported at the top.

> 
> You didn't reply to one of my last review comments here...
> 
> Why not checking for the new position? We would miss half of the test here.
> Or would that not work?
> 

Sorry about that, I should have missed.  Which previous comment have I missed?  And, is new position a reference to the variable name?  Or, I am not clear on what you are asking here.

> Also make sure to add the expected state as second argument.
> 
Doing this everywhere.

> Note, for an easier reviewing it would have been good to not move test
> methods around in that file at the same time  when you make changes.
> 
Agreed.

> >      def test_move_to_new_position(self):
> 
> What's the difference to `test_set_position`?
> 
There isn't any.  Something I missed.  Removed the method.

> > +    def test_move_to_new_size(self):
> 
> What's the difference to `test_set_size`?
> 
Removed.  There isn't any.

> > +    def test_move_to_new_position_and_size(self):
> 
> What's the difference to `test_set_position_and_size_with_rect`?
> 
Removed as well.

> > +        self.assertEqual(old["x"], new["x"])
> > +        self.assertEqual(old["y"], new["y"])
> 
> The expected value has to be the second argument. The same applies to all
> the following asserts.

Doing this everywhere.

Pending these, I have submitted to review.

  

  


Flags: needinfo?(hskupin)

    
    

    
      
  

    
    

    
  
You can find more information about running tests here:
https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Testing.html

And more general information on contributing to Marionette:
https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Contributing.html

  

  



    
    

    
  
(In reply to Venkatesh from comment #34)
> I am not clear on whether to move this
> https://searchfox.org/mozilla-central/source/testing/web-platform/tests/
> tools/wptrunner/wptrunner/executors/executorselenium.py#341 to
> set_window_rect as well.  Because there is no marionette imported at the top.

No, this is using the webdriver client. See the appropriate link in comment 6. There is nothing you have to care about.

> > Why not checking for the new position? We would miss half of the test here.
> > Or would that not work?
> 
> Sorry about that, I should have missed.  Which previous comment have I
> missed?  And, is new position a reference to the variable name?  Or, I am
> not clear on what you are asking here.

Maybe you missed it because older patches have been marked as obsolete, and I was commenting on a discarded commit. Anyway please have a look at the correct checks. And yes, with `new_position` I was referring to the variable name.

  

  


Flags: needinfo?(hskupin)

    
    

    
  
(In reply to Andreas Tolfsen ‹:ato› from comment #36)
> You can find more information about running tests here:
> https://firefox-source-docs.mozilla.org/testing/marionette/marionette/
> Testing.html
> 
> And more general information on contributing to Marionette:
> https://firefox-source-docs.mozilla.org/testing/marionette/marionette/
> Contributing.html

I am failing at the setup step `pip install -r testing/config/marionette_requirements.txt`  listed at https://firefox-source-docs.mozilla.org/testing/marionette/marionette/Testing.html with

Invalid requirement: '../tools/mozterm'
It looks like a path. Does it exist?

output of `python -m site`:
sys.path = [
    '/home/venkatesh/src/mozilla-unified',
    '/usr/lib/python2.7',
    '/usr/lib/python2.7/plat-x86_64-linux-gnu',
    '/usr/lib/python2.7/lib-tk',
    '/usr/lib/python2.7/lib-old',
    '/usr/lib/python2.7/lib-dynload',
    '/home/venkatesh/.local/lib/python2.7/site-packages',
    '/usr/local/lib/python2.7/dist-packages',
    '/usr/lib/python2.7/dist-packages',
    '/usr/lib/python2.7/dist-packages/PILcompat',
    '/usr/lib/python2.7/dist-packages/gtk-2.0',
    '/usr/lib/pymodules/python2.7',
    '/usr/lib/python2.7/dist-packages/wx-3.0-gtk3',
]
USER_BASE: '/home/venkatesh/.local' (exists)
USER_SITE: '/home/venkatesh/.local/lib/python2.7/site-packages' (exists)
ENABLE_USER_SITE: True

How do I solve this?

  

  


Flags: needinfo?(ato)
Flags: needinfo?(ato) → needinfo?(hskupin)

    
    

    
  
(In reply to Venkatesh from comment #38)
> I am failing at the setup step `pip install -r
> testing/config/marionette_requirements.txt`  listed at
> https://firefox-source-docs.mozilla.org/testing/marionette/marionette/
> Testing.html with

This is actually wrong. This part of the documentation relates to the test archives, which you would have to download from a finished build. This is not something what you want to do. So check the paragraph above regarding `mach`. This is the right tool to run the Marionette and other tests.

  

  


Flags: needinfo?(hskupin)

    
    

    
  
A side-note: Is it possible the documentation can be misunderstood
here?  I think possibly it isn’t clear to contributors if they want
to run tests form a “downloaded test archive” or not.

  

  



    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review243334

> You didn't reply to one of my last review comments here...
> 
> Why not checking for the new position? We would miss half of the test here. Or would that not work?
> 
> Also make sure to add the expected state as second argument.

This has still not resolved. Please check the open issues and if you responded to all of them before publishing a next version of the patch. It helps to speed-up the process. In case of questions just add a comment for the issue in mozreview.

> What's the difference to `test_set_position_and_size_with_rect`?

Still identical to `test_set_position_and_size`, right?

  

  



    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review244610

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:79
(Diff revisions 4 - 5)
>      def test_set_position(self):
>          old_position = self.marionette.window_rect
>          wanted_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
>  
> -        new = self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
> +        self.marionette.set_window_rect(x=wanted_position["x"], y=wanted_position["y"])
> +        new_position = self.marionette.window_rect

Please do not use the getter here, but keep what `set_window_rect` returns.

What you then could do is to assert that the return value and what `window_rec` returns is equal. That would actually be a nice addition.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:89
(Diff revisions 4 - 5)
>      def test_set_size(self):
>          old_size = self.marionette.window_rect
>          wanted_size = {"height": old_size["height"] - 50, "width": old_size["width"] - 50}
>  
> -        new = self.marionette.set_window_rect(height=wanted_size["height"], weight=wanted_size["width"])
> -
> +        self.marionette.set_window_rect(height=wanted_size["height"], weight=wanted_size["width"])
> +        new_size = self.marionette.window_rect

Same as above. Use the return value from `set_window_rect`.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:99
(Diff revisions 4 - 5)
> -                         "New width is {0} but should be {1}".format(new["width"], width))
> -        self.assertEqual(new["height"], height,
> -                         "New height is {0} but should be {1}".format(new["height"], height))
> -
> -    def test_move_to_new_position(self):
> -        old = self.marionette.window_rect
> +                         "New height is {0} but should be {1}".format(new_size["height"], wanted_dize["height"]))
> +
> +    def test_set_position_and_size(self):
> +        old_rect = self.marionette.window_rect
> +        wanted_rect = {"x": old_rect["x"] + 10, "y": old_rect["y"] + 10,
> +                           "width": old_rect["width"] - 50, "height": old_rect["height"] - 50}

The indentation here is wrong and the linter should complain about it. Start directly under "x".

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:103
(Diff revisions 4 - 5)
> -    def test_move_to_new_position(self):
> -        old = self.marionette.window_rect
> -        new = {"x": old["x"] + 10, "y": old["y"]}
> -
> -        self.marionette.set_window_rect(x=new["x"], y=new["y"])
> -
> +        wanted_rect = {"x": old_rect["x"] + 10, "y": old_rect["y"] + 10,
> +                           "width": old_rect["width"] - 50, "height": old_rect["height"] - 50}
> +
> +        self.marionette.set_window_rect(x=wanted_rect["x"], y=wanted_rect["y"],
> +                                        width=wanted_rect["width"], height=wanted_rect["height"])
> +        new_rect = self.marionette.window_rect

Same as above for using the return value of "set_window_rect".

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:106
(Diff revisions 4 - 5)
> -
> -        self.marionette.set_window_rect(x=new["x"], y=new["y"])
> -
> -        self.assertNotEqual(old["x"], new["x"])
> -        self.assertNotEqual(old["y"], new["y"])
> -
> +        self.marionette.set_window_rect(x=wanted_rect["x"], y=wanted_rect["y"],
> +                                        width=wanted_rect["width"], height=wanted_rect["height"])
> +        new_rect = self.marionette.window_rect
> +
> +        self.assertNotEqual(new_rect["x"], old_rect["x"])
> +        self.assertNotEqual(new_rect["y"], old_rect["y"])

Still why not equal and not equal?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:127
(Diff revisions 4 - 5)
> -        self.assertNotEqual(old["y"], new["y"])
> +        self.assertNotEqual(new["y"], old["y"])
>  
>          self.assertEqual(actual["width"], new["width"])
>          self.assertEqual(actual["height"], new["height"])
>  
>      def test_move_to_existing_position(self):

To better understand this test and the following, would you mind to update the title from `existing` to `current`? It's a bit unclear what existing means here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:223
(Diff revisions 4 - 5)
>          self.assertGreaterEqual(new["height"], self.max["height"])
>  
>      def test_resize_to_available_screen_size(self):
> -        result = self.marionette.set_window_rect(width=self.max['width'],
> +        self.marionette.set_window_rect(width=self.max['width'],
> -                                                 height=self.max["height"])
> +                                        height=self.max["height"])
> +        result = self.marionette.window_rect

Same as above to use the return value.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:232
(Diff revisions 4 - 5)
>  
>      def test_resize_while_fullscreen(self):
>          self.marionette.fullscreen()
> -        result = self.marionette.set_window_rect(width=self.max["width"] - 100,
> +        self.marionette.set_window_rect(width=self.max["width"] - 100,
> -                                                 height=self.max["height"] - 100)
> +                                        height=self.max["height"] - 100)
> +        result = self.marionette.window_rect

Here too.

  

  



        Attachment #8967922 -
        Flags: review?(hskupin) → review-

    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review242886

> Error: Invalid syntax [py3: is-parseable]

Can you please close those bot issues once you addressed them all? I'm not allowed to do so. Thanks.

  

  



    
    

    
  
(In reply to Andreas Tolfsen ‹:ato› from comment #40)
> A side-note: Is it possible the documentation can be misunderstood
> here?  I think possibly it isn’t clear to contributors if they want
> to run tests form a “downloaded test archive” or not.

Given that nowadays you usually run via `mach` and only use the test archive on one-click loaners, we should get rid of this section. I should have this covered that part in the other doc for taskcluster already.

  

  



    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review243334

> This has still not resolved. Please check the open issues and if you responded to all of them before publishing a next version of the patch. It helps to speed-up the process. In case of questions just add a comment for the issue in mozreview.

Done, now testing against wanted_position as well as expected_position as returned by the getter.

> Still identical to `test_set_position_and_size`, right?

Yes, simply dropped the _with_rect suffix.

  

  



    
    

    
      
  

    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review243334

> Yes, simply dropped the _with_rect suffix.

Sorry I missed the previous comment.  Are you suggesting that test_setting_window_rect_with_nulls_errors and test_set_position_and_size be made into one method?

  

  



    
    

    
      
  

    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review243334

> Sorry I missed the previous comment.  Are you suggesting that test_setting_window_rect_with_nulls_errors and test_set_position_and_size be made into one method?

No but `test_move_to_new_position_and_size` and `test_set_position_and_size` looks the same.

  

  



    
    

    
      
  

    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #49)
> Comment on attachment 8967922 [details]
> Bug 1348145 - Remove getWindowPosition/setWindowPosition and
> getWindowSize/setWindowSize.
> 
> https://reviewboard.mozilla.org/r/236620/#review243334
> 
> > Sorry I missed the previous comment.  Are you suggesting that test_setting_window_rect_with_nulls_errors and test_set_position_and_size be made into one method?
> 
> No but `test_move_to_new_position_and_size` and `test_set_position_and_size`
> looks the same.

I removed test_move_to_new_position_and_size and submitted for review.  Is it alright?  What else is left to conclude work on this bug?

  

  



    
    

    
  
I will have a look when I'm back on Wednesday. Thank you for the update.

  

  



    
    

    
  
Comment on attachment 8967922 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/236620/#review247256

Sorry that this review has been taken a bit longer. But there have been public holidays and some time off. As it looks like we are not that far from having the final code ready. Most likely only one remaining update of your patch is necessary. Once done I will submit it to try. Thank you!

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:126
(Diff revisions 5 - 8)
> -        self.assertEqual(actual["width"], new["width"])
> -        self.assertEqual(actual["height"], new["height"])
> -
> -    def test_move_to_existing_position(self):
>          old = self.marionette.window_rect
>          self.marionette.set_window_rect(x=old["x"], y=old["y"])

Please use the return value from set_window_rect similar to above. Same applies to the next two methods too.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:209
(Diff revisions 5 - 8)
>  
> -
>      def test_resize_larger_than_screen(self):
> -        self.marionette.set_window_rect(
> +        new = self.marionette.set_window_rect(
>              self.max["width"] * 2, self.max["height"] * 2)
> -        new = self.marionette.window_rect
> +        actual = self.marionette.window_rect

Try to keep the naming schema between tests. Above you used `new_rect` and `expected_rect` which I find more descriptive. This also applies to the next three methods.

  

  



        Attachment #8967922 -
        Flags: review?(hskupin) → review-

    
    

    
      
  


  

    
  

        Attachment #8967922 -
        Attachment is obsolete: true

    
    

    
  
./mach marionette test -z fails with 120 second server timeout.  So I tried merge and commit.  Didn't work.  Then I backed out.  hg histedit -o aborts saying cannot edit history that contains merge.  So this review submission is the edits just to test_window_rect.py.  How do I generate the complete patch?

  

  



    
    

    
  
(In reply to Venkatesh from comment #55)
> ./mach marionette test -z fails with 120 second server timeout.  So I tried

Did you pull from mozilla-central and missed to run "mach build" afterward? This could indicate a problem like that. If not please reach out to us on IRC and we can figure it out.

> merge and commit.  Didn't work.  Then I backed out.  hg histedit -o aborts
> saying cannot edit history that contains merge.  So this review submission
> is the edits just to test_window_rect.py.  How do I generate the complete
> patch?

Given that I don't know which exact commands you run, I cannot really help. Generally when you are working on a bookmark no backout is necessary. Maybe that confused mercurial, and now you are blocked on the merge commit. Best solution here is most likely to create a new bookmark, import the patch from the first attachment, and then apply your fix-up on top of it. Running "hg push review" afterward should update the mozreview patch on this bug.

  

  



    
    

    
  
Note, given that you closed the former mozreview request, reopen it please in mozreview. And first attachment from my last comment actually applies to this code.

  

  



    
    

    
  
Comment on attachment 8973373 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/241830/#review247836

  

  



        Attachment #8973373 -
        Flags: review?(hskupin)

    
    

    
      
  


  

    
  

        Attachment #8973373 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review248492


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/client/marionette_driver/marionette.py:15
(Diff revision 1)
>  import os
>  import socket
>  import sys
>  import time
>  import traceback
>  import warnings

Error: 'warnings' imported but unused [flake8: F401]

  

  



    
    

    
      
  

    
    

    
  
Venkatesh, I don't know why mozreview always creates a complete new review request / attachment for every time you upload something. For reviewers it means they have to re-review the whole patch because no interdiff is available. I will do that now, but for the next bug you want to work on, we should make sure that this doesn't happen anymore. Thanks!

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review248950

There are still a couple of things to fix. Beside that I will already trigger a try build so that we can see if there is something else which is failing and needs an update.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:135
(Diff revision 2)
> -        self.assertNotEqual(old_position["x"], new_position["x"])
> -        self.assertNotEqual(old_position["y"], new_position["y"])
> +        self.assertEqual(new_position["x"], old_position["x"])
> +        self.assertEqual(new_position["y"], old_position["y"])
>  
> -    def test_move_to_new_position(self):
> -        old_position = self.marionette.get_window_position()
> -        new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
> +    def test_move_to_current_size(self):
> +        old_size = self.marionette.window_rect
> +        self.marionette.set_window_rect(height=old_size["height"], width=old_size["width"])

Similar to other tests you still missed to add the use of the return value here as new_size.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:140
(Diff revision 2)
> -        new_position = {"x": old_position["x"] + 10, "y": old_position["y"] + 10}
> -        self.marionette.set_window_position(new_position["x"], new_position["y"])
> -        self.assertNotEqual(old_position["x"], new_position["x"])
> -        self.assertNotEqual(old_position["y"], new_position["y"])
> -
> -    def test_move_to_existing_position(self):
> +        self.marionette.set_window_rect(height=old_size["height"], width=old_size["width"])
> +
> +        new_size = self.marionette.window_rect
> +
> +        self.assertEqual(new_size["height"], old_size["height"])
> +        self.assertEuqal(new_size["width"], old_size["width"])

This is a typing failure which should have marked the test as failed. Did you run the test locally?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:144
(Diff revision 2)
> -
> -    def test_move_to_existing_position(self):
> -        old_position = self.marionette.get_window_position()
> -        self.marionette.set_window_position(old_position["x"], old_position["y"])
> -        new_position = self.marionette.get_window_position()
> -        self.assertEqual(old_position["x"], new_position["x"])
> +        self.assertEqual(new_size["height"], old_size["height"])
> +        self.assertEuqal(new_size["width"], old_size["width"])
> +
> +    def test_move_to_current_position_and_size(self):
> +        old_position_and_size = self.marionette.window_rect
> +        self.marionette.set_window_rect(old_position_and_size["x"], old_position_and_size["y"],

Similar for the return value here.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:215
(Diff revision 2)
>  
>          # in X the window size may be greater than the bounds of the screen
> -        self.assertGreaterEqual(new["width"], self.max["width"])
> -        self.assertGreaterEqual(new["height"], self.max["height"])
> +        self.assertGreaterEqual(new_size["width"], self.max["width"])
> +        self.assertGreaterEqual(new_size["height"], self.max["height"])
> +        self.assertEqual(new_size["width"], actual_size["width"])
> +        self.assertEqual(new_size["height"], actual_size["width"])

Given the comment from above we might not be able to do an equal check? Did you test this?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:220
(Diff revision 2)
> +        self.assertEqual(new_size["height"], actual_size["width"])
>  
>      def test_resize_to_available_screen_size(self):
> -        result = self.marionette.set_window_rect(width=self.max['width'],
> +        expected_size = self.marionette.set_window_rect(width=self.max["width"],
> -                                                 height=self.max["height"])
> +                                                        height=self.max["height"])
> -        self.assertEqual(result["width"], self.max["width"])
> +        result_size = self.marionette.window_rect

Please try to keep the naming schema across tests for the variables. That makes it easier to read and to understand what we test. So far I have seen 3 variations.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:225
(Diff revision 2)
> -        self.assertEqual(result["width"], self.max["width"])
> -        self.assertEqual(result["height"], self.max["height"])
> +        result_size = self.marionette.window_rect
> +
> +        self.assertEqual(result_size["width"], self.max["width"])
> +        self.assertEqual(result_size["height"], self.max["height"])
> +        self.assertEqual(result_size["width"], self.expected_size["width"])
> +        self.assertEqual(result_size["height"], self.expected_size["height"])

You should better compare expected_size with max, because that's what we interested in.

Keep in mind that if result_size is wrong, you don't know where we fail.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:238
(Diff revision 2)
>          self.assertTrue(self.marionette.execute_script("return window.fullscreenElement == null",
>                                                          sandbox=None))
> -        self.assertEqual(result["width"], self.max["width"] - 100)
> -        self.assertEqual(result["height"], self.max["height"] - 100)
> +        self.assertEqual(result_size["width"], self.max["width"] - 100)
> +        self.assertEqual(result_size["height"], self.max["height"] - 100)
> +        self.assertEqual(result_size["width"], self.expected_size["width"])
> +        self.assertEqual(result_size["height"], self.expected_size["height"])

Same as above.

  

  



        Attachment #8974311 -
        Flags: review?(hskupin) → review-

    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review248492

> Error: 'warnings' imported but unused [flake8: F401]

Can you please close this issue? I don't have permissions to do so.

  

  



    
    

    
  
The try results can be found here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8acade37095c25e874609bf05165774635f7eb3a

  

  



    
    

    
  
(In reply to Henrik Skupin (:whimboo) from comment #62)
> Venkatesh, I don't know why mozreview always creates a complete new review
> request / attachment for every time you upload something. For reviewers it
> means they have to re-review the whole patch because no interdiff is
> available. I will do that now, but for the next bug you want to work on, we
> should make sure that this doesn't happen anymore. Thanks!

I did not realise it was happening.  Sorry for that.  What can I do to help?

  

  



    
    

    
      
  

    
    

    
  
(In reply to Venkatesh from comment #66)
> I did not realise it was happening.  Sorry for that.  What can I do to help?

The way you did the last update of the patch was fine. Whatever was the difference to previous ones... As you can see in the comment before there is a link to https://reviewboard.mozilla.org/r/242630/diff/2-3/ which is the interdiff I usually have a look at. Generally make sure to keep the mozreview id in the commit message when updating/rebasing/histediting commits. And more importantly push to mozreview and publish from there instead from the command line. That way you can check the patch again if everything is alright before publishing the changes.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review249278

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:208
(Diff revisions 2 - 3)
>  
>          # in X the window size may be greater than the bounds of the screen
> -        self.assertGreaterEqual(new_size["width"], self.max["width"])
> -        self.assertGreaterEqual(new_size["height"], self.max["height"])
> -        self.assertEqual(new_size["width"], actual_size["width"])
> -        self.assertEqual(new_size["height"], actual_size["width"])
> +        self.assertGreaterEqual(self.max["width"], new_size["width"])
> +        self.assertGreaterEqual(self.max["height"], new_size["height"])
> +        self.assertGreaterEqual(self.max["width"], actual_size["width"])
> +        self.assertGreaterEqual(self.max["height"], actual_size["height"])

Why are you flipping the arguments of the assert method here? As mentioned in the comment above the window size might be larger than the max and not the other way around.

  

  



        Attachment #8974311 -
        Flags: review?(hskupin) → review-

    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review249278

> Why are you flipping the arguments of the assert method here? As mentioned in the comment above the window size might be larger than the max and not the other way around.

From the ./mach marionette test -z I run, I get:

new_size == actual_size -> PASS
new_size >= max -> PASS

So, max >= new_size is a fail.  For example, max[width] == 1356 and new_size[width] == 1366.  My screen resolution is 1366x768 (running Debian unstable, GNOME, updated/upgraded once every week to ten days).

I think what I am asking is, is 'window size' in the comment mapping to new_size/actual_size and 'bounds of the screen' to max?  Or should the mapping be flipped around?

  

  



    
    

    
  
(In reply to Venkatesh from comment #70)
> So, max >= new_size is a fail.  For example, max[width] == 1356 and
> new_size[width] == 1366.  My screen resolution is 1366x768 (running Debian
> unstable, GNOME, updated/upgraded once every week to ten days).

Correction: I meant to say max[width] == 1366, new_size[width] == 1356.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review249278

> From the ./mach marionette test -z I run, I get:
> 
> new_size == actual_size -> PASS
> new_size >= max -> PASS
> 
> So, max >= new_size is a fail.  For example, max[width] == 1356 and new_size[width] == 1366.  My screen resolution is 1366x768 (running Debian unstable, GNOME, updated/upgraded once every week to ten days).
> 
> I think what I am asking is, is 'window size' in the comment mapping to new_size/actual_size and 'bounds of the screen' to max?  Or should the mapping be flipped around?

So what you also might want to try is to run this test not in headless mode. You probably will get other results.

What I basically wonder is why was the assertion passing before with the values exchanged. Please run the original test without your modifications and check what that reports, and if it passes.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review249278

> So what you also might want to try is to run this test not in headless mode. You probably will get other results.
> 
> What I basically wonder is why was the assertion passing before with the values exchanged. Please run the original test without your modifications and check what that reports, and if it passes.

Below is a paste of failure message for height.

FAILED TESTS
-------
 7:04.33 INFO test_window_rect.py test_window_rect.TestWindowRect.test_resize_larger_than_screen
 7:04.33 SUITE_END

marionette-test
~~~~~~~~~~~~~~~
Ran 902 checks (902 tests)
Expected results: 853
Skipped: 48 tests
Unexpected results: 1
  test: 1 (1 fail)

Unexpected Results
------------------
FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768
Traceback (most recent call last):
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
    testMethod()
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py", line 206, in test_resize_larger_than_screen
    self.assertGreaterEqual(actual_size["height"], self.max["height"])


width comparison fails similarly as well.

new_size == actual_size passes.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review249278

> Below is a paste of failure message for height.
> 
> FAILED TESTS
> -------
>  7:04.33 INFO test_window_rect.py test_window_rect.TestWindowRect.test_resize_larger_than_screen
>  7:04.33 SUITE_END
> 
> marionette-test
> ~~~~~~~~~~~~~~~
> Ran 902 checks (902 tests)
> Expected results: 853
> Skipped: 48 tests
> Unexpected results: 1
>   test: 1 (1 fail)
> 
> Unexpected Results
> ------------------
> FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768
> Traceback (most recent call last):
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
>     testMethod()
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py", line 206, in test_resize_larger_than_screen
>     self.assertGreaterEqual(actual_size["height"], self.max["height"])
> 
> 
> width comparison fails similarly as well.
> 
> new_size == actual_size passes.

Running not in headless has more failures than running in headless.

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_move_to_negative_coordinates - AssertionError: 27 not less than or equal to 0

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 704 not greater than or equal to 741

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_set_position - AssertionError: 0 != 10
Traceback (most recent call last):

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py TestWindowMaximize.test_maximize - AssertionError: Window height is not within 22 px of availHeight: current height 704 should be greater than or equal to max height 719

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py TestWindowMaximize.test_maximize_twice_is_idempotent - AssertionError: Window height is not within 22 px of availHeight: current height 704 should be greater than or equal to max height 719

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_maximize.py TestWindowMaximize.test_stress - AssertionError: Window height is not within 22 px of availHeight: current height 704 should be greater than or equal to max height 719

  

  



    
    

    
  
Andreas, given that you wrote all the window resize tests, could you have a look at the last comment from Venkatesh? Flipping the max and expected looks strange, but also that tests fail without the modifications.

  

  


Flags: needinfo?(ato)

    
    

    
  
Which OS are you running the tests on?  Window resizing is to a
large degree dependent on the window manager (WM) and the desktop
environment (DE) that you use.  Some things which may be possible
in the DE we use on try might not be possible to you locally.

The characteristics of your environment and the requirement to have
tests passing in other environments unlike your own restrict how
the tests can be written.  In some cases the assertions have to be
relaxed to encompass a larger range, and in other cases it makes
more sense to use two different assertion values depending on the
system (macOS vs. Linux vs. Windows for example).

When you run the tests on Linux, you need to make sure you run them
in an xvfb with a WM that supports the same capabilities we use on
try.  I don’t know enough about our configuration on try to say
what that is, but I know it uses xvfb and some kind of simple WM.

The headless tests should pass regardless, so I would focus my time
on fixing those problems first.  Then for the other configurations
I would use try to make sure the tests pass.

  

  


Flags: needinfo?(ato)

    
    

    
  
How can I run the tests on try server myself?  I am thinking it will be better to submit next review after passing the tests on try server.

  

  


Flags: needinfo?(hskupin)

    
    

    
  
We discussed on IRC. So I will trigger it for you once the changes have been uploaded.

  

  


Flags: needinfo?(hskupin)

    
    

    
      
  

    
    

    
  
I uploaded a patch/diff to the review board sometime in the last week.  Did it pass the tests on the try server?  How or where can I see them results?

  

  


Flags: needinfo?(hskupin)

    
    

    
  
I was out the last days but will have a look at this bug later today. Thanks for the update.

  

  


Flags: needinfo?(hskupin)

    
    

    
  
I forgot to mention that I submitted a try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3cbeebb14f223fc46b8007d7cb215371715c027

Please check for the results later.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253014

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:59
(Diff revision 4)
> +        self.assertIsInstance(rect["y"], int)
> +        self.assertIsInstance(rect["height"], int)
> +        self.assertIsInstance(rect["width"], int)
>  
>      def test_set_types(self):
> -        for x, y in (["a", "b"], [1.2, 3.4], [True, False], [[], []], [{}, {}]):
> +        illegal_rects = (["a", "b", "h", "w"],

Mind changing the variable name to `invalid_rects`?

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:184
(Diff revision 4)
>          # but we don't have this information.  It should be safe to
>          # assume a window can be moved to (0,0) or less.
>          elif os == "linux":
>              # certain WMs prohibit windows from being moved off-screen
> -            self.assertLessEqual(position["x"], 0)
> -            self.assertLessEqual(position["y"], 0)
> +            self.assertGreaterEqual(new_position["x"], 0)
> +            self.assertGreaterEqual(new_position["y"], 0)

I still think this needs to be `assertLessEqual`. In your case and in automation we have a WM which might set it to 0, and as such all the tests are passing, but would fail if a WM allows -8/-8.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:191
(Diff revision 4)
>          # On macOS, windows can only be moved off the screen on the
>          # horizontal axis.  The system menu bar also blocks windows from
>          # being moved to (0,0).
>          elif os == "darwin":
> -            self.assertEqual(-8, position["x"])
> -            self.assertEqual(23, position["y"])
> +            self.assertEqual(-8, new_position["x"])
> +            self.assertEqual(23, new_position["y"])

Hm, interestingly this fails for me on OS X in non-headless mode with:

> AssertionError: 23 != 22

But this failure has not been introduced with your patch, so it would need to be investigated in a different bug.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:206
(Diff revision 4)
>              self.max["width"] * 2, self.max["height"] * 2)
> -        new = self.marionette.window_size
> +        actual_size = self.marionette.window_rect
>  
>          # in X the window size may be greater than the bounds of the screen
> -        self.assertGreaterEqual(new["width"], self.max["width"])
> -        self.assertGreaterEqual(new["height"], self.max["height"])
> +        self.assertGreaterEqual(self.max["width"], new_size["width"])
> +        self.assertGreaterEqual(self.max["height"], new_size["height"])

Please revert the order of the arguments in both methods. Your changes shouldn't have modified any of the existent expectations.

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:216
(Diff revision 4)
> -        result = self.marionette.set_window_rect(width=self.max['width'],
> +        expected_size = self.marionette.set_window_rect(width=self.max["width"],
> -                                                 height=self.max["height"])
> +                                                        height=self.max["height"])
> -        self.assertEqual(result["width"], self.max["width"])
> -        self.assertEqual(result["height"], self.max["height"])
> +        result_size = self.marionette.window_rect
> +
> +        self.assertGreaterEqual(self.max["width"], expected_size["width"])
> +        self.assertGreaterEqual(self.max["height"], expected_size["height"])

We resize to the available screensize, and as such it should not be greater. Please revert the changes here.

Also as best do not close all the issues. At least not those were a discussion is pending.

  

  



        Attachment #8974311 -
        Flags: review?(hskupin) → review-

    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253014

> Mind changing the variable name to `invalid_rects`?

Changed.

> I still think this needs to be `assertLessEqual`. In your case and in automation we have a WM which might set it to 0, and as such all the tests are passing, but would fail if a WM allows -8/-8.

Reverted.

> We resize to the available screensize, and as such it should not be greater. Please revert the changes here.
> 
> Also as best do not close all the issues. At least not those were a discussion is pending.

Reverted.

  

  



    
    

    
      
  

    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253014

> Please revert the order of the arguments in both methods. Your changes shouldn't have modified any of the existent expectations.

I still get failure on both height and width.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253014

> I still get failure on both height and width.

Mind pasting the exact failure with the new code?

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253518

While a new try build is running please fix the commit message. If all tests are passing we should land your patch, and move any other test failure out to a new bug.

::: commit-message-0cd10:5
(Diff revision 5)
> +Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.
> +
> +MozReview-Commit-ID: 83Z4dpIbwFQ
> +***
> +Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

Somehow those two additional lines sneaked in.

  

  



        Attachment #8974311 -
        Flags: review?(hskupin) → review-

    
    

    
      
  

    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253518

> Somehow those two additional lines sneaked in.

Ah, removed and pushed.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253014

> Mind pasting the exact failure with the new code?

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 1356 not greater than or equal to 1366

FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review253014

> FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 1356 not greater than or equal to 1366
> 
> FAIL testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py TestWindowRect.test_resize_larger_than_screen - AssertionError: 758 not greater than or equal to 768

For the failure details it would be great to also get the stack so I can see in which line this is happening, and the trace log to see which data got transferred. 

But for now can I assume that your screen resolution is 1366x768?

  

  



    
    

    
  
Maybe you have already seen the results from the try build. If not here those are:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6649c42bca03b278f4ddea5cdf402acf58915941

As you can see there are failures across platforms for the test `test_resize_larger_than_screen`. Lets take the example for Linux:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6649c42bca03b278f4ddea5cdf402acf58915941&selectedJob=180697744

Via an execute_script call we retrieve the screen max values:

https://treeherder.mozilla.org/logviewer.html#?job_id=180697744&repo=try&lineNumber=25470-25471

Those return 1600x1200. Then we set a start size of 1280x1040. As next step the 2xmax size should be set for both width and height which should be 3200x2400, but please check what is done:

https://treeherder.mozilla.org/logviewer.html#?job_id=180697744&repo=try&lineNumber=25474

> [0,8,"WebDriver:SetWindowRect",{"y":2400,"x":3200,"width":null,"height":null}]

We do not send a command to adjust the size but to move the window to a new position. And as such the assertions afterward fail as expected. It means you have to fix the arguments for the `set_window_rect` call.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review254604

::: testing/marionette/harness/marionette_harness/tests/unit/test_window_rect.py:201
(Diff revision 6)
> -        self.assertEqual(old["width"], new["width"])
> -        self.assertEqual(old["height"], new["height"])
>  
>      def test_resize_larger_than_screen(self):
> -        self.marionette.set_window_size(
> +        new_size = self.marionette.set_window_rect(
>              self.max["width"] * 2, self.max["height"] * 2)

As pointed out this call is wrong.

  

  



        Attachment #8974311 -
        Flags: review?(hskupin) → review-

    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review254604

> As pointed out this call is wrong.

Edited every set_window_rect call to explicitly naming arguments to match parameters.  No more tests fail.

I will learn to navigate the try server output better.

  

  



    
    

    
      
  

        Attachment #8967606 -
        Flags: review?(hskupin)

    
    

    
  
(In reply to Venkatesh from comment #95)
> Edited every set_window_rect call to explicitly naming arguments to match
> parameters.  No more tests fail.

Great to hear that! I will trigger another try build to verify that.

> I will learn to navigate the try server output better.

Let me know if you have questions in regards of that or anything else. I'm happy to explain.

  

  



    
    

    
  
Comment on attachment 8974311 [details]
Bug 1348145 - Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize.

https://reviewboard.mozilla.org/r/242630/#review254988

It looks all great now! Thanks a lot for all your work on this bug. We can finally land your changes.

  

  



        Attachment #8974311 -
        Flags: review?(hskupin) → review+

    
    

    
  
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/70c13cd61037
Remove getWindowPosition/setWindowPosition and getWindowSize/setWindowSize. r=whimboo

  

  



    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/70c13cd61037

  

  


Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox62:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla62

    
    

    
  
Sorry for the wrong information on IRC regarding the uplift request. We indeed need it on beta and for esr60. For now lets uplift to beta first. Thanks.

  

  



          status-firefox60:
          wontfix → ---

          status-firefox61:
          wontfix → affected

          status-firefox-esr60:
          --- → affected
Whiteboard: [lang=py][lang=js] → [lang=py][lang=js][checkin-needed-beta]

    
    

    
  
https://hg.mozilla.org/releases/mozilla-beta/rev/e4e135dd13f5

  

  



          status-firefox61:
          affected → fixed
Whiteboard: [lang=py][lang=js][checkin-needed-beta] → [lang=py][lang=js]

    
    

    
  
Beta looks fine so please also uplift the test-only patch to esr60.

  

  


Whiteboard: [lang=py][lang=js] → [lang=py][lang=js][checkin-needed-esr60]

    
    

    
  
https://hg.mozilla.org/releases/mozilla-esr60/rev/af365070668f

  

  



          status-firefox-esr60:
          affected → fixed
Whiteboard: [lang=py][lang=js][checkin-needed-esr60] → [lang=py][lang=js]

    
    

    
  
https://hg.mozilla.org/releases/mozilla-esr60/rev/ef91a3faffbf

  

  



    
  
Product: Testing → Remote Protocol

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: