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

RESOLVED FIXED in mozilla10

Status

Testing
Reftest
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: cjones, Assigned: jdm)

Tracking

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.

Comment 2

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

7 years ago
What isn't working? What is currently focused?

Comment 4

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

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

7 years ago
Cool!

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

Comment 7

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

Comment 8

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

Comment 9

7 years ago
Created 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=

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

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

Comment 11

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

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

7 years ago
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 16

7 years ago
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+

Comment 17

7 years ago
Josh, are you planning to get back to this?  I'm not sure what the next step is...
(Assignee)

Comment 18

7 years ago
Yes, I'll fix this up according to Neil's suggestions in comment 16.
(Assignee)

Updated

7 years ago
Assignee: nobody → josh
(Assignee)

Comment 19

7 years ago
Created attachment 563772 [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.
Attachment #563772 - Flags: review?(enndeakin)
(Assignee)

Updated

7 years ago
Attachment #539657 - Attachment is obsolete: true

Updated

7 years ago
Attachment #563772 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

7 years ago
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/190e4e5b23c3
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Comment 22

7 years ago
Is setting the activeWindow property guaranteed to be synchronous?  Also, shouldn't we be setting focusedWindow instead?
Depends on: 842683
You need to log in before you can comment on or make changes to this bug.