725015 - [SeaMonkey] permanent "dom/tests/mochitest/bugs/test_resize_move_windows.html | Test timed out."

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Serge Gautherie (:sgautherie)]

SM 2.8:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1328566990.1328572485.22195.gz
WINNT 5.2 comm-beta debug test mochitests-2/5 on 2012/02/06 14:23:10
is already failing.

SM 2.9:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1328598005.1328601563.15240.gz
WINNT 5.2 comm-aurora debug test mochitests-2/5 on 2012/02/06 23:00:05
is already failing.

SM 2.10:
http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1328503421.1328505166.30593.gz
Linux comm-central-trunk debug test mochitests-2/5 on 2012/02/05 20:43:41
{
...
14285 INFO TEST-PASS | /tests/dom/tests/mochitest/bugs/test_resize_move_windows.html | parameter screenY should be taken into account
14286 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_resize_move_windows.html | Test timed out.
SCREENSHOT: data:image/png;base64,[...]
14287 INFO TEST-END | /tests/dom/tests/mochitest/bugs/test_resize_move_windows.html | finished in 308908ms
}
NB: Screenshot simply shows the small opened window :-|

Reproducible on my local Windows 2000.

*****

This test needs to ensure "dom.disable_window_move_resize" is set to false!
http://mxr.mozilla.org/comm-central/search?string=dom.disable_window_move_resize&case=1&find=\.js

Comment 1

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Add missing 'return', Set needed preference, Improve some nits

Comment 2

[Mounir Lamouri (:mounir)]

Comment on attachment 595119 [details] [diff] [review]
(Av1) Add missing 'return', Set needed preference, Improve some nits

Review of attachment 595119 [details] [diff] [review]:
-----------------------------------------------------------------

A few of those changes are not very easy to understand. Could you give more context?

::: dom/tests/mochitest/bugs/test_resize_move_windows.html
@@ +72,3 @@
>    if (condition() || times == 0) {
>      test();
> +    SimpleTest.executeSoon(next);

Why?

@@ +261,5 @@
>    });
>    });
>  }
>  
> +SpecialPowers.pushPrefEnv({"set": [["dom.disable_window_move_resize", false]]}, runTests);

I would prefer:
SpecialPowers.pushPrefEnv({"set": [["dom.disable_window_move_resize", false]]}, function() {
SimpleTest.waitForFocus(function() {
  //
})
});

@@ +269,4 @@
>    if (screen.width <= 200 || screen.height <= 200) {
> +    todo(false, "Screen needs to be at least 200x200px to run this test.");
> +    SimpleTest.executeSoon(SimpleTest.finish);
> +    return;

Why?

@@ +281,5 @@
>      // However, passing size/position parameters to window.open should work.
>      var w = window.open("data:text/html,<script>" +
>        "function check(next) {" +
>        "  var is_range = function(aTest, aValue, aRange, aMsg) {" +
> +      "    window.opener.ok(aValue - aRange < aTest && aTest < aValue + aRange, aMsg);" +

Does not seem useful.

@@ +313,5 @@
>                checkChangeIsDisabled(w, function() {
>                  w.close();
>  
>                  restoreValues();
> +                SimpleTest.executeSoon(SimpleTest.finish);

Why?

Comment 3

[Serge Gautherie (:sgautherie)]

(In reply to Mounir Lamouri (:volkmar) (:mounir) from comment #2)

> ::: dom/tests/mochitest/bugs/test_resize_move_windows.html
> @@ +72,3 @@
> >    if (condition() || times == 0) {
> >      test();
> > +    SimpleTest.executeSoon(next);
> 
> Why?

executeSoon(), here and elsewhere, lets the calling callback fully end, fwiw: optional.

> I would prefer:
> SpecialPowers.pushPrefEnv({"set": [["dom.disable_window_move_resize",
> false]]}, function() {
> SimpleTest.waitForFocus(function() {
>   //
> })
> });

As you prefer: I will do.

> @@ +269,4 @@
> >    if (screen.width <= 200 || screen.height <= 200) {
> > +    todo(false, "Screen needs to be at least 200x200px to run this test.");
> > +    SimpleTest.executeSoon(SimpleTest.finish);
> > +    return;
> 
> Why?

(Why what?)

> @@ +281,5 @@
> >      // However, passing size/position parameters to window.open should work.
> >      var w = window.open("data:text/html,<script>" +
> >        "function check(next) {" +
> >        "  var is_range = function(aTest, aValue, aRange, aMsg) {" +
> > +      "    window.opener.ok(aValue - aRange < aTest && aTest < aValue + aRange, aMsg);" +
> 
> Does not seem useful.

Easier to read to me: optional.

Comment 4

[Benjamin Smedberg]

Please note that I just commited something with the wrong bug number in it: if you see a patch about category entries and observer notifications, you want bug 725016, not this bug.

Comment 5

[Legacy TBPL/Treeherder Robot]

mak77%bonardo.net
https://tbpl.mozilla.org/php/getParsedLog.php?id=9409823&tree=Firefox
Rev3 Fedora 12x64 mozilla-central pgo test mochitests-2/5 on 2012-02-17 01:00:48

14408 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/bugs/test_resize_move_windows.html | Test timed out.

Comment 6

[Serge Gautherie (:sgautherie)]

(In reply to TinderboxPushlog Robot from comment #5)
> mak77%bonardo.net
> https://tbpl.mozilla.org/php/getParsedLog.php?id=9409823&tree=Firefox

Not this SeaMonkey bug.

Comment 7

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Add missing 'return', Set needed preference, Improve some nits

Av1, without executeSoon() and with s/200/201/g.

Comment 8

[Mounir Lamouri (:mounir)]

Comment on attachment 598997 [details] [diff] [review]
(Av2) Add missing 'return', Set needed preference, Improve some nits

Review of attachment 598997 [details] [diff] [review]:
-----------------------------------------------------------------

Dropping the review request because I would like to see the fixed patch.

::: dom/tests/mochitest/bugs/test_resize_move_windows.html
@@ -22,5 @@
>  SimpleTest.waitForExplicitFinish();
>  
>  var previousX, previousY, previousWidth, previousHeight;
>  
> -

no need to remove this line.

@@ +67,5 @@
>   * If times < 0, the event loop will be hitten as long as the condition isn't
>   * true or the test doesn't time out.
>   */
> +function hitEventLoop(condition, test, times, next)
> +{

no need to change the style

@@ +266,3 @@
>  SimpleTest.waitForFocus(function() {
> +  if (screen.width < 201 || screen.height < 201) {
> +    todo(false, "Screen needs to be at least 201x201px to run this test.");

Ok to change the todo description (I would have preferred "The screen needs to be bigger than 200px*200px to run this test.") but I don't think you need to change the condition.

@@ +278,5 @@
>      // However, passing size/position parameters to window.open should work.
>      var w = window.open("data:text/html,<script>" +
>        "function check(next) {" +
>        "  var is_range = function(aTest, aValue, aRange, aMsg) {" +
> +      "    window.opener.ok(aValue - aRange < aTest && aTest < aValue + aRange, aMsg);" +

I would prefer if you could drop that change.

Comment 9

[Serge Gautherie (:sgautherie)]

Attachment: (Av3) Add missing 'return', Set needed preference, Make todo() message more explicit [Checked in: Comments 11 and 15]

Av2, with comment 8 suggestion(s).

Comment 10

[Mounir Lamouri (:mounir)]

Comment on attachment 601274 [details] [diff] [review]
(Av3) Add missing 'return', Set needed preference, Make todo() message more explicit
[Checked in: Comments 11 and 15]

Review of attachment 601274 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks :)

r=me

Comment 11

[Serge Gautherie (:sgautherie)]

Comment on attachment 601274 [details] [diff] [review]
(Av3) Add missing 'return', Set needed preference, Make todo() message more explicit
[Checked in: Comments 11 and 15]

https://hg.mozilla.org/mozilla-central/rev/30b4f99a137c


Succeeded as
https://tbpl.mozilla.org/?tree=Try&rev=6d1a41cb8058


[Approval Request Comment]
Regression caused by (bug #): Bug 565541.
User impact if declined: None, but perma-orange (timeout) on SeaMonkey.
Testing completed (on m-c, etc.): Try and this comment.
Risk to taking this patch (and alternatives if risky): None, test only.
String changes made by this patch: None.

Comment 12

[Mozilla RelEng Bot]

Try run for 6d1a41cb8058 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6d1a41cb8058
Results (out of 30 total builds):
    exception: 1
    success: 27
    warnings: 2
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6d1a41cb8058

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 601274 [details] [diff] [review]
(Av3) Add missing 'return', Set needed preference, Make todo() message more explicit
[Checked in: Comments 11 and 15]

[Triage Comment]
Test only change - approved for Aurora 12 and Beta 11. Note that beta 5 will be the last FF11 beta for which we'll accept test changes.

Comment 14

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey/1330540575.1330541423.13527.gz&fulltext=1
Linux comm-central-trunk debug test mochitests-2/5 on 2012/02/29 10:36:15
{
14453 INFO TEST-END | /tests/dom/tests/mochitest/bugs/test_resize_move_windows.html | finished in 11118ms
}

V.Fixed

Comment 15

[Jens Hatlak (:InvisibleSmiley)]

Comment on attachment 601274 [details] [diff] [review]
(Av3) Add missing 'return', Set needed preference, Make todo() message more explicit
[Checked in: Comments 11 and 15]

http://hg.mozilla.org/releases/mozilla-aurora/rev/b5ef80040f72
http://hg.mozilla.org/releases/mozilla-beta/rev/529498671c4c

Comment 16

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Aurora/1330595464.1330597769.12144.gz&fulltext=1
Linux comm-aurora debug test mochitests-2/5 on 2012/03/01 01:51:04
{
14303 INFO TEST-END | /tests/dom/tests/mochitest/bugs/test_resize_move_windows.html | finished in 12910ms
}

seamonkey2.9: verified.

Comment 17

[Serge Gautherie (:sgautherie)]

http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Beta/1330650199.1330652139.30264.gz
WINNT 5.2 comm-beta debug test mochitests-2/5 on 2012/03/01 17:03:19

firefox11: verified.