432740 - make SeaMonkey Windows unit test machine pass all mochitests

Status: RESOLVED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Robert Kaiser]

Attachment: screen shot of default window size on Windows on mochitests

After the fix for bug 431464, the SeaMonkey Windows unit test machine, cn-sea-qm-win2k3-01 dep unit test, still fails to pass 5 mochitests:

*** 45375 ERROR FAIL | Test timed out. |  | /tests/toolkit/content/tests/widgets/test_colorpicker_popup.xul
*** 49037 ERROR FAIL | decimal mouse down change event fired | got false, expected true | /tests/toolkit/content/tests/widgets/test_textbox_number.xul
*** 49038 ERROR FAIL | decimal mouse down | got "10.1", expected 9.1 | /tests/toolkit/content/tests/widgets/test_textbox_number.xul
*** 49097 ERROR FAIL | integer with increment mouse down change event fired | got false, expected true | /tests/toolkit/content/tests/widgets/test_textbox_number.xul
*** 49098 ERROR FAIL | integer with increment mouse down | got "10.1", expected 5.8 | /tests/toolkit/content/tests/widgets/test_textbox_number.xul


The problem seems to be that the mouse events we create don't fire over the iframe because the iframe is not fully visible, see the attached screen shot.

We either need to ensure that the iframe is visible and the window big enough for mochitests only, or we generally need to make the default window size bigger - it seems that Firefox has a pretty good algorithm to make the window size nice on all screen resolutions.

navigator.xul has style="width: 70ch; height: 45em;" currently, I've just uploaded a hacked version to the buildbot machine that sets a height of 55em, just for testing.

Comment 1

[Justin Wood (:Callek)]

Attachment: screen shot of Default Mochi on windows (just window)

Comment 2

[Justin Wood (:Callek)]

Attachment: [reference] screen shot of Firefox mochi on windows

Comment 3

[Justin Wood (:Callek)]

Attachment: increase default window size

Comment 4

[Justin Wood (:Callek)]

Attachment: Screen shot of Mochi with the proposed new size on windows

Comment 5

[Justin Wood (:Callek)]

not sure if simply increasing default window size like this is best, but it does work

Comment 6

[Robert Kaiser]

Comment on attachment 319894 [details] [diff] [review]
increase default window size

I'd like better if we made this more dynamic, we might want to fit something like a 800x600 or 1024x768 screen still, and I'm not sure if 65em height works there.

Comment 7

[Robert Kaiser]

With a default window height of 55em, the colorpicker test remains failing, all others pass, see http://tinderbox.mozilla.org/showlog.cgi?log=SeaMonkey-Ports/1210223893.1210226842.6530.gz#err0

Is this still the same problem?

Comment 8

[Robert Kaiser]

Attachment: dynamic navigator window default size, v1

This patch ports the block from http://mxr.mozilla.org/mozilla/source/browser/base/content/browser.js#755 (which sets the FF default window size) to SeaMonkey. This is a dynamic approach that should work on all window sizes and still make the window big enough on our buildbot machine that it can pass mochitests. I patched the machine locally with this for testing.

Comment 9

[Justin Wood (:Callek)]

Comment on attachment 319976 [details] [diff] [review]
dynamic navigator window default size, v1

looks good to me, and I'm fine with the change.

Comment 10

[Robert Kaiser]

The Windows buildbot did really go green with this patch! :)

Comment 11

[[e-mail]]

Comment on attachment 319976 [details] [diff] [review]
dynamic navigator window default size, v1

>+  // Set a sane starting width/height for all resolutions on new profiles.
Um, yes, well, there are all sorts of magic numbers here :-(
1. If the screen is 1024x600 or less, then the default size is set to 610x450 regardless of the actual size, and then the window is maximised.
2. If the available width is between about 1024 and 1440 pixels, then the default width is 994 pixels, otherwise the default width is slightly less than half the available width.
3. The default height is slightly less than the screen height, with an extra guess tweak for X11.
You'll be glad to know that I'm not going to argue with maximising the window on small screens, or halving the width for wide screens, or even the X11 tweak. All I'd like to see is some consistency with the default height and width on small and medium screens. Something like this:
1. Get the available width and height
2. If the width is greater than 1440 then halve it
3. Tweak the size slightly
4. If the height is less then or equal to 600 maximise the window

Comment 12

[Robert Kaiser]

Attachment: dynamic navigator window default size, v2

OK, I tweaked the patch to follow the 4-step process given by Neil in the last comment. I'll probably file a followup to tweak the mailnews default size in a similar way.

Comment 13

[[e-mail]]

Comment on attachment 320181 [details] [diff] [review]
dynamic navigator window default size, v2

>+    // Create a narrower window for large or wide-aspect displays, to suggest
>+    // side-by-side page view.
>+    if (screen.availWidth >= 1600)
>+      defaultWidth = (screen.availWidth / 2);
>+    else
>+      defaultWidth = screen.availWidth;
Please use > 1440 instead of >= 1600. You could also init defaultWidth when you declare it, and then /= 2 here (or >> 1 if you prefer!)

>+    if (navigator.appVersion.indexOf("X11") != -1) {
>+      // On X, we're not currently able to account for the size of the window
>+      // border.  Use 28px as a guess (titlebar + bottom window border)
>+      defaultHeight -= 28;
>+    }
>+
>+    if (defaultHeight <= 600) {
>+      // On small screens, default to maximized state
>+      document.documentElement.setAttribute("sizemode", "maximized");
>+    }
Nit: you could avoid the {}s like this:
// On small screens, default to maximized state
if (defaultHeight <= 600)
  document.documentElement.setAttribute("sizemode", "maximized");

Comment 14

[[e-mail]]

(In reply to comment #12)
>I'll probably file a followup to tweak the mailnews default size in a similar way.
Instead of half width you'd probably want to autoswitch to vertical layout.

Comment 15

[Robert Kaiser]

(In reply to comment #14)
> (In reply to comment #12)
> >I'll probably file a followup to tweak the mailnews default size in a similar way.
> Instead of half width you'd probably want to autoswitch to vertical layout.

Filed bug 433001, mentioning this comment.

Comment 16

[Robert Kaiser]

Landed with Neil's sr comments addressed. This should be fixed now. yay!