859339 - unfocus reftests that aren't marked as needs-focus to catch errors earlier

Status: RESOLVED FIXED

(Testing :: Reftest, defect)

Description

[Nathan Froyd [:froydnj]]

Ted pointed out in bug 858203 comment 4:

> It would be cool if we could add something to the reftest harness to explicitly *not*
> focus tests that aren't marked as needs-focus, to prevent these from creeping in.

I think this as simple as telling the focus manager to blur the window for non-needs-focus tests.

Comment 1

[(no longer active)]

It would also be nice to somehow make the focus manager never give you OS level focus no matter how badly you want it.  ;-)

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: unfocus reftests that declare they don't need focus

This sounded simple, and indeed this patch is simple, but it doesn't work right
on Mac:

https://tbpl.mozilla.org/?tree=Try&rev=fafada4fbca9

*Some* needs-focus tests don't work, but the majority of them do.  I don't
understand why.

Comment 3

[Nathan Froyd [:froydnj]]

dbaron, do you have any ideas why the simple-minded approach in the previously-attached patch doesn't work?

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: add a deactivateApplication method to nsIMacDockSupport

Since reftest.js treats Macs specially for focusing, it makes sense that we'd need to
treat it differently for blurring, too.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: blur reftests that don't require focus

A slightly updated iteration of the previous patch.  This works on Linux and Windows,
and Mac locally, but turns everything orange and/or red on Try for Mac.

Comment 6

[Nathan Froyd [:froydnj]]

Comment on attachment 759868 [details] [diff] [review]
blur reftests that don't require focus

Neil, as the resident focus expert, do you know what might be the problem here on Mac?  Example try run:

https://tbpl.mozilla.org/?tree=Try&rev=ad26c937ca9f

Comment 7

[Neil Deakin]

It likely means that those tests that fail actually do need focus. Looking at a few of them, it appears that they rely on the selection colour/ spellcheck highlight/etc which appear differently for a focused versus nonfocused window.

Note also that window.blur() means:
 - if no other mozilla window is open, do nothing
 - focus a different mozilla window

I suspect that isn't what you think it does. On Windows, for example, I'm not sure your patch is even doing anything.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Neil Deakin from comment #7)
> It likely means that those tests that fail actually do need focus. Looking
> at a few of them, it appears that they rely on the selection colour/
> spellcheck highlight/etc which appear differently for a focused versus
> nonfocused window.

Hm, OK.  But there are a lot of other tests that passed which also need focus.  I'm just curious how some tests got the focus and others didn't--maybe it part of the "blur doesn't do what I thought it does" problem.

> Note also that window.blur() means:
>  - if no other mozilla window is open, do nothing
>  - focus a different mozilla window
> 
> I suspect that isn't what you think it does. On Windows, for example, I'm
> not sure your patch is even doing anything.

OK.  So what's the right approach here?  I see nsIFocusManager::clearFocus, but IIUC, windows can still retain focus:

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIFocusManager.idl#106
http://mxr.mozilla.org/mozilla-central/source/browser/base/content/test/browser_tabfocus.js#135 (lovely comment there)

Do we have anything that completely removes OS-level focus from the app?

Comment 9

[Neil Deakin]

(In reply to Nathan Froyd (:froydnj) from comment #8)
> OK.  So what's the right approach here?  I see nsIFocusManager::clearFocus,
> but IIUC, windows can still retain focus:
> 

clearFocus is used to clear the focused element within the window. You want the window itself to be lowered, right?

> Do we have anything that completely removes OS-level focus from the app?

We do not.

Comment 10

[Nathan Froyd [:froydnj]]

(In reply to Neil Deakin from comment #9)
> (In reply to Nathan Froyd (:froydnj) from comment #8)
> > OK.  So what's the right approach here?  I see nsIFocusManager::clearFocus,
> > but IIUC, windows can still retain focus:
> 
> clearFocus is used to clear the focused element within the window. You want
> the window itself to be lowered, right?

What do you mean by "lowered" here?

Comment 11

[Neil Deakin]

(In reply to Nathan Froyd (:froydnj) from comment #10)
> 
> What do you mean by "lowered" here?

Lowered such that some other window is in front or the desktop is focused.

Comment 12

[Nathan Froyd [:froydnj]]

(In reply to Neil Deakin from comment #11)
> (In reply to Nathan Froyd (:froydnj) from comment #10)
> > 
> > What do you mean by "lowered" here?
> 
> Lowered such that some other window is in front or the desktop is focused.

That sounds like what I want, then.

Comment 13

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #12)
> (In reply to Neil Deakin from comment #11)
> > (In reply to Nathan Froyd (:froydnj) from comment #10)
> > > 
> > > What do you mean by "lowered" here?
> > 
> > Lowered such that some other window is in front or the desktop is focused.
> 
> That sounds like what I want, then.

...but I don't think it's actually implementable.  I looked through the Windows documentation and it didn't look like there was anything for programmatically lowering an app.  Hide/unhide doesn't seem to do the right thing on Mac on the test slaves.  I think it's doable with Linux, but 1/3 of our platforms working isn't cause for celebration.

I looked for things that do programmatic lowering in Gecko and there didn't seem to be anything there, either.

Neil, is this a totally lost cause?  Or is there some other clever way of giving up focus that we can use in reftests?

Comment 14

[(no longer active)]

(In reply to comment #13)
> (In reply to Nathan Froyd (:froydnj) from comment #12)
> > (In reply to Neil Deakin from comment #11)
> > > (In reply to Nathan Froyd (:froydnj) from comment #10)
> > > > 
> > > > What do you mean by "lowered" here?
> > > 
> > > Lowered such that some other window is in front or the desktop is focused.
> > 
> > That sounds like what I want, then.
> 
> ...but I don't think it's actually implementable.  I looked through the Windows
> documentation and it didn't look like there was anything for programmatically
> lowering an app.  Hide/unhide doesn't seem to do the right thing on Mac on the
> test slaves.  I think it's doable with Linux, but 1/3 of our platforms working
> isn't cause for celebration.
> 
> I looked for things that do programmatic lowering in Gecko and there didn't
> seem to be anything there, either.
> 
> Neil, is this a totally lost cause?  Or is there some other clever way of
> giving up focus that we can use in reftests?

Does SetWindowPos(HWND_BOTTOM) do what you want? <http://msdn.microsoft.com/en-us/library/windows/desktop/ms633545%28v=vs.85%29.aspx>

Comment 15

[(not currently active) Ted Mielczarek]

Hacky, but couldn't you just open a new window and focus that, thus ensuring that your window is unfocused?

Comment 16

[Nathan Froyd [:froydnj]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #14)
> Does SetWindowPos(HWND_BOTTOM) do what you want?
> <http://msdn.microsoft.com/en-us/library/windows/desktop/ms633545%28v=vs.
> 85%29.aspx>

Ah, thanks for the pointer!  That looks vaguely promising.

Turns out we use SetWindowPos in the windows widget code to implement nsIWidget::PlaceBehind:

http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#1556

Mac seems to have a similar function, nsWindow::orderBack:

https://developer.apple.com/library/mac/documentation/Cocoa/Reference/ApplicationKit/Classes/NSWindow_Class/Reference/Reference.html#//apple_ref/doc/uid/20000013-BCICJAFB

Unfortunately, implementing nsIWidget::PlaceBehind for Mac using that function and then calling it via nsDOMWindowUtils doesn't seem to have any visible effect.  It's possible I'm just not doing something right, but I have toplevel widgets and windows, I think...

Haven't tried to see if SetWindowPos can work for this on Windows.

Comment 17

[Nathan Froyd [:froydnj]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> Hacky, but couldn't you just open a new window and focus that, thus ensuring
> that your window is unfocused?

Heh, I had thought about this, but wasn't sure how well it would be received.  Two people with the same idea can't be crazy, can they...

A try build using Ted's suggestion is running at: https://tbpl.mozilla.org/?tree=Try&rev=1bb508e2a62e  Seems to DTRT in light testing on my Mac desktop.

Comment 18

[Nathan Froyd [:froydnj]]

Attachment: blur reftests that don't require focus

This patch adds a second window that's purely used as a place to hold focus
when tests don't require focus.  Testing on Try indicates that it's effective
for discovering tests that don't correcetly mark themselves as needs-focus.

It requires the patches from bug 881242 for gFocusFilterMode.

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did you test that this makes needs-focus tests fail if you remove the needs-focus annotation?

And have you checked that this doesn't interfere with any of the things we do to ensure that we have accelerated graphics being tested (which I think may depend on the whole window being visible in some cases).  You should probably look at the reftest.js history to see which Graphics folks to ping about that.

Comment 20

[Nathan Froyd [:froydnj]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #19)
> Did you test that this makes needs-focus tests fail if you remove the
> needs-focus annotation?

I did find tests that didn't have the needs-focus annotation and should have with this patch:

https://tbpl.mozilla.org/?tree=Try&rev=1bb508e2a62e

which is equivalent.  (Those tests have since been fixed.)

> And have you checked that this doesn't interfere with any of the things we
> do to ensure that we have accelerated graphics being tested (which I think
> may depend on the whole window being visible in some cases).  You should
> probably look at the reftest.js history to see which Graphics folks to ping
> about that.

I have not; I did attempt to ensure that the dummy window doesn't overlap the main window by placing it out of the way, but my attempt does assume the main window is located at (0,0), which is probably not robust.

Do the accelerated graphics tests show up in normal reftests, or are special switches required to run them on try?  From the above run, the three major desktop platforms appear to have done just fine.

Comment 21

[Nathan Froyd [:froydnj]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #19)
> You should
> probably look at the reftest.js history to see which Graphics folks to ping
> about that.

I talked to Matt Woodrow on IRC and he said the visibility of the window shouldn't matter.

Between comment 20 and this comment, I think your questions have been answered.  Good for review now (along with bug 881242)?

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 763604 [details] [diff] [review]
blur reftests that don't require focus

># HG changeset patch
># User Nathan Froyd <[e-mail]>
>
>blur reftests that don't require focus

The first line of the commit message should mention the bug number, and probably also the rationale (e.g., "so that tests missing a needs-focus annotation cause failures more reliably")

>diff --git a/layout/tools/reftest/reftest-cmdline.js b/layout/tools/reftest/reftest-cmdline.js
>index 1078561..21d91a6 100644
>--- a/layout/tools/reftest/reftest-cmdline.js
>+++ b/layout/tools/reftest/reftest-cmdline.js
>@@ -105,15 +105,28 @@ RefTestCmdLineHandler.prototype =
>     // Checking whether two files are the same is slow on Windows.
>     // Setting this pref makes tests run much faster there.
>     branch.setBoolPref("security.fileuri.strict_origin_policy", false);
> 
>     var wwatch = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>                            .getService(nsIWindowWatcher);
>     wwatch.openWindow(null, "chrome://reftest/content/reftest.xul", "_blank",
>                       "chrome,dialog=no,all", args);
>+    var remote = false;
>+    try {
>+      remote = prefs.getBoolPref("reftest.remote");
>+    } catch (ex) {
>+    }
>+    if (!remote) {
>+      // We have this dummy window to help enforce proper focus discipline in
>+      // our tests.  We position it just to the right of the main reftest window.
>+      var dummy = wwatch.openWindow(null, "about:blank", "dummy",
>+				    "chrome,dialog=no,left=800,height=200,width=200,all", null);
>+      dummy.focus();
>+    }

I think you should file a followup bug on making this work when reftest.remote is set.  (Why doesn't it work now?)

Also, do you know that dummy.focus() works that quickly after the windowwatcher.openWindow call?  It seems odd that it would.

And what guarantees that the dummy window is open by the time the reftest window calls Blur()?  (And is blur() really sufficient to move focus to the other window?)

Otherwise I think this seems fine.

Comment 23

[:Gavin Sharp [email: [e-mail]]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #22)
> Also, do you know that dummy.focus() works that quickly after the
> windowwatcher.openWindow call?  It seems odd that it would.

I think it's probably guaranteed to ensure that the window is raised, but since the window contents won't have loaded yet, it may not properly adjust other aspects of focus (focused element, etc.). I don't actually know how this stuff works in detail, but it's probably better to wait for the window's load event?

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 763604 [details] [diff] [review]
blur reftests that don't require focus

I probably should have minused when I wrote comment 20.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

er, comment 22

Comment 26

[Nathan Froyd [:froydnj]]

Attachment: blur reftests that don't need focus

I think this ensures the dummy window is properly focused before the reftests begin
executing.  I made this conditional on !remote because I wasn't sure that you could
open up multiple windows on, say, B2G (or Android, for that matter).  If remote
multiple windows can be made to work, then I'll file a followup for removing the
!remote restriction.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 783181 [details] [diff] [review]
blur reftests that don't need focus

From comment 22:
> The first line of the commit message should mention the bug number, and probably also the rationale (e.g., "so that tests missing a needs-focus annotation cause failures more reliably")

I'll drop the "probably" this time.  It's important to have that in the
first line for sheriffs and for code archaeology.


Second, you dropped a bunch of your commit message from the previous
version:

>-This patch adds a second window that's purely used as a place to hold focus
>-when tests don't require focus.  Testing on Try indicates that it's effective
>-for discovering tests that don't correcetly mark themselves as needs-focus.
>-
>-It requires the patches from bug 881242 for gFocusFilterMode.

I think this should remain in the commit message.


Your additions to the commit message:
>+I think this ensures the dummy window is properly focused before the reftests begin
>+executing.  I made this conditional on !remote because I wasn't sure that you could
>+open up multiple windows on, say, B2G (or Android, for that matter).  If remote
>+multiple windows can be made to work, then I'll file a followup for removing the
>+!remote restriction.

should be wrapped at less than 80 characters.  They should also be
reworded to describe the behavior of this patch rather than the delta
between versions of this patch.




Also (on the comment and the code), it seems like there should be code
comments documenting what |remote| means, both its existing behavior
and the new semantic you're adding to it.


r=dbaron with that

Comment 28

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9525aba3e7d2

Try-tested:

https://tbpl.mozilla.org/?tree=Try&rev=659c4ee44e97 (reftests)
https://tbpl.mozilla.org/?tree=Try&rev=74156309669f (crashtests)

AFAICS there haven't been any extra tests landed today, so there shouldn't be any spurious oranges showing up.  If some do, all the tests likely need is a needs-focus annotation.

Comment 29

[Ed Morley [:emorley]]

Wasn't sure whether to mark as needs-focus or backout; went with the latter since it was an assertion rather than straight-up test failure:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=10.8%20mozilla-inbound debug%20test%20reftest&fromchange=2ad7dd8a1201&tochange=80ceea190a42

eg:
https://tbpl.mozilla.org/php/getParsedLog.php?id=27038087&tree=Mozilla-Inbound

Initially got starred as bug 896054, but actually turned that failure mode permaorange, so backed out:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/03b026f7ee72

Wonder if that helps us with bug 896054...? :-)

Comment 30

[Gregory Szorc [:gps]]

What's the status on this BuildFaster:P1-blocking bug?

Comment 31

[Nathan Froyd [:froydnj]]

(In reply to Gregory Szorc [:gps] from comment #30)
> What's the status on this BuildFaster:P1-blocking bug?

Waiting on the intermittent orange bug 896054 to be fixed so that this patch doesn't permaorange things.

Comment 32

[Gregory Szorc [:gps]]

According to a comment in bug 896054 made a few hours ago, that failure should no longer be an issue (on inbound so far) and thus this bug should be free to land.

Nathan: Feel free to work your magic!

Comment 33

[Nathan Froyd [:froydnj]]

(In reply to Gregory Szorc [:gps] from comment #32)
> According to a comment in bug 896054 made a few hours ago, that failure
> should no longer be an issue (on inbound so far) and thus this bug should be
> free to land.
> 
> Nathan: Feel free to work your magic!

Nobody's landed any tests that need to be marked as needs-focus and a try run looks good.  Unfortunately, I have to wait for inbound to reopen before landing, as Markus's patch hasn't been merged to central/other inbound branches.

Comment 34

[Nathan Froyd [:froydnj]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=4546ae6ec60d (didn't include the patch from bug 896054, so the OS X reftest orange is expected).

https://hg.mozilla.org/integration/mozilla-inbound/rev/06c3ed54ed62

Any persistent failures ideally just require a needs-focus annotation.

Comment 35

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/06c3ed54ed62