1541139 - TEST-UNEXPECTED-FAIL | comm/mail/base/test/unit/test_windows_font_migration.js | xpcshell return code: 0

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | comm/mail/base/test/unit/test_windows_font_migration.js | xpcshell return code: 0

EDIT: Wrong range, see comment #2.
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ea09774456976ada64ed753a00724ef6bc&tochange=92d1c344e7c53af9b8ad8a4192d93478f7

Hmm, something changed with Windows fonts, I can't see it right now. I'll have to check it out locally. BTW, this test only runs on Windows:
https://searchfox.org/comm-central/rev/9bc5aee7e03b167f154af5011a9f6ab286ad7bf0/mail/base/test/unit/xpcshell.ini#18

Comment 1

[Jorg K (CEST = GMT+2)]

Very strange, test fails with:
0:01.02 ERROR NS_ERROR_UNEXPECTED: Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getIntPref]
and debug shows that it got font.size.variable.x-unicode successfully and fails on font.size.fixed.x-unicode.

The test passes if I remove these two lines:

    // Assert.equal(Services.prefs.getIntPref("font.size.fixed." + aEncoding),
    //              expectedFonts.fixedSize);

Weird.

Comment 2

[Jorg K (CEST = GMT+2)]

Wrong regression range, that was already present earlier:https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=e8b3c73b4e328be88aa90f31e1fa9772b5&tochange=ea09774456976ada64ed753a00724ef6bc

So it looks like
Bug 1537575 - Cleanup generic font-family handling. r=jfkthame

Emilio, where did that preference get lost?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Actually, it's https://hg.mozilla.org/mozilla-central/rev/663c37508e2f. s/fixed/monospace on the test should do.

Comment 4

[Jorg K (CEST = GMT+2)]

Great, thanks. Well, I got the right person to talk to :-)

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1541139-monospace.patch

That needed to hit all 13 occurrences in the entire tree.

Comment 6

[:aceman]

Comment on attachment 9055318 [details] [diff] [review]
1541139-monospace.patch

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

So what about users that have that pref modified? Do we need migration code for them? Or will it somehow happen automatically?

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/7bfb148ee3d1
Port bug 1537594: s/font.size.fixed./font.size.monospace./ in C-C. rs=bustage-fix

Comment 8

[Jorg K (CEST = GMT+2)]

OK, I landed that now. I hit font.size.fixed. in the entire tree, 13 occurrences. The one in MailMigrator.jsm was necessary to get the test to pass.

Emilio, can you please look at the patch? As Aceman pointed out, what happens if people have changed the prefs?

Comment 9

[:aceman]

I tested it and now TB just uses the new prefs ignoring the old ones. So this needs decision whether we want to add migration code for this (just take all the font..fixed. prefs and move them to the new name).

Comment 10

[Jorg K (CEST = GMT+2)]

Not in this bug ;-)

Comment 11

[Emilio Cobos Álvarez (:emilio)]

There are two sets of prefs, font.size.monospace. and font.size.fixed., we're not migrating the prefs automatically because it's not clear which one should override which. We mostly used to use the monospace prefs, except in some cases.

I guess you could convert the fixed ones to monospace if the monospace one isn't set. Regarding the patch, there's a list where font.size.monospace appears twice. Looks ok otherwise.

Comment 12

[Magnus Melin [:mkmelin]]

(Doesn't sound like there's really too much need to migrate.)

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

Regarding the patch, there's a list where font.size.monospace appears twice. Looks ok otherwise.

I can't see it, maybe you can point it out or the reviewer can spot it.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9055318 [details] [diff] [review]
1541139-monospace.patch

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

::: mail/base/test/unit/test_windows_font_migration.js
@@ +128,5 @@
>   */
>  var kPrefBranchesToClear = [
>    "font.name.serif.",
>    "font.name.sans-serif.",
>    "font.name.monospace.",

Here.

@@ +130,5 @@
>    "font.name.serif.",
>    "font.name.sans-serif.",
>    "font.name.monospace.",
>    "font.size.variable.",
> +  "font.size.monospace.",

And here.

Comment 15

[Jorg K (CEST = GMT+2)]

Umm, one is size, the other name?

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Err, I'm blind, sorry :(

Comment 17

[:aceman]

(In reply to Magnus Melin [:mkmelin] from comment #12)

(Doesn't sound like there's really too much need to migrate.)

Well, users will lose their settings. Some of them may not know how to set them back (think pre-configured installs for older people and with sight problems).