Implement closeChromeWindow() to close the currently active chrome window

RESOLVED FIXED in mozilla37

Status

Testing
Mozbase
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: whimboo, Assigned: chmanchester)

Tracking

(Blocks: 1 bug)

unspecified
mozilla37
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
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
(Assignee)

Comment 1

4 years ago
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
(Reporter)

Comment 2

4 years ago
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)
(Reporter)

Comment 4

4 years ago
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
(Assignee)

Comment 5

4 years ago
Created attachment 8540505 [details]
MozReview Request: bz://1114623/chmanchester
(Assignee)

Comment 6

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

Pull down this commit:

hg pull review -r 7172d67408e0a0c40d676e56b46920d93c64c337
(Reporter)

Comment 8

3 years ago
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.
(Assignee)

Comment 9

3 years ago
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.
(Reporter)

Comment 10

3 years ago
(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

3 years ago
Attachment #8540505 - Flags: review?(dburns)
(Assignee)

Comment 11

3 years ago
/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: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
(Reporter)

Updated

3 years ago
Blocks: 1120364
(Reporter)

Updated

3 years ago
Blocks: 1121698
(Assignee)

Comment 16

3 years ago
Comment on attachment 8540505 [details]
MozReview Request: bz://1114623/chmanchester
Attachment #8540505 - Attachment is obsolete: true
Attachment #8618961 - Flags: review+
(Assignee)

Comment 17

3 years ago
Created attachment 8618961 [details]
MozReview Request: Bug 1114623 - Implement closeChromeWindow endpoint for marionette.
You need to log in before you can comment on or make changes to this bug.