Closed Bug 623625 Opened 9 years ago Closed 8 years ago

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

Categories

(Testing :: Reftest, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla10

People

(Reporter: cjones, Assigned: jdm)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
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.
What isn't working? What is currently focused?
(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.
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)
Cool!

Josh, this is needed very badly!  Is this something that you'd be interested in looking into?
What's the desired interface/method/flag/whatever here?
(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.
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.
This looks to do the job, I think, unless Neil tells us that RaiseWindow is asynchronous.
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.
(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?
It can. The paranthesized part of comment 5 was just an observation. I wasn't actually expecting you to change it at all.
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
Blocks: 583976
so can we request review for the patch or are there more changes necessary?
Attachment #539657 - Flags: review?(enndeakin)
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.
Attachment #539657 - Flags: review?(enndeakin) → review+
Josh, are you planning to get back to this?  I'm not sure what the next step is...
Yes, I'll fix this up according to Neil's suggestions in comment 16.
Assignee: nobody → josh
Attachment #539657 - Attachment is obsolete: true
Attachment #563772 - Flags: review?(enndeakin) → review+
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/190e4e5b23c3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Is setting the activeWindow property guaranteed to be synchronous?  Also, shouldn't we be setting focusedWindow instead?
You need to log in before you can comment on or make changes to this bug.