Implement closeChromeWindow() to close the currently active chrome window

RESOLVED FIXED in mozilla37

Status

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: whimboo, Assigned: chmanchester)

Tracking

(Blocks 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
Summary: marionette.close() is not able close chrome window if multiple tabs are open → marionette.close() is not able to close a chrome window directly if multiple tabs are open
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
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.
Flags: needinfo?(dburns)
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
Flags: needinfo?(dburns)
Thanks David! Updating the summary of the bug accordingly. Chris, could you work on this over the next week?
Summary: marionette.close() is not able to close a chrome window directly if multiple tabs are open → Implement closeChromeWindow() to close the currently active chrome window
/r/1665 - Bug 1114623 - Implement closeChromeWindow endpoint for marionette.

Pull down this commit:

hg pull review -r 7172d67408e0a0c40d676e56b46920d93c64c337
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.
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.
(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.
Assignee

Updated

4 years ago
Attachment #8540505 - Flags: review?(dburns)
/r/1665 - Bug 1114623 - Implement closeChromeWindow endpoint for marionette.

Pull down this commit:

hg pull review -r ae5c56baa5be679cc79fd2d5af1ec22d6cc0cd91
Attachment #8540505 - Flags: review?(dburns) → review+
https://hg.mozilla.org/mozilla-central/rev/f1b76a42c910
Assignee: nobody → cmanchester
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Attachment #8540505 - Attachment is obsolete: true
Attachment #8618961 - Flags: review+
You need to log in before you can comment on or make changes to this bug.