1114623 - Implement closeChromeWindow() to close the currently active chrome window

Status: RESOLVED FIXED

(Testing :: Mozbase, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

When there is a window with multiple tabs open, there is no way to directly close the window. Any call to self.marionette.close() will only close a tab. Once all tabs are closed the window will also be closed.

There has to be a way to specify that the window should be closed directly.

I have seen this problem in various of our firefox-greenlight tests, where we fail to correctly clean up the test due to multiple tabs open. One example can be found here:

https://github.com/chmanchester/firefox-greenlight-tests/blob/master/greenlight/tests/functional/privateBrowsing/test_about_private_browsing.py#L67

Comment 1

[Chris Manchester (limited bugmail, email directly)]

The spec on this[1] only refers to closing individual top level browser contexts. The ability to close an OS level window may need to live in our libraries if we don't want to special case this is marionette.

That test case and some others were broken due to relying on the old behavior, they need to be updated.

[1] https://dvcs.w3.org/hg/webdriver/raw-file/tip/webdriver-spec.html#widl-WebDriver-close-void

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Well, its similar to the window handles which behave differently in regards which context is currently set. So if chrome context is set close() might have to work on the chrome window but not the tab?

Lets ask David how to best cope with that. I think it should be Marionette proper and not in our library. At the very least a method like close_chrome_window() or whatever.

Comment 3

[David Burns :automatedtester]

The specification always describes how we should do things for Content.

For simplicity I think it would be better to have closeChromeWindow() as the server call and probably something similar on the client side

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

Thanks David! Updating the summary of the bug accordingly. Chris, could you work on this over the next week?

Comment 5

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: bz://1114623/chmanchester

Comment 6

[Chris Manchester (limited bugmail, email directly)]

/r/1665 - Bug 1114623 - Implement closeChromeWindow endpoint for marionette.

Pull down this commit:

hg pull review -r 7172d67408e0a0c40d676e56b46920d93c64c337

Comment 7

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29fe7034f0ef

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

https://reviewboard.mozilla.org/r/1663/#review1195

::: testing/marionette/marionette-server.js
(Diff revision 1)
> +      if (numOpenWindows === 1) {

How do we handle OS X here, where we are perfectly fine without any open chrome window?

::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
(Diff revision 1)
> +        self.assertEqual(len(self.marionette.window_handles), 1)

You closed the chrome window, so you may want to check via self.marionette.chrome_window_handles.

::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
(Diff revision 1)
> +        self.assertEqual(len(self.marionette.window_handles), 2)

Maybe you also want to test via self.marionette.chrome_window_handles to ensure the new chrome window has been opened.

::: testing/marionette/client/marionette/www/newTab.html
(Diff revision 1)
> +  <a href="about:blank" id="new-window" onClick='javascript:window.open("about:blank", null, "location=1,toolbar=1");'>Click me!</a>

I don't think this line fits into this testcase data file, which is about tabs.

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Thanks for the feedback Henrik, I'll update before requesting review. I have a question below.

(In reply to Henrik Skupin (:whimboo) from comment #8)
> https://reviewboard.mozilla.org/r/1663/#review1195
> 
> ::: testing/marionette/marionette-server.js
> (Diff revision 1)
> > +      if (numOpenWindows === 1) {
> 
> How do we handle OS X here, where we are perfectly fine without any open
> chrome window?
I don't know if I see what you mean here. I think you mean that on osx, the application is still running even after closing the windows associated with it. As I understand it running/stopping the application is taken care of by mozrunner, and isn't really a concern here. I based this off of the old implementation of marionette.close, which was based on OS level windows, but it's possible I misunderstand the intent of the session teardown behavior.
> 
> ::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
> (Diff revision 1)
> > +        self.assertEqual(len(self.marionette.window_handles), 1)
> 
> You closed the chrome window, so you may want to check via
> self.marionette.chrome_window_handles.
> 
> ::: testing/marionette/client/marionette/tests/unit/test_window_handles.py
> (Diff revision 1)
> > +        self.assertEqual(len(self.marionette.window_handles), 2)
> 
> Maybe you also want to test via self.marionette.chrome_window_handles to
> ensure the new chrome window has been opened.
> 
> ::: testing/marionette/client/marionette/www/newTab.html
> (Diff revision 1)
> > +  <a href="about:blank" id="new-window" onClick='javascript:window.open("about:blank", null, "location=1,toolbar=1");'>Click me!</a>
> 
> I don't think this line fits into this testcase data file, which is about
> tabs.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 10/03 - 10/13)]

(In reply to Chris Manchester [:chmanchester] from comment #9)
> > How do we handle OS X here, where we are perfectly fine without any open
> > chrome window?
> I don't know if I see what you mean here. I think you mean that on osx, the
> application is still running even after closing the windows associated with
> it. As I understand it running/stopping the application is taken care of by

Right, that's what I mean. Does killing the session mean that mozrunner will quit the application? If that is the case we have to get it fixed. So it might be worth checking. If it's a problem we need it fixed, and we may spun this out into a new bug.

Comment 11

[Chris Manchester (limited bugmail, email directly)]

/r/1665 - Bug 1114623 - Implement closeChromeWindow endpoint for marionette.

Pull down this commit:

hg pull review -r ae5c56baa5be679cc79fd2d5af1ec22d6cc0cd91

Comment 12

[Chris Manchester (limited bugmail, email directly)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85803b290e12

Comment 13

[David Burns :automatedtester]

https://reviewboard.mozilla.org/r/1663/#review1351

Ship It!

Comment 14

[Chris Manchester (limited bugmail, email directly)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1b76a42c910

Comment 15

[Carsten Book [:Tomcat]]

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

Comment 17

[Chris Manchester (limited bugmail, email directly)]

Attachment: MozReview Request: Bug 1114623 - Implement closeChromeWindow endpoint for marionette.