Closed Bug 1353278 Opened 3 years ago Closed 3 years ago

Font chooser broken - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/pref-window/test-font-chooser.js | ...::test_font_name_displayed - mail/base/test/unit/test_windows_font_migration.js | verifier - [verifier : 54] "" == "Times New Roman"

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 55.0

People

(Reporter: jorgk, Assigned: aceman)

References

Details

(Keywords: intermittent-failure, Whiteboard: [Thunderbird-testfailure: XM all])

Attachments

(3 files)

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/pref-window/test-font-chooser.js | test-font-chooser.js::test_font_name_displayed

M-C last good:
aaa0cd3bd620daf6be29c72625f6e63fd0
M-C first bad:
b5d8b27a753725c1de41ffae2e338798f3

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=aaa0cd3bd620daf6be29c72625f6e63fd0&tochange=b5d8b27a753725c1de41ffae2e338798f3

Looks like bug 1344990.

https://archive.mozilla.org/pub/thunderbird/tinderbox-builds/comm-central-macosx64/1491215363/comm-central_yosemite_r7_test-mozmill-bm106-tests1-macosx-build12.txt.gz

17:14:58     INFO -  SUMMARY-UNEXPECTED-FAIL | test-font-chooser.js | test-font-chooser.js::test_font_name_displayed
17:14:58     INFO -    EXCEPTION: The display pane font should be Al Bayan, but nothing is actually selected.
17:14:58     INFO -      at: test-font-chooser.js line 60
17:14:58     INFO -         assert_fonts_equal test-font-chooser.js:60 11
17:14:58     INFO -         verify_display_pane test-font-chooser.js:77 5
17:14:58     INFO -         waitForPaneLoad test-pref-window-helpers.js:55 5
17:14:58     INFO -         Runner.prototype.wrapper frame.js:580 7
17:14:58     INFO -         startTest test-window-helpers.js:317 11
17:14:58     INFO -         WindowWatcher_notify test-window-helpers.js:349 9
17:14:58     INFO -         waitFor utils.js:478 5
17:14:58     INFO -         WindowWatcher_waitForModalDialog test-window-helpers.js:376 5
17:14:58     INFO -         wait_for_modal_dialog test-window-helpers.js:613 3
17:14:58     INFO -         open_pref_window test-pref-window-helpers.js:60 3
17:14:58     INFO -         _verify_fonts_displayed test-font-chooser.js:81 3
17:14:58     INFO -         test_font_name_displayed test-font-chooser.js:121 3
17:14:58     INFO -         Runner.prototype.wrapper frame.js:585 9
17:14:58     INFO -         Runner.prototype._runTestModule frame.js:655 9
17:14:58     INFO -         Runner.prototype.runTestModule frame.js:701 3
17:14:58     INFO -         Runner.prototype.runTestDirectory frame.js:525 7
17:14:58     INFO -         runTestDirectory frame.js:707 3
17:14:58     INFO -         Bridge.prototype._execFunction server.js:179 10
17:14:58     INFO -         Bridge.prototype.execFunction server.js:183 16
17:14:58     INFO -         Session.prototype.receive server.js:283 3
17:14:58     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3
17:14:58     INFO -    EXCEPTION: The sans-serif font should be Al Bayan, but nothing is actually selected.
17:14:58     INFO -      at: test-font-chooser.js line 60
17:14:58     INFO -         assert_fonts_equal test-font-chooser.js:60 11
17:14:58     INFO -         verify_advanced test-font-chooser.js:86 5
17:14:58     INFO -         Runner.prototype.wrapper frame.js:580 7
17:14:58     INFO -         startTest test-window-helpers.js:317 11
17:14:58     INFO -         WindowWatcher_notify test-window-helpers.js:349 9
17:14:58     INFO -         _verify_fonts_displayed test-font-chooser.js:95 3
17:14:58     INFO -         test_font_name_displayed test-font-chooser.js:121 3
17:14:58     INFO -         Runner.prototype.wrapper frame.js:585 9
17:14:58     INFO -         Runner.prototype._runTestModule frame.js:655 9
17:14:58     INFO -         Runner.prototype.runTestModule frame.js:701 3
17:14:58     INFO -         Runner.prototype.runTestDirectory frame.js:525 7
17:14:58     INFO -         runTestDirectory frame.js:707 3
17:14:58     INFO -         Bridge.prototype._execFunction server.js:179 10
17:14:58     INFO -         Bridge.prototype.execFunction server.js:183 16
17:14:58     INFO -         Session.prototype.receive server.js:283 3
17:14:58     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3
17:14:58     INFO -  SUMMARY-UNEXPECTED-FAIL | test-font-chooser.js | test-font-chooser.js::test_font_name_not_present
17:14:58     INFO -    EXCEPTION: The display pane font should be Helvetica, but nothing is actually selected.
17:14:58     INFO -      at: test-font-chooser.js line 60
17:14:58     INFO -         assert_fonts_equal test-font-chooser.js:60 11
17:14:58     INFO -         verify_display_pane test-font-chooser.js:77 5
17:14:58     INFO -         waitForPaneLoad test-pref-window-helpers.js:55 5
17:14:58     INFO -         Runner.prototype.wrapper frame.js:580 7
17:14:58     INFO -         startTest test-window-helpers.js:317 11
17:14:58     INFO -         WindowWatcher_notify test-window-helpers.js:349 9
17:14:58     INFO -         waitFor utils.js:478 5
17:14:58     INFO -         WindowWatcher_waitForModalDialog test-window-helpers.js:376 5
17:14:58     INFO -         wait_for_modal_dialog test-window-helpers.js:613 3
17:14:58     INFO -         open_pref_window test-pref-window-helpers.js:60 3
17:14:58     INFO -         _verify_fonts_displayed test-font-chooser.js:81 3
17:14:58     INFO -         test_font_name_not_present test-font-chooser.js:168 3
17:14:58     INFO -         Runner.prototype.wrapper frame.js:585 9
17:14:58     INFO -         Runner.prototype._runTestModule frame.js:655 9
17:14:58     INFO -         Runner.prototype.runTestModule frame.js:701 3
17:14:58     INFO -         Runner.prototype.runTestDirectory frame.js:525 7
17:14:58     INFO -         runTestDirectory frame.js:707 3
17:14:58     INFO -         Bridge.prototype._execFunction server.js:179 10
17:14:58     INFO -         Bridge.prototype.execFunction server.js:183 16
17:14:58     INFO -         Session.prototype.receive server.js:283 3
17:14:58     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3
17:14:58     INFO -    EXCEPTION: The serif font should be Times, but nothing is actually selected.
17:14:58     INFO -      at: test-font-chooser.js line 60
17:14:58     INFO -         assert_fonts_equal test-font-chooser.js:60 11
17:14:58     INFO -         verify_advanced test-font-chooser.js:85 5
17:14:58     INFO -         Runner.prototype.wrapper frame.js:580 7
17:14:58     INFO -         startTest test-window-helpers.js:317 11
17:14:58     INFO -         WindowWatcher_notify test-window-helpers.js:349 9
17:14:58     INFO -         _verify_fonts_displayed test-font-chooser.js:95 3
17:14:58     INFO -         test_font_name_not_present test-font-chooser.js:168 3
17:14:58     INFO -         Runner.prototype.wrapper frame.js:585 9
17:14:58     INFO -         Runner.prototype._runTestModule frame.js:655 9
17:14:58     INFO -         Runner.prototype.runTestModule frame.js:701 3
17:14:58     INFO -         Runner.prototype.runTestDirectory frame.js:525 7
17:14:58     INFO -         runTestDirectory frame.js:707 3
17:14:58     INFO -         Bridge.prototype._execFunction server.js:179 10
17:14:58     INFO -         Bridge.prototype.execFunction server.js:183 16
17:14:58     INFO -         Session.prototype.receive server.js:283 3
17:14:58     INFO -         AsyncRead.prototype.onDataAvailable server.js:88 3

Masayuki-san, any idea what happened here?
Flags: needinfo?(masayuki)
Perhaps, the test checks default font. After landing bug 1344990, all default fonts (e.g., font.name.*.*) are set to empty string and it is displayed as "Default (%s)" (%s is the font name which is first available font in font.name-list.*.*).
Flags: needinfo?(masayuki)
This is surprising, isn't it?
Flags: needinfo?(masayuki)
I got myself a build from this M-C merge:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=b5d8b27a753725c1de41ffae2e338798f3b5cacd

The same panel looks good and I see:
Serif: Default (Times New Roman)
Sans-Serif: Default (Arial)
Monospace: Default (Courier New).

Hmm, so we need to see how we populate the TB panel.
Flags: needinfo?(masayuki)
> Hmm, so we need to see how we populate the TB panel.

Firefox inserts a dropdown list item for empty string pref value and its label is created with nsIFontEnumerator.getDefaultFont().
http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/mozapps/preferences/fontbuilder.js#26-28,42-48
Summary: TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/pref-window/test-font-chooser.js | test-font-chooser.js::test_font_name_displayed → Font chooser broken - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/pref-window/test-font-chooser.js | test-font-chooser.js::test_font_name_displayed
Hmm, just looking at
https://dxr.mozilla.org/comm-central/search?q=readFontSelection&redirect=false
shows that C-C uses gFontsDialog.readFontSelection() where FF uses FontBuilder.readFontSelection().

gFontsDialog does a lot of stuff, so perhaps that's now obsolete:
https://dxr.mozilla.org/comm-central/rev/754fbc931bbbe681a1a41074a1a1c6fa9b011c85/mail/components/preferences/fonts.js#19

Hard to say how to adapt to the FF changes without any knowledge of the area :-(
I can try to look if we can port this. But no promise.
(In reply to Jorg K (GMT+2) from comment #5)
> gFontsDialog does a lot of stuff, so perhaps that's now obsolete:
> https://dxr.mozilla.org/comm-central/rev/
> 754fbc931bbbe681a1a41074a1a1c6fa9b011c85/mail/components/preferences/fonts.
> js#19

But it uses FontBuilder which is shared with mozilla-central. And it creates "default (%s)" list item to the dropdown list, no?
(In reply to Masayuki Nakano [:masayuki] from comment #7)
> But it uses FontBuilder which is shared with mozilla-central. And it creates
> "default (%s)" list item to the dropdown list, no?
Sorry, I haven't had time to look.

I noticed that we have another test failure:
mail/base/test/unit/test_windows_font_migration.js | verifier - [verifier : 54] "" == "Times New Roman".

We might address this here or move it to another bug.
Summary: Font chooser broken - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/pref-window/test-font-chooser.js | test-font-chooser.js::test_font_name_displayed → Font chooser broken - TEST-UNEXPECTED-FAIL | /builds/slave/test/build/tests/mozmill/pref-window/test-font-chooser.js | ...::test_font_name_displayed - mail/base/test/unit/test_windows_font_migration.js | verifier - [verifier : 54] "" == "Times New Roman"
Whiteboard: [Thunderbird-testfailure: M all] → [Thunderbird-testfailure: XM all]
This fixes the Xpcshell test:
mozilla/mach xpcshell-test mail/base/test/unit/test_windows_font_migration.js

We've met this test in bug 1351721. Please no discussion about the test, what it does and what it's good for. It fails right now, so let's fix that. Quick review much appreciated. I have no reviewer in Windows, so please just rs+ this.

As Masayuki suggested in comment #4, the code is taken from:
http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/toolkit/mozapps/preferences/fontbuilder.js#11 and a few lines further down.

Note that all the preferences where removed here:
https://hg.mozilla.org/mozilla-central/rev/2b99ea907d3d

However, the test sets some preferences:
Services.prefs.setCharPref("font.name.serif.x-unicode", "Foo Serif");
to my function getFont() first checks the preference and then gets the default.
Attachment #8854418 - Flags: review?(acelists)
Comment on attachment 8854418 [details] [diff] [review]
1353278-xpcshell-test.patch (v1).

>+  function getFont(aFontType, aEncoding) {
>+    var font = Services.prefs.getCharPref("font.name." + aFontType + "." + aEncoding);
>+    if (font)
>+      return font;
>+
>+    // Get the default.
>+    var enumerator = Components.classes["@mozilla.org/gfx/fontenumerator;1"]
>+                               .createInstance(Components.interfaces.nsIFontEnumerator);
>+    var fonts = enumerator.EnumerateFonts(aEncoding, aFontType, {});
>+    var defaultFont = null;
>+    if (fonts.length > 0)
>+      defaultFont = enumerator.getDefaultFont(aEncoding, aFontType);
>+    return defaultFont;
>+  }

This returns "%s" of "default (%s)", is it your intention? (I've not checked the compared value.)

So, if it compares with dropdown list label, they won't match.
Thanks for your interest.

(In reply to Masayuki Nakano [:masayuki] from comment #10)
> This returns "%s" of "default (%s)", is it your intention? (I've not checked
> the compared value.)
Yes, we compare against "Times New Roman", etc.

Frankly, I don't know what the test is for or what it does. Hence my comment "Please no discussion about the test" which was directed at Aceman, my reviewer, who had questioned the usefulness of the test in bug 1351721 comment #5.

My aim here is to port bug 1344990 to C-C as quickly as possible and evaluating the usefulness of legacy(?) tests mentioning XP and Vista is not part of that task.

Since the test is fixed, we should now focus on the functional failure we have in the font chooser. I haven't looked into it. Maybe it's retrieving the preference values and needs some code similar to what I've added to the test.
Comment on attachment 8854418 [details] [diff] [review]
1353278-xpcshell-test.patch (v1).

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

::: mail/base/test/unit/test_windows_font_migration.js
@@ +34,5 @@
>   * whatever's provided in aFonts and aNonDefaultFonts.
>   */
>  function makeVerifier(aFonts) {
> +  function getFont(aFontType, aEncoding) {
> +    var font = Services.prefs.getCharPref("font.name." + aFontType + "." + aEncoding);

Why do we have to check the pref? Why doesn't the font enumerator do that internally? Or do we somehow differ from m-c here? Is  enumerator.getDefaultFont() ignoring this pref?
Attachment #8854418 - Flags: review?(acelists) → review+
I explained all that in comment #9 ;-)

M-C is now nulling all the prefs: https://hg.mozilla.org/mozilla-central/rev/2b99ea907d3d
If they're null, they have another scheme to get the font name.

The test sets: Services.prefs.setCharPref("font.name.serif.x-unicode", "Foo Serif");
If I remove
  var font = Services.prefs.getCharPref("font.name." + aFontType + "." + aEncoding);
  ...
the test fails since it expects the pref value and not the default value.

If you want, open another bug to look at the usefulness of the test.

I hope you can help with fixing the functional failure here which causes the test failure in Mozmill. That Xpcshell stuff is just a side issue.
Attachment #8854633 - Flags: review?(jorgk)
Great, this works for me, so I'll ship this off to try with another version of the patch in bug 1353326:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b683630f50fbeb5e8b6be537f37034ca2477c1e3
Comment on attachment 8854633 [details] [diff] [review]
fix font choosers in TB prefs

That made the try run green ;-)
Attachment #8854633 - Flags: review?(jorgk) → review+
https://hg.mozilla.org/comm-central/rev/0c599e7089d8dd169b819ede2ea1fb42865543c1
https://hg.mozilla.org/comm-central/rev/5e2a6be318bf9b805eb47e9376c15d9b2c81b1b0

Assigning bug to Aceman since he fixed the bigger issue here - Thanks!
Assignee: nobody → acelists
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 55.0
You need to log in before you can comment on or make changes to this bug.