No Font list in Prefs > Display - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/pref-window/test-font-chooser.js | test-font-chooser.js

RESOLVED FIXED in Thunderbird 62.0

Status

defect
--
major
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

({regression})

unspecified
Thunderbird 62.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Thunderbird-testfailure: Z all][Thunderbird-disabled-test])

Attachments

(8 attachments, 10 obsolete attachments)

2.00 KB, patch
Details | Diff | Splinter Review
2.01 KB, patch
Details | Diff | Splinter Review
23.95 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
5.24 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
3.49 KB, patch
Details | Diff | Splinter Review
1.23 KB, patch
jorgk
: review+
Pike
: review+
Details | Diff | Splinter Review
883 bytes, patch
jorgk
: review+
Details | Diff | Splinter Review
9.96 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
When opening the prefs I get:

document.l10n is undefined fontbuilder.js:46

And the font list in the prefs is empty. Probably a fallout of bug 1457021.
(Assignee)

Comment 1

a year ago
Zibi, what do we need to do to make the list work again? This would be the first time we need to use a ftl file.
Flags: needinfo?(gandalf)
Oh, fun...

Well, I assume you need Fluent. Which involves a couple things that should be easy :)

 - include l10n.js just like https://searchfox.org/mozilla-central/source/browser/components/preferences/fonts.xul#22 does
 - copy https://searchfox.org/mozilla-central/source/browser/locales/en-US/browser/preferences/fonts.ftl to some location in thunderbird and expose it. This is frankly suboptimal. Those strings should live where fontbuilder lives - in toolkit - but they don't. So you need an FTL file to supply two strings:

 - fonts-label-default
 - fonts-label-default-unnamed

 - Once you have it, you need to initialize Localization in your product, just like Firefox does it:

1) https://searchfox.org/mozilla-central/source/browser/locales/jar.mn#10 - this will make your product include omni.ja:/localization with the path to the new file
2) https://searchfox.org/mozilla-central/source/browser/components/nsBrowserGlue.js#727-730 - this will make your product register the new FileSource in L10nRegistry so that Fluent can pick it up.

I'm happy to review patches and help you debug, since it seems like we just triggered the first case where Fluent leaked onto Toolkit. Sorry for not giving you a warning - I didn't realize that it'll be it!
Flags: needinfo?(gandalf)
Alternatively, of course, you can fork the fontbuilder.js to keep using the .properties and file a bug to move the strings used by fontbuilder.js to toolkit (which is where they should be).

But then we'll have to initialize toolkit FileSource for L10nRegistry anyway in Tb (and Fx), so not that much of a win :)
(Assignee)

Comment 4

a year ago
Posted patch fluentFonts.patch (obsolete) — Splinter Review
This works for me.

The registering in L10nRegistry was already done in bug 1431260.

Zibi, do we need to do also something like https://hg.mozilla.org/mozilla-central/rev/af2f85201ec2 ? Or is this when we would migrate the complete files?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8974996 - Flags: review?(acelists)
Attachment #8974996 - Flags: feedback?(gandalf)

Comment 5

a year ago
Thanks, I'll look at this soon as it breaks the 2 tests as seen on c-c trunk.
Comment on attachment 8974996 [details] [diff] [review]
fluentFonts.patch

that looks great!

Please, monitor bug 1455649 which I hope to land next week and which will make the <script src="...l10n.js"/> unnecessary :)
Attachment #8974996 - Flags: feedback?(gandalf) → feedback+
(Assignee)

Comment 7

a year ago
Posted patch fluentFonts.patch (obsolete) — Splinter Review
Added a try{} catch (e) {} in preferences.xml to stop a NS_ERROR_FAILURE.
Attachment #8974996 - Attachment is obsolete: true
Attachment #8974996 - Flags: review?(acelists)
Attachment #8975178 - Flags: review?(acelists)

Updated

a year ago
Summary: No Font list in Prefs > Display → No Font list in Prefs > Display - TEST-UNEXPECTED-FAIL | /builds/worker/workspace/build/tests/mozmill/pref-window/test-font-chooser.js | test-font-chooser.js
Whiteboard: [Thunderbird-testfailure: Z all]

Comment 8

a year ago
Comment on attachment 8975178 [details] [diff] [review]
fluentFonts.patch

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

This looks good when playing with it manually, but the test still fails locally (can't push a try run as the tree is closed).
The error now is:
SUMMARY-UNEXPECTED-FAIL | test-font-chooser.js | test-font-chooser.js::test_font_name_displayed
  EXCEPTION: Timeout waiting for font picker serif to populate.
    at: utils.js line 406
       TimeoutError utils.js:406 13
       waitFor utils.js:444 11
       MozMillController.prototype.waitFor controller.js:687 3
       verify_advanced test-font-chooser.js:85 7
       Runner.prototype.wrapper frame.js:579 7
       startTest test-window-helpers.js:326 11
       WindowWatcher_notify test-window-helpers.js:358 9
       _verify_fonts_displayed test-font-chooser.js:98 3
       test_font_name_displayed test-font-chooser.js:124 27
Attachment #8975178 - Flags: feedback+

Comment 9

a year ago
Also visible on try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b2bee81db3d1838e2d77373965edb3392c786c44

So there may be some subtle timing issue, that is not visible manually.

Updated

11 months ago
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: Z all] → [Thunderbird-testfailure: Z all][Thunderbird-disabled-test]

Comment 11

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a15a0e87508f
temporarily disable failing test test-font-chooser.js. rs=bustage-fix

Comment 12

11 months ago
With 2018-05-15 and 2018-05-18 nightly on windows, the General options tab is shown and clicking other tabs is ineffective.  Different issue?
Flags: needinfo?(richard.marti)

Comment 13

11 months ago
It would be a different issue, if it were an issue. I can't reproduce this with 62.0a1 (2018-05-19) (64-bit) (just downloaded).

If you can reproduce reliably, please file another bug with exact STR.
(Assignee)

Comment 14

11 months ago
I don't see this issue. And then, it would be a different one for a new bug.
Flags: needinfo?(richard.marti)

Updated

11 months ago
Blocks: 1457021

Updated

11 months ago
Blocks: 1462920, 1462923
Severity: normal → major

Updated

11 months ago
No longer blocks: 1462923

Comment 15

11 months ago
Posted patch fluentFonts.patch (obsolete) — Splinter Review
Aceman and I have been looking at this for a few days/nights now.

This patch brings back the font chooser in the in-content prefs, however, the "Advanced" button still doesn't work.

Sadly, when applying the patch, pane-switching in the "regular old-fashioned" prefs doesn't work any more. So I can't even land this as a first step.

Adding the error dump and Cu.reportError(e); I see:
 [Show/hide message details.] [Exception... "Failure"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: chrome://messenger/content/preferences/preferences.xml :: _selectPane :: line 827"  data: no]
_selectPane                chrome://messenger/content/preferences/preferences.xml:827:21
showPane                   chrome://messenger/content/preferences/preferences.xml:763:13
prefwindow_XBL_Constructor chrome://messenger/content/preferences/preferences.xml:663:9
document.l10n.ready<       chrome://global/content/l10n.js:54:5

Looks this the new system is geared towards in-content prefs and we might have to kiss the "regular old-fashioned" prefs good-bye.
Attachment #8975178 - Attachment is obsolete: true
Attachment #8975178 - Flags: review?(acelists)
> Looks this the new system is geared towards in-content prefs

If you mean Fluent than no, it doesn't care and is not tied to any model. I suspect a lot of issues that Tb is facing will go away once we land bug 1455649 (in review) because it'll expose Fluent "for free" to all Gecko chrome documents.

You may also be affected by regular regressions that I need to fix like bug 1462675. Sorry for that!

Comment 17

11 months ago
Probably not Fluent itself (unless we are still missing some import of l10n.js somewhere), but the changes to /preferences/* in browser. Like removing all the preferences bindings (which we adopted for now) and switching to Preferences instead of <preference> elements. We try to follow the changes but in this case we differ from m-c for now.

Comment 18

11 months ago
Aceman beat me to an answer here ;-(

With "new system" I was referring not so much to Fluent but to its friends, like l10n.js (which is intl/l10n/l10n.js, I assume). Maybe that's more geared towards in-content prefs, which we also have in TB since TB 38, but hidden behind a pref. Most users still use the "regular old-fashioned" prefs stand-alone window that FF also had once upon a time. As Aceman said, we're still using <preference> elements and don't write the preference values straight away. Some (sub-)panels still even have a "Cancel" button allowing to discard changes made on the panel.

So preference processing has pretty much diverged from FF preferences, and moving the latter to Fluent was just the straw that broke the camel's back.

My suggestion would be to remove the "regular old-fashioned" prefs and align the code used in the in-content prefs with FF. That will a) reduce the maintenance load and b) make it easier to port things in the future.
> With "new system" I was referring not so much to Fluent but to its friends, like l10n.js (which is intl/l10n/l10n.js, I assume).

That's not true. l10n.js is just a wrapper for any document to bind it to DOMLocalization (from the Fluent package). Once again, it doesn't take any assumptions and should work all the same for any XUL/XHTML/HTML document.

And this script will be replaced with nsIDocumentL10n once bug 1455649 lands which will give `document.l10n` property available on all documents in Gecko.

> So preference processing has pretty much diverged from FF preferences, and moving the latter to Fluent was just the straw that broke the camel's back.

My concern is that Fluent may be a red herring here. I don't understand what breaks, but I don't think Fluent should impact your work except that if you include any of the Firefox preferences code you may have to provide it with Fluent l10n.js/resources now.

I can try to build Tb and debug it if you want me later this week.

Comment 20

11 months ago
Thanks for the offer, Zibi. I think it's about time we remove the stand-alone options window in TB, see bug 1465061, and announcement here: http://lists.thunderbird.net/pipermail/maildev_lists.thunderbird.net/2018-May/001192.html

We should focus on fixing the font lists behind the "Advanced" button and not waste time on getting the stand-alone window going again.
(Assignee)

Comment 21

11 months ago
I found that the "Default (*)" label wasn't shown.
It seems the

[localization] @AB_CD@.jar:
  messenger                          (%messenger/**/*.ftl)

doesn't work. As a workaround I added the fonts.ftl to copy to the normal locale directory. Now, the label is filled.

I also tried to add the

@RESPATH@/messenger/localization/*

to the package-manifest as I saw in bug 1402069, but this does also not help to create the 'localization/en-US/messenger/preferences' directory like FX does fro browser. I see nothing in this bug which is missing.

Zibi, what do we miss to create this directory automatically?
Flags: needinfo?(gandalf)

Comment 22

11 months ago
This aligns the code in fonts.js/.xul with what the Firefox version has and makes the dialog work for me (with prefs in a tab). It even converts away from <prefpane> to a <dialog> but without visible difference.
There is still one behaviour difference to Firefox, in that when you select a language where you didn't set user specified fonts (e.g. Thai), you get no fonts selected in the menulists (as if empty is selected), but you can choose a font from the list. In Firefox, in this case the default fonts get preselected in the menulists. I do not yet see where the difference may lie.
The patch contains some debugging and unneeded changes that will be removed in the final version.

This patch is to be applied on top of Paenglab's.
Attachment #8983238 - Flags: feedback?(richard.marti)
Attachment #8983238 - Flags: feedback?(jorgk)

Comment 23

11 months ago
We should really land bug 1465061 first.

(In reply to :aceman from comment #22)
> There is still one behaviour difference to Firefox, in that when you select
> a language where you didn't set user specified fonts (e.g. Thai), you get no
> fonts selected in the menulists (as if empty is selected), ...
Related to comment #21? ("Default (*)" label wasn't shown).

Updated

11 months ago
Attachment #8983238 - Flags: feedback?(richard.marti)
Attachment #8983238 - Flags: feedback?(jorgk)

Comment 24

11 months ago
Backout patch to re-enable tests.

Updated

11 months ago
Attachment #8983313 - Attachment is patch: true

Comment 25

11 months ago
I've landed bug 1465061 to cut through the Gordian Knot of the interdependency of the two bugs. Here's the rebased patch, and unless I messed up, I'm sad to say that it doesn't work. I get:

JavaScript error: about:preferences, line 1: ReferenceError: gDisplayPane is not defined
Attachment #8983314 - Flags: feedback-

Comment 26

11 months ago
Just saw it, I messed up.

Comment 27

11 months ago
Works, after I corrected the merge mistake. Sorry about the noise.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4d939a4e27b6be2e05b151352b658125b6b5ff70
Attachment #8983238 - Attachment is obsolete: true
Attachment #8983314 - Attachment is obsolete: true
Attachment #8983317 - Flags: feedback+

Comment 28

11 months ago
Comment on attachment 8983317 [details] [diff] [review]
1460721.patch - further tweaks of fonts dialog (rebased and cleaned up, take 2)

The font chooser tests still fail. But I'm landing this now, since it's been broken for much too long. Then we fix the finer points later.
Attachment #8983317 - Flags: review+

Updated

11 months ago
Attachment #8981342 - Attachment is obsolete: true

Comment 29

11 months ago
Comment on attachment 8982870 [details] [diff] [review]
fluent-fonts.patch with working label in "Display"

Thanks, working better than the previous version. I'm going to land this now. We'll have to follow up and fix the tests anyway.
Attachment #8982870 - Flags: review+

Comment 30

11 months ago
Without the non-working hunk:
[localization] @AB_CD@.jar:
  messenger                          (%messenger/**/*.ftl)
Attachment #8982870 - Attachment is obsolete: true
Attachment #8983323 - Flags: review+

Comment 31

11 months ago
Removed unnecessary "async" keyword and fixed indentation issue.
Attachment #8983317 - Attachment is obsolete: true
Attachment #8983325 - Flags: review+

Comment 32

11 months ago
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=b395522db4caa6a2ffc8377f3b08d07b0e1cd82d
failed with
Missing file(s): Thunderbird Daily.app/Contents/Resources/messenger/localization/*

Looks like Richard already knew that, see comment #21.

Comment 33

11 months ago
Thanks for the cleanup.
Does the "Default" label work now? Also, are the menulists for other languages populated now?
The function FontBuilder.readFontSelection() does return empty when the default font should be used, but I do not understand how that is converted to display the default font (in Firefox prefs).

The tests may still fail as we probably need to add some more waiting into them as the menulists are populated even more async now and it is so slow I can see it redrawing. Oh the progress...

Comment 35

11 months ago
(In reply to :aceman from comment #33)
> Does the "Default" label work now? Also, are the menulists for other
> languages populated now?
Yes, as far as I can see both work. 

As soon as the try run is done, I'm going to land this, so we can stop shuffling various versions around in multiple bugs and focus on the real issue. Maybe it still won't work on server builds, but at least it appears to be working locally.

Comment 36

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2ffa15315fa9
Port bug 1457021 to TB: Migrate the JS of Preferences::Fonts to Fluent. r=zibi,jorgk
https://hg.mozilla.org/comm-central/rev/045dc8345aea
Align font preferences processing with M-C. r=jorgk

Comment 37

11 months ago
OK, let's take it from here:
- fix tests
- fix packaging issues
- further issues, like:
  _readDefaultFontTypeForLanguage() is different in M-C. Do we need this?
  https://hg.mozilla.org/comm-central/rev/045dc8345aea#l2.159
  preference = document.createElementNS("...", "preference");

Comment 38

11 months ago
Creating of <preference> elements will go once we convert all the pref panes to Preferences object. Now only fonts.* was fully converted. The conversion could mimic the m-c patch for that. We should file a new bug for it.

Comment 39

11 months ago
Looks like the landings in comment #36 busted the Linux builds:
make[1]: *** [automation/l10n-check] Error 2

And the log says:
[task 2018-06-05T09:55:11.863Z] 09:55:11     INFO -  l10n-check> RuntimeError: File "chrome/messenger/preferences/fonts.ftl" not found in /builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales/merge-dir/x-test/mail, /builds/worker/workspace/build/src/obj-thunderbird/comm/mail/locales/x-test/mail

So who can help us out with this? Zibi sadly hasn't replied, so let's make some noise.
Flags: needinfo?(gandalf) → needinfo?(l10n)

Updated

11 months ago
Flags: needinfo?(gandalf)

Comment 40

11 months ago
Looks like I broke Windows as well :-( - The only platform that's building is Mac and that's the one I used for the try. I'd rather not back this out if we can fix the build issues quickly. It's really painful not to have a build person, since all those build issues come back to regular developers.
Flags: needinfo?(francesco.lodolo)
(Assignee)

Comment 41

11 months ago
Maybe affected what I asked in comment 4 about porting https://hg.mozilla.org/mozilla-central/rev/af2f85201ec2. Maybe the .ftl files aren't automatically added to l10n like the .dtd and .properties.
Sorry, I can't really help when it comes to build system.

Re comment 4: migration is completely unrelated, it's about moving strings from DTD/properties to FTL files to avoid retranslation, it doesn't affect build.
Flags: needinfo?(francesco.lodolo)

Comment 43

11 months ago
Let me take this off of everybody else's radar, I'll follow up.

The jar.mn part won't work in https://hg.mozilla.org/comm-central/rev/2ffa15315fa9#l4.12, you'll need to get back to the earlier versions. I don't have a build to try out, and won't for a couple of hours, but here's my thinking:

hg mv mail/locales/en-US/chrome/messenger/preferences/fonts.ftl mail/locales/en-US/messenger/preferences/fonts.ftl

i.e., move it away from /chrome/

Remove the line that packs it as chrome in jar.mn.

Add a hunk like 

[localization] @AB_CD@.jar:
  messenger                          (%messenger/**/*.ftl)

That hunk didn't work for you on try because you had the file landed in en-US/chrome/messenger instead of en-US/messenger.
Flags: needinfo?(l10n)
Flags: needinfo?(gandalf)

Comment 45

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1e99d17fb28f
Follow-up: Fix packaging issues. rs=bustage-fix CLOSED TREE DONTBUILD

Comment 46

11 months ago
The Daily run based in this looks successful. Richard, can you confirm? So now we need to fix the test.
(Assignee)

Comment 47

11 months ago
Yes, also the Advanced dialog works. :-)
(Assignee)

Comment 48

11 months ago
my comment 47 was wrong. It worked only because the fonts.ftl was still in my normal locales directory. Removing it showed again the broken "Advanced" dialog.

It doesn't work because the path for the L10nRegistry wasn't correct. This patch fixes it for me.
Attachment #8983894 - Flags: review?(jorgk)

Comment 49

11 months ago
Comment on attachment 8983894 [details] [diff] [review]
Bug1460721-fup.patch

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

This looks right to me.
Attachment #8983894 - Flags: review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 50

11 months ago
Comment on attachment 8983894 [details] [diff] [review]
Bug1460721-fup.patch

I'm really not the expert here. I'll land with r=Pike, rs=jorgk.
Attachment #8983894 - Flags: review?(jorgk) → review+
(Assignee)

Updated

11 months ago
Keywords: checkin-needed

Comment 51

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/82206ad8748f
Follw-up: Fix the localization link in mailGlue.js. r=Pike, rs=jorgk
Keywords: checkin-needed
(Assignee)

Comment 52

11 months ago
This all works now on my local build. But in the Dailies it doesn't work. I found that the localization files (also the devtools file) aren't packaged in the omni.ja. Manually packaging the "localization" directory in the root of the omni.ja makes it working.

Pike or Zibi, do you know what is missing during the packaging?
Flags: needinfo?(l10n)
Flags: needinfo?(gandalf)

Comment 55

11 months ago
Like this? We won't use "browser" as in: @RESPATH@/browser/localization/*

Richard, you can package locally, can't you?
Attachment #8984869 - Flags: review?(l10n)
Attachment #8984869 - Flags: feedback?(richard.marti)

Updated

11 months ago
Attachment #8984868 - Flags: review?(jorgk) → review+

Comment 56

11 months ago
Comment on attachment 8984869 [details] [diff] [review]
1460721-fix-packaging-take2.patch

Richard was faster, I assume he tested his try run.
Attachment #8984869 - Attachment is obsolete: true
Attachment #8984869 - Flags: review?(l10n)
Attachment #8984869 - Flags: feedback?(richard.marti)
(Assignee)

Comment 57

11 months ago
Yes, I tested it.
Keywords: checkin-needed

Comment 58

11 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/13b6979b94b0
Follow-up: Add the localization path to the package manifest. r=jorgk
Keywords: checkin-needed

Comment 59

10 months ago
Posted patch 1460721-tests.patch (obsolete) — Splinter Review
This should update the font chooser tests for the new behaviour when displaying the default fonts.
Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central
Attachment #8989022 - Flags: review?(jorgk)

Comment 60

10 months ago
Comment on attachment 8989022 [details] [diff] [review]
1460721-tests.patch

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

I can't say that I enjoyed reading this code. But great that we can finally close this bug.

Let's wait for the Mac result, I guess it will be closer to Linux. In this case, check for "darwin". Did you know that Darwin is the name of the BSD system onto macOS is built?

::: mail/test/mozmill/pref-window/test-font-chooser.js
@@ +96,5 @@
> +      // only the label, in the form "Default (font name)".
> +      if (AppConstants.platform == "linux") {
> +        // On Linux the prefs we set contained only the generic font names,
> +        // like 'serif', but here a specific font name will be shown, but it is system
> +        // dependent what it will be. So we just check for the 'Default' prefix.

system-dependent.
http://www.btb.termiumplus.gc.ca/tcdnstyl-chap?lang=eng&lettr=chapsect2&info0=2.03

@@ +141,5 @@
>    let fontTypes = kFontTypes.map(fontType => expected[fontType]);
> +  _verify_fonts_displayed(false, ...fontTypes);
> +
> +  for (let [fontType, fontList] of Object.entries(gRealFontLists)) {
> +    Services.prefs.clearUserPref("font.name." + fontType + "." + kLanguage);

Do we need this here or can this go to teardown?

@@ +192,5 @@
>    let fontTypes = kFontTypes.map(fontType => expected[fontType]);
> +  _verify_fonts_displayed(true, ...fontTypes);
> +
> +  for (let [fontType, fakeFont] of Object.entries(kFakeFonts)) {
> +    Services.prefs.clearUserPref("font.name." + fontType + "." + kLanguage);

Should this go to the teardown as well?
Attachment #8989022 - Flags: review?(jorgk) → review+

Comment 61

10 months ago
(In reply to Jorg K (GMT+2) from comment #60)
> Let's wait for the Mac result, I guess it will be closer to Linux. In this
> case, check for "darwin". Did you know that Darwin is the name of the BSD
> system onto macOS is built?

Sure.
 
> @@ +141,5 @@
> >    let fontTypes = kFontTypes.map(fontType => expected[fontType]);
> > +  _verify_fonts_displayed(false, ...fontTypes);
> > +
> > +  for (let [fontType, fontList] of Object.entries(gRealFontLists)) {
> > +    Services.prefs.clearUserPref("font.name." + fontType + "." + kLanguage);
> 
> Do we need this here or can this go to teardown?
> Should this go to the teardown as well?

OK, to teardownTest() actually.

Comment 62

10 months ago
Attachment #8989022 - Attachment is obsolete: true
Attachment #8989027 - Flags: review+

Updated

10 months ago
Attachment #8989027 - Flags: approval-comm-beta+

Comment 63

10 months ago
Most parts landed on TB 62 and the test fix will be uplifted, so making the Target Thunderbird 62.
Keywords: leave-open
Target Milestone: --- → Thunderbird 62.0

Comment 64

10 months ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bc158a134fe5
fix test-font-chooser.js tests. r=jorgk
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.