Closed Bug 516440 Opened 13 years ago Closed 13 years ago

Port waitForFocus to browser chrome tests framework

Categories

(Testing :: Mochitest, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: ehsan.akhgari, Assigned: mak)

References

(Depends on 1 open bug)

Details

(Whiteboard: [fixed1.9.2b1])

Attachments

(2 files, 2 obsolete files)

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.
Keywords: checkin-needed
SimpleTest.waitForFocus should already just work if you called it from a browser-chrome test.
(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?
Do you have an example, or a patch I could try out?
Attached patch DemonstrationSplinter Review
Yes, here's a patch.
Ah, ok, I assumed it used the same SimpleTest.js. EventUtils seems to be available though. I think Gavin wrote much of this though.
CCing Gavin...
Attached patch patch v1.0 (obsolete) — Splinter Review
I've tested this and it works, dunno if the approach is acceptable.
Attachment #401434 - Flags: review?(gavin.sharp)
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.
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 :\
Blocks: 507172
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?
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.
I'm looking into it, trying to get the scripts in the separate test window and inject in tests...
Attached patch patch v2.0 (obsolete) — Splinter Review
largely better. I'll push to the tryserver to see what happens :)
Attachment #401434 - Attachment is obsolete: true
Attachment #401434 - Flags: review?(gavin.sharp)
Attached patch patch v2.1Splinter Review
more scope separation.
Attachment #401500 - Attachment is obsolete: true
on tryservers, changeset d79e42aaa2ee.
uops sorry i pushed again to tryservers and everything was fine, just forgot to ask review again :)
Assignee: nobody → mak77
http://hg.mozilla.org/mozilla-central/rev/80dc1c31b175
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Whiteboard: [fixed1.9.2b1]
Does this actually work?
afaict yes, i was able to use it locally. Do you have issues with it?
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.
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
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;
  }
  ...
}
please file a new bug, i suppose we could set what we need before calling SimpleTest.waitForFocus
Depends on: 521233
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?
(In reply to comment #25)
> getFocusedElementForWindow will return activeWindow when its focused

Only if the content window isn't focused too.
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?
please goto bug 521233, the discussion belongs there
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.
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).
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.
Component: BrowserTest → Mochitest
You need to log in before you can comment on or make changes to this bug.