1123401 - Implement handling of chrome windows

Status: RESOLVED FIXED

(Testing :: Firefox UI Tests, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Until we can get bug 1121698 fixed, we have to get at least some workarounds into our code to handle chrome windows. The API to get implemented should be identical to the one for working with observers.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Given that e10s is currently causing test failures for us, I will check if we might have to disable it completely for now, or only the affected tests for BaseWindow.

A pull for preview is currently up at:
https://github.com/mozilla/firefox-ui-tests/pull/50

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually this problem came in with the patch from Chris due to bug 1122187. I wonder if we really should disable e10s for our tests, until the remaining problems are really fixed. Chris, what do you think about it?

Comment 3

[Chris Manchester (limited bugmail, email directly)]

We could turn it off as a last resort. I expect we're going to continue to find issues because of the large number of outstanding issues with e10s, but some of those will be bugs of interest to e10s, and more others will be bugs of interest to us conceiving of marionette in a world with e10s turned on everywhere.

If we find very severe marionette problems that impede progress, turning it off for a short time might be necessary. But we can't expect everything to work exactly as expected, if only because e10s isn't ready. Things might not be "really fixed" until it is. In the mean time, if we turn e10s off for the sake of the tests and libraries we are explicitly building for the sake of e10s, we run the risk of building something subtly e10s incompatible and making it much harder to turn back on later.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

We discussed and agreed that we should turn e10s off for a short time. After the first week in February, I'll revisit the status of known issues and prioritize re-enabling e10s in the harness.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: github_pull_request.txt

I uploaded the latest changes as a PR to Github. It would be good if you can have a look at it Chris. This is WIP and for now a feedback request. Lets me know what doesn't play well and were we need improvements.

Something which is still left to do from my side is:

* Move general BaseWindow open/close code into Windows open/close methods
* Return BaseWindow instances for open/close/switch/focus methods
* Fix unit and functional tests and add more complete ones for new Window classes
* Fix private browsing window checks
* Fix shortcut misbehavior when opening a new browser window

Comment 6

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8555500 [details] [review]
github_pull_request.txt

I think this mostly looks ok (comments in github). It probably makes sense to take this close to its current form and move on to tab handling for the moment.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8555500 [details] [review]
github_pull_request.txt

Everything has been updated to the feedback comments, and the remaining items be implemented. Please have a look at the patch Chris. Thanks.

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8555500 [details] [review]
github_pull_request.txt

I left a few more comments in Github, mostly nits. I left a note about the use of window handles. If that doesn't work out, please file a follow up along the lines of "Figure out how to identify the currently focused chrome window in marionette" to address that issue, but that's the only part I'm really worried about.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The PR has been merged as https://github.com/mozilla/firefox-ui-tests/commit/b534fd1e40b1db64f65cce9d7a0f30d25958497f