Closed Bug 432740 Opened 12 years ago Closed 12 years ago

make SeaMonkey Windows unit test machine pass all mochitests

Categories

(SeaMonkey :: Build Config, defect)

x86
Windows Server 2003
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.0a1

People

(Reporter: kairo, Assigned: kairo)

References

Details

Attachments

(5 files, 2 obsolete files)

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.
Attached patch increase default window size (obsolete) — Splinter Review
Attachment #319894 - Flags: superreview?(neil)
Attachment #319894 - Flags: review?
Attachment #319894 - Flags: review? → review?(kairo)
not sure if simply increasing default window size like this is best, but it does work
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.
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?
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.
Attachment #319894 - Attachment is obsolete: true
Attachment #319976 - Flags: superreview?(neil)
Attachment #319976 - Flags: review?(bugspam.Callek)
Attachment #319894 - Flags: superreview?(neil)
Attachment #319894 - Flags: review?(kairo)
Comment on attachment 319976 [details] [diff] [review]
dynamic navigator window default size, v1

looks good to me, and I'm fine with the change.
Attachment #319976 - Flags: review?(bugspam.Callek) → review+
The Windows buildbot did really go green with this patch! :)
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
Attachment #319976 - Flags: superreview?(neil) → superreview-
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.
Attachment #319976 - Attachment is obsolete: true
Attachment #320181 - Flags: superreview?(neil)
Attachment #320181 - Flags: review?(bugspam.Callek)
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");
Attachment #320181 - Flags: superreview?(neil) → superreview+
(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.
(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.

Attachment #320181 - Flags: review?(bugspam.Callek) → review+
Landed with Neil's sr comments addressed. This should be fixed now. yay!
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Blocks: 433358
You need to log in before you can comment on or make changes to this bug.