Closed
Bug 516440
Opened 15 years ago
Closed 15 years ago
Port waitForFocus to browser chrome tests framework
Categories
(Testing :: Mochitest, defect)
Testing
Mochitest
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)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Gavin
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 1•15 years ago
|
||
SimpleTest.waitForFocus should already just work if you called it from a browser-chrome test.
Reporter | ||
Comment 2•15 years ago
|
||
(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•15 years ago
|
||
Do you have an example, or a patch I could try out?
Reporter | ||
Comment 4•15 years ago
|
||
Yes, here's a patch.
Comment 5•15 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.
Reporter | ||
Comment 6•15 years ago
|
||
CCing Gavin...
Assignee | ||
Comment 7•15 years ago
|
||
I've tested this and it works, dunno if the approach is acceptable.
Attachment #401434 -
Flags: review?(gavin.sharp)
Comment 8•15 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•15 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 :\
Comment 10•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
I'm looking into it, trying to get the scripts in the separate test window and inject in tests...
Assignee | ||
Comment 13•15 years ago
|
||
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•15 years ago
|
||
more scope separation.
Attachment #401500 -
Attachment is obsolete: true
Assignee | ||
Comment 15•15 years ago
|
||
on tryservers, changeset d79e42aaa2ee.
Updated•15 years ago
|
Attachment #401511 -
Flags: review+
Assignee | ||
Comment 16•15 years ago
|
||
uops sorry i pushed again to tryservers and everything was fine, just forgot to ask review again :)
Assignee | ||
Updated•15 years ago
|
Assignee: nobody → mak77
Assignee | ||
Comment 17•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Assignee | ||
Comment 18•15 years ago
|
||
Updated•15 years ago
|
Whiteboard: [fixed1.9.2b1]
Comment 19•15 years ago
|
||
Does this actually work?
Assignee | ||
Comment 20•15 years ago
|
||
afaict yes, i was able to use it locally. Do you have issues with it?
Comment 21•15 years ago
|
||
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•15 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
Comment 23•15 years ago
|
||
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•15 years ago
|
||
please file a new bug, i suppose we could set what we need before calling SimpleTest.waitForFocus
Comment 25•15 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?
Comment 26•15 years ago
|
||
(In reply to comment #25)
> getFocusedElementForWindow will return activeWindow when its focused
Only if the content window isn't focused too.
Comment 27•15 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•15 years ago
|
||
please goto bug 521233, the discussion belongs there
Comment 29•15 years ago
|
||
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•15 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).
Comment 31•15 years ago
|
||
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.
Updated•7 years ago
|
Component: BrowserTest → Mochitest
You need to log in
before you can comment on or make changes to this bug.
Description
•