Port waitForFocus to browser chrome tests framework

RESOLVED FIXED in mozilla1.9.3a1

Status

defect
RESOLVED FIXED
10 years ago
a year ago

People

(Reporter: Ehsan, Assigned: mak)

Tracking

(Depends on 1 bug)

Trunk
mozilla1.9.3a1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fixed1.9.2b1])

Attachments

(2 attachments, 2 obsolete attachments)

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

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

Comment 3

10 years ago
Do you have an example, or a patch I could try out?
Posted patch DemonstrationSplinter Review
Yes, here's a patch.

Comment 5

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

Comment 7

10 years ago
Posted 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 8

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

Comment 9

10 years ago
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 :\
Assignee

Updated

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

Comment 12

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

Comment 13

10 years ago
Posted 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)
Assignee

Comment 14

10 years ago
Posted patch patch v2.1Splinter Review
more scope separation.
Attachment #401500 - Attachment is obsolete: true
Assignee

Comment 15

10 years ago
on tryservers, changeset d79e42aaa2ee.
Assignee

Comment 16

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

Updated

10 years ago
Assignee: nobody → mak77
Assignee

Comment 17

10 years ago
http://hg.mozilla.org/mozilla-central/rev/80dc1c31b175
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Whiteboard: [fixed1.9.2b1]
Does this actually work?
Assignee

Comment 20

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

Comment 22

10 years ago
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;
  }
  ...
}
Assignee

Comment 24

10 years ago
please file a new bug, i suppose we could set what we need before calling SimpleTest.waitForFocus
Depends on: 521233

Comment 25

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

Comment 27

10 years ago
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?
Assignee

Comment 28

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

Comment 30

10 years ago
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
Product: Testing → Testing
You need to log in before you can comment on or make changes to this bug.