803492 - Enhance the constructor of MozMillController to set focus on the window

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect, P1)

Description

[Maniac Vlad Florin (:vladmaniac)]

See https://bugzilla.mozilla.org/show_bug.cgi?id=758187#c70
for details.

We want to call window.focus() to 'virtually' give focus to the window even if its in the background.

Comment 1

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

I will take this.

Comment 2

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

Attachment: Patch 1.5 (v1)

Pointer to Github pull-request

Comment 3

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

Attachment: Pointer to Github pull request: https://github.com/mozilla/mozmill/pull/111#attach-to-bugzilla

Pointer to Github pull-request

Comment 4

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

Comment on attachment 673227 [details]
Patch 1.5 (v1)

This will do it. Vlad please test it across platforms and with your geo location test. If it passes and we do not regress something we can get a new version released.

Comment 5

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

Comment on attachment 673227 [details]
Patch 1.5 (v1)

The patch works fine on OS X. No failing tests and the new geolocation test passes too. I think it's ready for review.

Comment 6

[Maniac Vlad Florin (:vladmaniac)]

Comment on attachment 673227 [details]
Patch 1.5 (v1)

works just fine!

Comment 7

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

Comment on attachment 673227 [details]
Patch 1.5 (v1)

Clint is out. So lets try with Andrew and hopefully we can have a release for Mozmill 1.5 later today.

Comment 8

[Andrew Halberstadt [:ahal]]

Comment on attachment 673227 [details]
Patch 1.5 (v1)

Lgtm. Just wondering if there might be side effects on tests with specific focus requirements. Was it also tested with testmode disabled?

Comment 9

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

(In reply to Andrew Halberstadt [:ahal] from comment #8)
> Lgtm. Just wondering if there might be side effects on tests with specific
> focus requirements.

You mean when a window e.g. popup gets opened in the background? It's indeed a valid point. Looks like we should work on the Mozmill 2.0 patch first and get some tests implemented.

> Was it also tested with testmode disabled?

No, given that we will never run Mozmill tests with testmode disabled.

Comment 10

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

Andreea, I kinda would like to include this fix in the upcoming Mozmill 1.5.21. Can you please coordinate excessive testing around it? Especially the behavior when Firefox runs in the background. Does this extra focus() call bring the window into the forground or not? I would like to see how it works across platforms. Thanks.

Comment 11

[Daniela Petrovici]

Without the testGeolocation/testShareLocation.js the results are the same on Linux, Windows and Mac:
- with and without this.window.focus(); - tests passed
- with the above changes and with focusmanager.testmode set to TRUE and FALSE - tests passed

With the testGeolocation/testShareLocation.js the results are different:
- the test fails once out of 10 times on MAC without this.window.focus(); change
- the test passes every time out of 10 times on MAC with this.window.focus(); change
- the test passes every time out of 11 times on Linux with and without this.window.focus(); change
- the test fails intermittently on Windows with this.window.focus(); change - it fails once out of 5 times

We have tested with the Firefox window in background and the window does not get into foreground on either platform no matter the settings used.

On Linux and Mac, the popups notifications are visible in the foreground when the Firefox windows is in background, but on Windows this does not happen.

Comment 12

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

The good thing is that it will at least fix a long standing failure with the master password test (bug 611455). Even we can't fix the geolocation test it's probably very helpful to push this change with 1.5.21.

Comment 13

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

Landed the pieces for Mozmill 1.5:
https://github.com/mozilla/mozmill/commit/68309ed0986c5cf378ba07129ecb02311b4b1fcb

Jonathan, would you be interested to work on the fix for Mozmill 2.0 and the appropriate tests via Mutt? I believe it would be a very good task for you.

Comment 14

[Jonathan French (:jfrench)]

Sure, will have a go, sounds fun. Is it time-sensitive? As I may not be able to get to it until early next week.

Comment 15

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

No, no rush necessary here. The important fix for Mozmill 1.5 is done and checked in. So we are fine. But thanks for letting me know!

Comment 16

[Jonathan French (:jfrench)]

I will have a couple of backports to do first next week for bug 842058 and bug 790597, so will be moving this bug behind those in the queue. If anyone wants to pick this bug up first, feel free to do so, otherwise I will do this after the above ones.

Comment 17

[Jonathan French (:jfrench)]

So, I had a look at all files which contain the string MozMillController, in master.

mozmill/mozmill/extension/resource/driver/controller.js
mozmill/mozmill/extension/resource/driver/mozmill.js
mutt/mutt/tests/js/utils/pageload.js

Based on the initial patch for hotfix-1.5(to just one file, controller.js), and a review of the above files, it appears the only fix for master is to simply integrate that same change to its controller.js. If everyone is in agreement let me know and I will do that, and do a test run with the desired build(s).

Comment 18

[Jonathan French (:jfrench)]

Ok, well I've made the change based on my understandings above, and could produce a patch if required. I've been trying to sort out a variety of problems setting up mozmill 2.0rc1 and running the mutt tests. The js mutt tests don't run successfully, either in a clean clone of master, or in my branch of master with the change, with mozmill 2.0rc1 on windows. I assume it's something to do with my l10n test shell environment. I will have to pursue this and sort it out.

When I sort it out, I won't be able to speak for the patch behaviour Linux or Mac unfortunately due to available machine resources.

btw. I was unable to find this file in any branch of the current mozmill-tests or in master, if someone can clarify where it lives
testGeolocation/testShareLocation.js

If you need the patch in the meantime, let me know.

Comment 19

[Jonathan French (:jfrench)]

I've established the first problem I'm seeing, doesn't appear to be related to my environment. I've entered it as a separate issue, bug 846007. I will continue the mutt test run on my branch with the constructor change applied, and ensure it is at least 'no-worse' in results than a clean master (ie. pre-change).

Comment 20

[Jonathan French (:jfrench)]

Ok, so I've done a local test run, running with mozmill 2.0rc1 plus the controller change, with a local sync of the main mozmill tests from http://hg.mozilla.org/qa/mozmill-tests. I am assuming 2.0 is intended to run with those tests.

I have confirmed all the tests pass where expected, and also fail where expected, compared with a baseline run of mozmill 2.0.rc1 master(ie. without the controller change). There are no differences between the two runs.

Given there are no differences in the two runs, this implies that the change has no unexpected effects in mozmill 2.0.

As far as mutt tests, they didn't appear to have any different behaviour either, after sleuthing out that earlier unrelated mutt failure.

I'm not sure if I've done everything required, but if there is anything else before providing a patch, let me know. I am still unclear where testGeolocation/testShareLocation.js referenced in comment #11, resides, or where/what that repo is. It isn't in the repos I'm aware of so far.

Tested with 19.0 Release 20130215130331. Tests pass where expected.

Comment 21

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

Sounds good! Beside the change for the controller it would be great to also get some tests which ensure the correct behavior. Do you have any proposals about tests which might be useful?

For the referenced test please see the open dependency in the blocker section of this bug, which is bug 758187. But as we have heard there are still issues with the patch in place, at least for Mozmill 1.5.21.

Comment 22

[Jonathan French (:jfrench)]

If the question for proposals was directed at me rather than everyone else in the cc' list, no I'm sorry I don't have any additional suggestions.

Unrelated question, is there any documentation ho you 'attached' your 1.5 patch from your Github account? (ie. the 1.5 attachment above, when clicked on is a redirect to your pull request).

I ask as the document here
https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Ready_for_a_review
seems to indicate one would just produce an ascii git diff and attach it.

Comment 23

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

(In reply to Jonathan French (:jfrench) from comment #22)
> If the question for proposals was directed at me rather than everyone else
> in the cc' list, no I'm sorry I don't have any additional suggestions.

So something we should at least test is that when we create a new controller instance of a popup window which has been opened in the background, that it will not be brought into the foreground. Means the order of open windows is the same before and after the constructor call. It can be checked with the window enumerator, or with one of the helper functions in utils.js (mozmill code), which returns an array of the open windows. 

> Unrelated question, is there any documentation ho you 'attached' your 1.5
> patch from your Github account? (ie. the 1.5 attachment above, when clicked
> on is a redirect to your pull request).

You can use "git show --format='From: %an <%ae>%n%s%n%b'" to export the changes and forward to a file, which you then can attach.

> I ask as the document here
> https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/
> RepoSetup#Ready_for_a_review
> seems to indicate one would just produce an ascii git diff and attach it.

We should update that content with the above command. Would you mind doing it if it works for you?

Comment 24

[Jonathan French (:jfrench)]

Sure, I will test this out with several of the popup tests to make sure the window stack is being respected, and report back. 

Thanks for the git workflow, I will use that along with a pull request once the blocker bug is gone and/or you guys want to update 2.0 with this change. I will also update the wiki after I've tested it and confirmed it works.

Comment 25

[Jonathan French (:jfrench)]

Here's the exact output on a dummy repo, using that show format. If the output header looks correct to you, let me know, I'll update the wiki with the git syntax.

$ git show --format='From: %an <%ae>%s%n%b'
--- beginning of file here ---
From: Jonathan French <[e-mail]>added fum to foo


diff --git a/foo b/foo
index 257cc56..bdd5099 100644
--- a/foo
+++ b/foo
@@ -1 +1,2 @@
 foo
+fum

--- end of file here ---

Comment 26

[Jonathan French (:jfrench)]

Sorry.. missed a newline. One moment.

Comment 27

[Jonathan French (:jfrench)]

Here's the actual output format result.

$ git show --format='From: %an <%ae>%n%s%n%b'
--- beginning of file here ---
From: Jonathan French <[e-mail]>
added fum to foo


diff --git a/foo b/foo
index 257cc56..bdd5099 100644
--- a/foo
+++ b/foo
@@ -1 +1,2 @@
 foo
+fum

--- end of file here ---

Comment 28

[Jonathan French (:jfrench)]

> (In reply to Jonathan French (:jfrench) from comment #22)
> You can use "git show --format='From: %an <%ae>%n%s%n%b'" to export the
> changes and forward to a file, which you then can attach.

I'm still curious about how you produced the "Patch (1.5)" attachment, uploaded with this bug, though.

When you click on the attachment link above, it will spawn this:
"You can review this patch at https://github.com/mozilla/mozmill/pull/111, or wait 5 seconds to be redirected there automatically."

Was the content of this attachment, just a one-line file containing the url pointing to the pull request?

However it was created, I will probably do the same for 2.0, since I have a branch with the change on Github.

Comment 29

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> (In reply to Jonathan French (:jfrench) from comment #22)
> > If the question for proposals was directed at me rather than everyone else
> > in the cc' list, no I'm sorry I don't have any additional suggestions.
> 
> So something we should at least test is that when we create a new controller
> instance of a popup window which has been opened in the background, that it
> will not be brought into the foreground. Means the order of open windows is
> the same before and after the constructor call. It can be checked with the
> window enumerator, or with one of the helper functions in utils.js (mozmill
> code), which returns an array of the open windows. 

In re-reading this part, if you're suggesting the authoring of a new test for this scenario, I defer to others in the cc' list with a knowledge of the architecture for that. Perhaps that can be split out into a separate bug.

Comment 30

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

We should land both at the same time. So no worries then. We will find someone who can work on a fix.

Adjusting severity given that this is not an enhancement but a very productive fix on the 1.5 branch. We absolutely need this in mozmill 2.0.

Comment 31

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

Actually lets put this into the next weeks sprint.

Comment 32

[Jonathan French (:jfrench)]

Thanks Henrik.

Comment 33

[Jonathan French (:jfrench)]

(In reply to Henrik Skupin (:whimboo) from comment #23)
> 
> You can use "git show --format='From: %an <%ae>%n%s%n%b'" to export the
> changes and forward to a file, which you then can attach.
>
> We should update that content with the above command.

Wiki has been updated
https://wiki.mozilla.org/Auto-tools/Projects/Mozmill/RepoSetup#Ready_for_a_review

Comment 34

[Andrei Eftimie]

Neil,
Do you happen to know if the following is correct and intended behaviour?

--
TLDR: Firefox bleeds some content through windows that are drawn on top. Is that intended?
--

We've changed Mozmill to focus any new opened windows. This allows Mozmill to run Firefox tests in the background (other windows at OS level can be in front of Firefox).

While doing this certain elements bleed through any other windows that are on top.
Initially I thought that all of them are probably drawn at the OS level, hence the bleeding and that it is probably out of our reach.

Bleedthrough:
 - Panels (Download, SSL
 - Autocomplete (form items)
 - Dropdown Options (menulist)
 - Right Click context menus
 - LocationBar autocomplete
 - Search Engine list & suggestions

Stack:
 - Download Manager (old)

Demo (screencast): http://www.youtube.com/watch?v=StF_JdNlxYg

Interesting items:
00:37 Location Bar history
00:57 Favicon Autocomplete
01:08 Right Click menu
01:11 (Suggestions) StarIn Autocomplete
01:44 Download Manager (window)
02:20 Input Autocomplete
02:32 Password Manager Notification
02:45 Dropdown (menulist) options
03:45 Download Panel Indocator
03:51 Search Engine Manager
04:19 Search Suggestions
04:35 SSL Certificate panel
06:00 TabGroups context menu

Comment 35

[Neil Deakin]

I don't see anything unusual in the video. If you're asking if panels appear on top of other windows, then yes, that is expected, as panels appear on top of other windows on Mac.

Comment 36

[Andrei Eftimie]

Attachment: mutt test v0.1

I've written a basic mutt test, but I have some issues with getting it working.

I was unable to programatically change a windows ZLevel (or z-order) or open the a window directly in the background.

The gist of the test is:

- open a window, instantiate its controller
- open a second window
- grab the windows zlayer order
- move the second window in the back // have not been able to make this work
- grab the windowz zlayer order again, assert that they have been reversed
- instantiate the second window controller
- grab the windows zlayer order, assert that it has not changed from the previous value

I have also tried to open the second window directly in the background, with no
success either.

Please advise on how to proceed.

There is the setting "dom.disable_window_flip" (on by default) which disallows programatically changing the ZLevel. This should make our new window.focus() call safe.
Given the "dom.disable_window_flip" option was specifically made to disable what we are trying to test here, do we feel the need to have this covered in a test?

Comment 37

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

Comment on attachment 727192 [details] [diff] [review]
mutt test v0.1

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

First, I miss the change to the controller class to be able to run the test. Did you forget to add it? Other comments inline.

::: mutt/mutt/tests/js/controller/window_order.js
@@ +7,5 @@
> +const TEST_FOLDER = collector.addHttpResource('../_files/');
> +
> +var setupModule = function () {
> +  controller = mozmill.getBrowserController();
> +  Services.prefs.setBoolPref('dom.disable_window_flip', false);

There shouldn't be a need to change that pref. Please see the alwaysLowered feature of window.open():

https://developer.mozilla.org/en-US/docs/DOM/window.open

@@ +11,5 @@
> +  Services.prefs.setBoolPref('dom.disable_window_flip', false);
> +}
> +
> +var testMenu = function () {
> +  controller.open(TEST_FOLDER + "link.html");

Make a constant for the TEST_URL.

@@ +15,5 @@
> +  controller.open(TEST_FOLDER + "link.html");
> +  controller.waitForPageLoad();
> +
> +  // Open a popup
> +  var popupWindow = controller.window.open(TEST_FOLDER + "form.html");

If we do not have a testcase for popups in the repository we should add it.

Comment 38

[Andrei Eftimie]

Attachment: mutt test v0.2

Updated the test with Henrik's requests.
Test is functional now.

But we do have a problem with what we are trying to accomplish here:
This test _fails_ because setting this.window.focus() when initialising the controller does change the order of the windows.

This behaviour net being what we expected, how should we proceed?
1. Accept this as normal and scrap the test
2. Try to force our window to maintain its zLevel when focusing it (could this have other repercussions?)

**Not for this patch to run (see bug 855325) in the latest Nightly you either need to:
a) patch /jsbridge/jsbridge/extension/resource/modules/NSPR.jsm
-    file.append(ctypes.libraryName("nspr4"));
+    file.append(ctypes.libraryName("nss3"));

b) Use a version older then ~22nd March

Comment 39

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

(In reply to Andrei Eftimie from comment #38)
> But we do have a problem with what we are trying to accomplish here:
> This test _fails_ because setting this.window.focus() when initialising the
> controller does change the order of the windows.

Hm, strange. Not sure if that is the expected behavior or not in focus test mode. At least when I read the comment from Neil on bug 758187 comment 68 I can't find an answer for:

> Is the test framework intenionally opening Firefox as a background window?
> If so, you'll need to 'focus' the firefox window before running any tests by
> calling window.focus(). If focusmanager.testmode is enabled, this will set
> the window as focused internally without making it actually focused by the
> platform.

Neil, can you please tell us if even in test mode the background window will be put into the foreground? I got by reading your lines that this should not happen. Thanks.

Comment 40

[Andrei Eftimie]

Attachment: mutt test v0.3

couple of nits, and made sure it still applies

Comment 41

[Andrei Eftimie]

Attachment: mutt test v0.4

I've refactored the test to make it clearer:

We start with 1 window:
> starting window //controller initialized

We open 2 popups:
> starting window //controller initialized
> popup 1
> popup 2

We initialize popup 2 controller:
> starting window //controller initialized
> popup 1
> popup 2 //controller initialized

We save the window order

We initialize popup 1 controller:
> starting window //controller initialized
> popup 2 //controller initialized
> popup 1 //controller initialized

We check the final window order is the same the the initial one, and we fail, windows have been reordered.

====

I've written to Neil, but his bugzilla status says he'll be away untill the 10th of June, so we might not hear back for a while.

Comment 42

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

Comment on attachment 743538 [details] [diff] [review]
mutt test v0.4

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

I think that's a good approach and we should get this landed for the next RC3 of Mozmill. Andrei, can you please finalize the patch?

::: mutt/mutt/tests/js/_files/popup.html
@@ +1,3 @@
> +<!doctype html>
> +<html>
> +  <head>

I don't see a reason why we need that test file. Any exist file would be sufficient.

::: mutt/mutt/tests/js/controller/tests.ini
@@ +8,4 @@
>  [native_events.js]
>  [screenshot.js]
>  [synthesize_events.js]
> +[window_order.js]

I would call it window_focus.js

::: mutt/mutt/tests/js/controller/window_order.js
@@ +35,5 @@
> +  popupController1 = new mozmill.controller.MozMillController(popupWindow1);
> +
> +  // Check order after controller instantiation
> +  var enumerator = Services.wm.getZOrderDOMWindowEnumerator(null, false);
> +  finalWindowsOrder = [];

missing var declaration

@@ +43,5 @@
> +  }
> +
> +  // Check that the order has been maintained
> +  assert.deepEqual(intialWindowsOrder, finalWindowsOrder,
> +                   "Windows have not changed their order");

I would call it 'Z order' to make it clearer.

Comment 43

[Andreea Matei [:AndreeaMatei]]

I'll finish the work here since Andrei is in PTO this week.

Comment 44

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test v1

Updated the test with the required changes, one note: this will pass on Linux because .getZOrderDOMWindowEnumerator() is not working there (see bug 462222) and it compares 2 empty arrays. 
On OS X we have the expected fail, windows are reordered from [3, 17, 27] to [3, 27, 17] - ids.

Comment 45

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

Comment on attachment 746427 [details] [diff] [review]
mutt test v1

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

We are close. Lets get those two nits fixed.

::: mutt/mutt/tests/js/controller/tests.ini
@@ +8,5 @@
>  [native_events.js]
>  [screenshot.js]
>  [synthesize_events.js]
> +[window_focus.js]
> +expected = fail

Nope, we can't do that here. Instead please ensure we run this test on OS X and Windows only until the bug in Firefox has been fixed.

::: mutt/mutt/tests/js/controller/window_focus.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +const TEST_URL = collector.addHttpResource("../_files/") + "complex.html";

nit: BASE_URL and TEST_DATA please.

Comment 46

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test v2

Updated the manifest to skip the test if we're on linux and the constants in the test.

Comment 47

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

Comment on attachment 746863 [details] [diff] [review]
mutt test v2

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

r=me with the given comment fixed.

::: mutt/mutt/tests/js/controller/tests.ini
@@ +8,5 @@
>  [native_events.js]
>  [screenshot.js]
>  [synthesize_events.js]
> +[window_focus.js]
> +skip-if = os == 'linux'

Please add a comment why we are skipping Linux as of now.

Comment 48

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test v2.1

Solved!

Comment 49

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

Comment on attachment 746869 [details] [diff] [review]
mutt test v2.1

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

Looks good. We should get this landed.

Andreea, one more thought, I think it would be good to also have a test for bug 758187 in the Mozmill repository. That way we can make sure that popups which usually only opens when Firefox is focused are really opened. We can do that as a follow-up patch on that bug.

Comment 50

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

Comment on attachment 746869 [details] [diff] [review]
mutt test v2.1

This patch has been landed on master:
https://github.com/mozilla/mozmill/commit/78b5efd98c66dff9e0c3fcd2962bf51a91a7abad

Comment 51

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

Andreea, can you please check if Mozmill 2.0 in its current state can handle the geolocation notification now?

Comment 52

[Andreea Matei [:AndreeaMatei]]

I've checked with the current patch from bug 758187 (which is working on 1.5 but needs some updates as per review) and on OS X test passes, but on Linux it fails with waitForPageLoad(), while loading the position.html page. I'll check more, it's strange cause on OS X works. I also want to update the test in the other bug.

Comment 53

[Jonathan French (:jfrench)]

(In reply to Andreea Matei [:AndreeaMatei] from comment #52)
> I also want to update the test in the other bug.

I will do the same with my tests in bug 855427, when this bug and bug 758187 are sorted and ready to land.

Comment 54

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

(In reply to Andreea Matei [:AndreeaMatei] from comment #52)
> I've checked with the current patch from bug 758187 (which is working on 1.5
> but needs some updates as per review) and on OS X test passes, but on Linux
> it fails with waitForPageLoad(), while loading the position.html page. I'll

You can modify the window.js file inside the extension folder of Mozmill and remove the comments from the dump statements. That way you have a chance to see when events are fired for new pages to be loaded and when a waitForPageLoad was successful.

Comment 55

[cmtalbert]

(In reply to Henrik Skupin (:whimboo) from comment #50)
> Comment on attachment 746869 [details] [diff] [review]
> mutt test v2.1
> 
> This patch has been landed on master:
> https://github.com/mozilla/mozmill/commit/
> 78b5efd98c66dff9e0c3fcd2962bf51a91a7abad

What more is left here to do?

Comment 56

[Andreea Matei [:AndreeaMatei]]

We wanted to check how this works with the patch from bug 758187. I've updated the test there, checked on all platforms with 2.0 and it works as expected. There's an issue with closeAllTabs() on Linux and Windows (bug 875331) that makes the geolocation test to fail with waitForPageLoad(), but it's unrelated to this bug or the functionality of the test itself. 
So I think we're done here unless Henrik wants me to check something more.

2.0 reports:
Linux: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916165acd

OS X: http://mozmill-crowd.blargon7.com/#/functional/report/c93c51b0c0b1af1b0562392916167ad0

On windows I get bug 872414 failure that stops the testrun, so I removed the restartTests from the manifest to get the results, but we have another issue with the dashboard there for which I'll file a bug.

Comment 57

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

Sounds good. Lets close it in this case. Thanks Andreea.

Comment 58

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

Andrei, was the answer from Neil via email sufficient? If not what else is still questionable? The one thing I'm not sure about is the wrong order of windows given back by the enumerator on OS X.

Comment 59

[Andrei Eftimie]

The enumerator appears to be working fine in Windows and on OSX.
That's where this test fails.

The window order is changed by focusing a background window.
On Linux the test passes (probably due to the broken enumerator).

===
We've not heard back from Neil on this case. Lets try again.

Neil, can you please confirm that focusing a window that is in the background, when having focusmanager.testmode enabled, should not change the order of the windows.

We're seeing this behaviour using getZOrderDOMWindowEnumerator and not sure if it's intended behaviour or a bug.

Comment 60

[Neil Deakin]

Attachment: Patch for testing - don't call SetVisibility in testmode

Does this patch have any affect on this?

Comment 61

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

Comment on attachment 765942 [details] [diff] [review]
Patch for testing - don't call SetVisibility in testmode

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

I would suggest we move the discussion over to bug 887315, which has been filed yesterday to cover this outstanding issue. Otherwise one small question to the patch.

::: dom/base/nsFocusManager.cpp
@@ +686,5 @@
>      if (NS_SUCCEEDED(baseWindow->GetEnabled(&isEnabled)) && !isEnabled) {
>        return NS_ERROR_FAILURE;
>      }
>  
> +    if (!sTestMode) {

Neil, I assume this should be isTestMode?

Comment 62

[Neil Deakin]

(In reply to Henrik Skupin (:whimboo) [away 06/28 - 07/07] from comment #61)
> Neil, I assume this should be isTestMode?

No.