516440 - Port waitForFocus to browser chrome tests framework

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[(no longer active)]

SimpleTest.waitForFocus is much needed for the browser chrome tests framework as well.  I am not sure whether this involves a simple copy/pasting of the code from the SimpleTest framework, but I myself know of several tests which could benefit from this, such as bug 513560.

Comment 1

[Neil Deakin]

SimpleTest.waitForFocus should already just work if you called it from a browser-chrome test.

Comment 2

[(no longer active)]

(In reply to comment #1)
> SimpleTest.waitForFocus should already just work if you called it from a
> browser-chrome test.

I did, and it doesn't.  Am I missing something?

Comment 3

[Neil Deakin]

Do you have an example, or a patch I could try out?

Comment 4

[(no longer active)]

Attachment: Demonstration

Yes, here's a patch.

Comment 5

[Neil Deakin]

Ah, ok, I assumed it used the same SimpleTest.js. EventUtils seems to be available though. I think Gavin wrote much of this though.

Comment 6

[(no longer active)]

CCing Gavin...

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

I've tested this and it works, dunno if the approach is acceptable.

Comment 8

[Neil Deakin]

Comment on attachment 401434 [details] [diff] [review]
patch v1.0

>@@ -178,6 +178,8 @@ function testScope(aTester, aTest) {
>   var scriptLoader = Cc["@mozilla.org/moz/jssubscript-loader;1"].
>                      getService(Ci.mozIJSSubScriptLoader);
>   scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/EventUtils.js", this.EventUtils);
>+  scriptLoader.loadSubScript("chrome://mochikit/content/MochiKit/packed.js", this.SimpleTest);
>+  scriptLoader.loadSubScript("chrome://mochikit/content/tests/SimpleTest/SimpleTest.js", this.SimpleTest);

Is this being done just once at the beginning of a test run, or for every test? If every test, using the subscript loader would have a impact on performance of the tests.

Comment 9

[Marco Bonardo [:mak]]

i hope it's run just once when initing the harness.
Btw, this works but packes.js has lot of warnings due to strict js, and they pollute the output :\

Comment 10

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

Comment on attachment 401434 [details] [diff] [review]
patch v1.0

I'm kind of nervous about importing all of mochikit like this. Have you measured its impact on the length of a full test run?

Comment 11

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

Oh, sorry, I missed the comments. A testScope is created for each test (i.e. each file), so this would indeed load it for each test. We should probably avoid that, and also avoid the loading of EventUtils as well.

Comment 12

[Marco Bonardo [:mak]]

I'm looking into it, trying to get the scripts in the separate test window and inject in tests...

Comment 13

[Marco Bonardo [:mak]]

Attachment: patch v2.0

largely better. I'll push to the tryserver to see what happens :)

Comment 14

[Marco Bonardo [:mak]]

Attachment: patch v2.1

more scope separation.

Comment 15

[Marco Bonardo [:mak]]

on tryservers, changeset d79e42aaa2ee.

Comment 16

[Marco Bonardo [:mak]]

uops sorry i pushed again to tryservers and everything was fine, just forgot to ask review again :)

Comment 17

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/80dc1c31b175

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/de6be721d0b0

Comment 19

[Dão Gottwald [:dao]]

Does this actually work?

Comment 20

[Marco Bonardo [:mak]]

afaict yes, i was able to use it locally. Do you have issues with it?

Comment 21

[Dão Gottwald [:dao]]

Neither waitForFocus(callback) nor waitForFocus(callback, window) work. I guess it's because of these lines:

    var focusedWindow = { };
    if (fm.activeWindow)
      fm.getFocusedElementForWindow(fm.activeWindow, true, focusedWindow);

    // if this is a child frame, ensure that the frame is focused
    SimpleTest.waitForFocus_focused = (focusedWindow.value == targetWindow);

waitForFocus(callback, content) works.

Comment 22

[Marco Bonardo [:mak]]

i just ported the method in SimpleTest to our harness, so i guess if that's reproduceable it should have its own bug, Enn could know more.

btw i had used waitForFocus(callback) and it was working for me

Comment 23

[Dão Gottwald [:dao]]

It's not really a bug in the original code, because that code was designed for content windows. For waitForFocus(callback) in a browser chrome test, we really want (fm.activeWindow == targetWindow) instead of (focusedWindow.value == targetWindow). That's basically what I've been doing in a few tests already:

function test() {
  waitForExplicitFinish();      

  if (Cc["@mozilla.org/focus-manager;1"].getService(Ci.nsIFocusManager).activeWindow !=
       window) {
    setTimeout(test, 0);
    window.focus();
    return;
  }
  ...
}

Comment 24

[Marco Bonardo [:mak]]

please file a new bug, i suppose we could set what we need before calling SimpleTest.waitForFocus

Comment 25

[Neil Deakin]

getFocusedElementForWindow will return activeWindow when its focused, so both (fm.activeWindow == targetWindow) and (focusedWindow.value ==
targetWindow) should check the same thing, no?

Maybe you mean that the browser-chrome test opens a tab or something, and you want to check focus on the content window versus the chrome window?

Comment 26

[Dão Gottwald [:dao]]

(In reply to comment #25)
> getFocusedElementForWindow will return activeWindow when its focused

Only if the content window isn't focused too.

Comment 27

[Neil Deakin]

I'm not following. Can you give an example of what windows/child frames exist, what is currently focused and what you expect to be focused?

Comment 28

[Marco Bonardo [:mak]]

please goto bug 521233, the discussion belongs there

Comment 29

[Dão Gottwald [:dao]]

There's a chrome window (window) and there's a content window (content). waitForFocus(callback, window) doesn't work if content has focus.

You can easily reproduce this with the error console, just execute this:

var w = top.opener;
w.setTimeout(function (w) {
  var fm = w.Cc["@mozilla.org/focus-manager;1"].getService(w.Ci.nsIFocusManager);
  var focusedWindow = {};
  fm.getFocusedElementForWindow(fm.activeWindow, true, focusedWindow);
  alert(focusedWindow.value.location);
}, 3000, w)

... and click in the browsers content area. Then do the same and click in the location bar.

Comment 30

[Neil Deakin]

Still not following.

If you expect 'window' to be focused, you should call waitForFocus(callback, window). If you expect 'content' to be focused, you should call waitForFocus(callback, content).

Comment 31

[Dão Gottwald [:dao]]

I expect window to be focused and I do call waitForFocus(callback, window). It times out because content has focus, i.e. focusedWindow.value == content.