623625 - Implement code to detect whether reftest window has focus and/or acquire focus if not

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bug 615386 added the ability to skip tests when the reftest window is unfocused (or if focus is broken), but left the focus-detection/acquisition code FIXME.  I don't exactly know what's required to implement that; I hope someone else can grab it or give a spec here.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

dbaron notes in bug 615386 comment 62 that

Could you add a comment that we shouldn't add anything to the non-remote mode
that would return false, since then we could lose testing coverage due to
problems on the test machines?

So we probably need a way to force focus.  I would lean towards defaulting to checking the window state, since that's friendlier to developers, and having tinderbox set the pref or whatever.

Comment 2

[(no longer active)]

Neil, we have a bunch of reftests which need the window to be focused, so we need to fix this bug.  Can you please help here?

I once took a stab at this bug, but got lost in the focus manager.  I was trying to detect whether the main application window is focused, and focus it if it's not, before each test run (by stealing the code from SimpleTest.waitForFocus, but I couldn't get things to work.  Specifically, nothing that I tried could restore the focus back to the reftest window if I switched the focus away.

Comment 3

[Neil Deakin]

What isn't working? What is currently focused?

Comment 4

[(no longer active)]

(In reply to comment #3)
> What isn't working? What is currently focused?

When the reftest window is not focused and another window (from another application, say Terminal) is focused, calling window.focus() or focusManager.activeWindow = applicationWindow doesn't focus the reftest window.

Comment 5

[Neil Deakin]

On Mac, window.focus() only focuses the window if the application is active.

You can activate the application with nsIMacDockSupport::activateApplication

(That function would probably be better as a flag in nsIFocusManager)

Comment 6

[(no longer active)]

Cool!

Josh, this is needed very badly!  Is this something that you'd be interested in looking into?

Comment 7

[Josh Matthews [:jdm]]

What's the desired interface/method/flag/whatever here?

Comment 8

[(no longer active)]

(In reply to comment #7)
> What's the desired interface/method/flag/whatever here?

Here's what should happen.  For tests specified with the needs-focus in the reftest manifest file, we should check the focus state of the reftest window, and focus it before loading the page so that reftests which need focus are always loaded in a focused browser.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Force window to the front on mac, and make sure the app window is active on all platforms for needs-focus reftests. r=

This successfully brought the app window to the foreground for me on a needs-focus reftest, and the whole process seemed to work without a hitch. I'll see what happens with a full run of the suite.

Comment 10

[(no longer active)]

This looks to do the job, I think, unless Neil tells us that RaiseWindow is asynchronous.

Comment 11

[Neil Deakin]

I meant it would have been better to have added a flag such as FLAG_RAISEAPP similar to FLAG_RAISE (as switching applications should not be the default behaviour) rather than have had one focus-related api in a different component.

Comment 12

[(no longer active)]

(In reply to comment #11)
> I meant it would have been better to have added a flag such as FLAG_RAISEAPP
> similar to FLAG_RAISE (as switching applications should not be the default
> behaviour) rather than have had one focus-related api in a different component.

Can this wait for another bug?  I mean, can't we just call nsIMacDockSupport directly in the reftest framework code for now, in the interest of resolving this as soon as possible?

Comment 13

[Neil Deakin]

It can. The paranthesized part of comment 5 was just an observation. I wasn't actually expecting you to change it at all.

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Great patch.
With the recent changes in the patch for Bug 583976 (the remote content activation is now inside " if (mActiveWindow) .. " blocks, I also need this patch to be able to enable remote reftests that needs focus

Comment 15

[:Felipe Gomes (needinfo for replies!)]

so can we request review for the patch or are there more changes necessary?

Comment 16

[Neil Deakin]

Comment on attachment 539657 [details] [diff] [review]
Force window to the front on mac, and make sure the app window is active on all platforms for needs-focus reftests. r=

I think you should just do what Ehsan suggests in comment 12.

It would be very bad form on Mac to call ActivateApplication every time a window needed to be raised. It should only happen in the very rare case where a user is expecting to switch applications. (The existing usage of ActivateApplication is good as it is called from the dock menu) In addition, I believe this patch allows content to call this.

Comment 17

[(no longer active)]

Josh, are you planning to get back to this?  I'm not sure what the next step is...

Comment 18

[Josh Matthews [:jdm]]

Yes, I'll fix this up according to Neil's suggestions in comment 16.

Comment 19

[Josh Matthews [:jdm]]

Attachment: Force window to the front on mac, and make sure the app window is active on all platforms for needs-focus reftests.

Comment 20

[Josh Matthews [:jdm]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/190e4e5b23c3

Comment 21

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/190e4e5b23c3

Comment 22

[(no longer active)]

Is setting the activeWindow property guaranteed to be synchronous?  Also, shouldn't we be setting focusedWindow instead?