Closed Bug 465633 Opened 13 years ago Closed 12 years ago

Switch to better default fonts on platforms which have them

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.1b2

People

(Reporter: faaborg, Assigned: rain1)

References

Details

(Keywords: polish)

Attachments

(7 files, 6 obsolete files)

Many platforms like XP and OS X do not include a great monospaced font, which makes reading plan text email an inelegant experience.  Shipping a good default monospaced font is a nice way to make an app feel modern and give it an extra level of polish, since users are so accustomed to the typewriter hell called courier.

So far I think the best monospaced font out there is Inconsolata, which features micro-serifs.  Unfortunately this font is still in development:

http://www.levien.com/type/myfonts/inconsolata.html

A second still decent alternative is Deja Vu Sans, which was featured in Coda as "Panic Sans":

http://dejavu.sourceforge.net/wiki/index.php/Main_Page
It'd be good to get an idea of the implementation work needed to make this happen.
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3?
I have just taken a look at the Deja Vu Sans Mono and Inconsolata.  Inconsolata is far from meeting the needs of Tb in terms of multi-lingual support. Deja Vu Sans on the other hand includes over 3,000 glyphs covering the Latin1 and Latin2 based languages plus Arabic, Lao and others. These are True Type outlines packaged according to the Open Type Layout specification.  That means they are Windows and Mac OSX compatible. Each of the three font families fulfill the standard Windows 4 face set, being Regular, Regular Italic, Bold and Bold Italic.

I recommend the Deja Vu suite of font faces for consideration and evaluation of license fit with Mozilla standards.
If anyone knows of a better monospaced font than the ones mentioned, please be sure to mention it.  I haven't really done a thorough search, these are just a few fonts that caught my eye as being particularly good.
I think this is a really good idea.  I'd really like to have an idea of about how much work would be involved if we did find a font that matched our criteria. 

Here are a couple of free font that I know of plus an article I had read a while back with some good resources on other open source / free fonts.

Bitstream ( AFAIK work here was continued in the Deja Vu fonts )
http://www.gnome.org/fonts/

Liberation ( licensed under the GPL + exception, might be a problem )
https://www.redhat.com/promo/fonts/

Open Source Fonts ( RH Mag article )
http://www.redhatmagazine.com/2007/12/13/open-source-fonts/
(In reply to comment #4)
> Bitstream ( AFAIK work here was continued in the Deja Vu fonts )
> http://www.gnome.org/fonts/

The Bitstream Vera font suite was the seed for the Deja Vu fonts. An plus for the Deja Vu fonts is they are in active development. The fonts at gnome.org have not been updated since Bitstream published them at gnome.org.

> 
> Liberation ( licensed under the GPL + exception, might be a problem )
> https://www.redhat.com/promo/fonts/
> 
> Open Source Fonts ( RH Mag article )
> http://www.redhatmagazine.com/2007/12/13/open-source-fonts/

These references I will look at.
(In reply to comment #4)
> Here are a couple of free font that I know of plus an article I had read a
> while back with some good resources on other open source / free fonts.
> 
> Liberation ( licensed under the GPL + exception, might be a problem )
> https://www.redhat.com/promo/fonts/

I have taken a look at Liberation and it is inferior to Deja Vu in its range of characters, 600+ versus 3,000+.  The initial design work was done by Ascender Corporation, the holder of all the MS Typography copyrights. The Liberation font name is Trademark by Red Hat. The RH GPL+exception license permits modification,  redistribution, and embedding of the fonts.

For Thunderbird, We do not have who can work with expanding the glyph set of Liberation to cover the range of languages that Deja Vu currently covers.
Based on what Thunderbird does in terms of implementation and license issues we might want to use a similar approach for improving the default fixed width font in Firefox, for text areas and view source (or vice versa).  Filed follow up bug 465633 to cover adding a new font to Firefox.
er, bug 468169 for Firefox
Ignoring licensing work, which I assume would be taken care of by Mozilla legal, would this be technically possible to get in for the next beta?  Ronald?
I like this idea as well.  As far the technical details, this would be the first Gecko app that I'm aware of to ship its own font (not that I'd be all that likely to know if someone had done it already).  I've added pavlov and roc to the CC in the hopes that one of them can talk about what sort of work this would likely entail...
You're on 1.9.1 right? In principle you could use CSS @font-face to load the font in each document where you need it. The main problem with that is we don't cache these fonts very effectively, so we'll create a lot of platform font objects which will probably increase memory usage quite a bit. So either we could invest in fixing that problem, or we could create another mechanism that allows for app-wide font embedding.
What I thought would be an issue of bundling fonts would be Installer related. What would have to be done to get the installer to copy the fonts into the OS system fonts library. Essentially giving the system some additional fonts the User could use with any installed application.

That way Gecko can use it's existing methods to access and use the bundled fonts. That the default themes would have there font-family CSS revised to utilize our font package and that could yeald more consistent X-platform chrome appearance. Another gain is a style consistency between Serif and Sans-Serif families in the case of the Deja Vu set of font families.
roc: We are on 1.9.1, yeah.  When you say "a lot of platform font objects", what's the order of magnitude we're talking about?  One per glyph?  One per document?

Bundling OS-level fonts could be an option too.
One per document.

Just installing the font into the OS sounds better to me.
One per document doesn't sound like much to me; what makes you prefer the installation approach?

Adding robstrong & Mossop to the CC for their thoughts on the installing-into-the-OS approach...
(In reply to comment #11)
> You're on 1.9.1 right? In principle you could use CSS @font-face to load the
> font in each document where you need it. The main problem with that is we don't
> cache these fonts very effectively, so we'll create a lot of platform font
> objects which will probably increase memory usage quite a bit. So either we
> could invest in fixing that problem, or we could create another mechanism that
> allows for app-wide font embedding.

I'm going to tackle better caching of activated fonts in bug 468568.

There's actually a third way, which is to simply load a set of fonts application wide at startup (or just after startup).  Work done for the @font-face implementation means that it would be fairly simple to add code that loads the fonts in given folder at runtime and treats them as normal system fonts.  Not an option for 1.9.1 but definitely possible for 1.9.2.

As for fonts, Droid Mono might be another option although it ships with an Apache license from what I've heard.
(In reply to comment #15)
> One per document doesn't sound like much to me; what makes you prefer the
> installation approach?

I don't know how many documents Thunderbird creates. But assuming you pick a font with broad Unicode coverage, we could be talking hundreds of kilobytes per document.
(In reply to comment #15)
> One per document doesn't sound like much to me; what makes you prefer the
> installation approach?
> 
> Adding robstrong & Mossop to the CC for their thoughts on the
> installing-into-the-OS approach...
It is relatively simple to install fonts using NSIS on Windows as long as the user is running as administrator
http://nsis.sourceforge.net/Register_Fonts

I verified the sample above works by installing all of Deja Vu on my system. I can look into how to accomplish this without the plugin if you decide to go forward with this. Just let me know which font you decide upon and where you will land it in the repo
(In reply to comment #17)
> (In reply to comment #15)
> > One per document doesn't sound like much to me; what makes you prefer the
> > installation approach?
> 
> I don't know how many documents Thunderbird creates. But assuming you pick a
> font with broad Unicode coverage, we could be talking hundreds of kilobytes per
> document.

One per message, so presumably a somewhat similar order of magnitude to Firefox (assuming one per web page).  An individual temporary cost of 100K per message doesn't seem that big of a deal to me, but, on the other hand, if these objects hang around forever even after the message disappears (without object recycling), that'd be different.  Is the latter the case?  If not, I'm not understanding the details of your concern.
Probably more than 100K if you use a font that has broad Unicode coverage, which you would. Anyway, 100K per message document sounds like a lot to me.

But I think installing the font in the installer is a clear winner here so we don't need to worry about this anymore.
(In reply to comment #20)
> Probably more than 100K if you use a font that has broad Unicode coverage,
> which you would. Anyway, 100K per message document sounds like a lot to me.
> 
> But I think installing the font in the installer is a clear winner here so we
> don't need to worry about this anymore.

On my Vista system, just the Arial Unicode MS font face is 22,731 K Bytes. One of the Chinese font files is 26,852 K Bytes. The Deja-Vu Sans family has faces ranging from 300 to 600 K Bytes. 

There is also another method to offer the fonts.  That is as a Font Pack at AMO, in the same way Language Packs are offered. Then we add the desired font into the CSS and if the system has Deja-Vu, etc. it would be picked up from the system. The Font Pack option then keeps the shipping application package smaller.
Flags: blocking-thunderbird3? → blocking-thunderbird3-
Given that theory up until now was that we'd have to find and ship a separate
font, it has been the right call not have this block Tb3.  However, from
talking to Mossop and sid0, it sounds like there's extremely low-hanging fruit
to be had for OS X and Vista because they ship with good fonts that we could
just put in the default pref files.  I believe that anything that low-hanging
and high-value should block; resetting the flags.  sid0, Mossop, what specific
fonts do you suggest we use as defaults?
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Obviously font choice can be very subjective, but here is what I find vastly better on OSX:

Sans-serif: Lucida Grande
Monospace: Monaco

I set the default proportional font to sans-serif since I find that far more readable, so I haven't really got a recommendation for a serif font. I'm pretty sure the default of Times probably could be improved on though.
For Vista and above versions of Windows, the best choices probably are:

Sans-serif: Calibri, <http://en.wikipedia.org/wiki/Calibri>
Monospace: Consolas, <http://en.wikipedia.org/wiki/Consolas>

I agree with Mossop on switching the default proportional font to sans-serif
(it's notable that Office has switched from Times New Roman to Calibri as the
default font for both screen and print). As for serif, Cambria might be a good
choice, but I'm less sure there as I don't really see serif fonts in the UI.

Serif: possibly Cambria, <http://en.wikipedia.org/wiki/Cambria_%28typeface%29>

I'm hesitant about recommending these fonts for XP and below, even if they're
installed with e.g. Office or the PowerPoint Viewer, as they look really bad
with Cleartype off, and XP doesn't have Cleartype on by default.
(In reply to comment #24)
> For Vista and above versions of Windows, the best choices probably are:
> 
> Sans-serif: Calibri, <http://en.wikipedia.org/wiki/Calibri>
> Monospace: Consolas, <http://en.wikipedia.org/wiki/Consolas>
> 
> I agree with Mossop on switching the default proportional font to sans-serif
> (it's notable that Office has switched from Times New Roman to Calibri as the
> default font for both screen and print). As for serif, Cambria might be a good
> choice, but I'm less sure there as I don't really see serif fonts in the UI.
> 
> Serif: possibly Cambria, <http://en.wikipedia.org/wiki/Cambria_%28typeface%29>
> 
> I'm hesitant about recommending these fonts for XP and below, even if they're
> installed with e.g. Office or the PowerPoint Viewer, as they look really bad
> with Cleartype off, and XP doesn't have Cleartype on by default.

I concur for Vista suggestions. They have much better Unicode support that the MS Web fonts shipped with XP. The Deja-Vu would be a good fit for Linux, and many of community are already familiar with the fonts.
Hmm, need an owner to stay on the blocking list, who could we possibly find, who's demonstrated an interest in it? :)
Assignee: nobody → dmose
Target Milestone: --- → Thunderbird 3.0b3
Whiteboard: [b3ux]
Status: NEW → ASSIGNED
Whiteboard: [b3ux] → [b3ux] [m3]
Whiteboard: [b3ux] [m3] → [b3ux][m3]
Whiteboard: [b3ux][m3] → [b3ux][m4]
While this would be a great thing to have if someone feels like jumping on this, if this were the last bug standing, we wouldn't wait for it.  Switching from blocking+ to wanted+.
Assignee: dmose → nobody
Flags: wanted-thunderbird3+
Flags: blocking-thunderbird3-
Flags: blocking-thunderbird3+
Whiteboard: [b3ux][m4]
Target Milestone: Thunderbird 3.0b3 → ---
Here's a tentative strategy that I think we can take for choosing font types which already ship with the different OSes.  The major change is defaulting to sans-serif and choosing a better sans-serif font.  We need to check with each change that the fonts look relatively the same size as changing fonts tends to make the text look larger and smaller due to differences in the font designs even though they are the same pixel sizes.

For sizes with a ? I'm not sure what the optimum size is yet but I believe that size will work or that I'm not sure what the current default size is.  Sans-serif and Serif fonts share the same size value.

Mac:

Default     : sans-serif
Sans-Serif  : Lucida Grande : 15?
Serif       : Times         : 16?
Mono        : Monaco        : 12

Windows:

Default     : sans-serif
Sans-Serif  : Calibri       : ???
Serif       : Times         : 16?
Mono        :               :

Linux:

Default     : sans-serif
Sans-Serif  : DejaVu Sans-Serif   : ???
Serif       : DejaVu Serif        : ???
Mono        : DejaVu Sans Mono    : ???

The rest of this bug should be devoted to actually shipping a better font.
>Windows

It is worth having different values from [2000/XP] and [Vista/7], with Vista Microsoft invested pretty heavily in font design and introduced a number of new fonts that are considerably better than their older ones.  For instance Consolas in particular is an extremely strong monospaced font: http://en.wikipedia.org/wiki/Consolas

This is something I've been pushing on really hard with the Firefox team, but if we keep considering Windows to be a single platform (since there is no compilation difference), we will end up looking and feeling around 10 years out of date on modern versions of Windows.  Windows really needs to be considered to be two entirely separate platforms at this point.
So I've got a patch with the fonts themselves changed, but there's a bunch left
to do.  In particular, the font sizes haven't been tuned on any of the
platforms as described in comment 28 yet.

There also needs to be some testing to confirm that the font names are all
correct and that we're not executing some OS font manager fallback because I
misspelled something or because of bugs in our font handling code.  In an ideal
world, that would be automated, but my suspicion is that this isn't exposed to
MozMill (via the DOM or some other mechanism) in an easy-to-access way.  If
someone knows that that suspicion is incorrect, I'd love to hear it!  If not,
we'll need to do some manual testing.

Short version is that I'll attach the patch, but it's not going to land
tonight, I don't think.  It _might_ want to land tomorrow with a post-freeze
approval, I'll talk to clarkbw and Standard8 about that in the morning.

In poking around here, I noticed that the OS/2 portion of the preferences often
uses multiple fonts for fallback.  One thing we could (should?) do here is do
the same thing, keeping the old fonts last in the list, for the case when the
new fonts we're choosing are missing glyphs.  That might have performance
implications, however.  Knowledge from folks with more experience here highly
desired.  :-)

Just about to submit this and collided with Faaborg's comment.  The patch I'm about to attach actually uses Consolas, at sid's suggestion from an earlier comment.  I haven't tested on either Windows platform yet, so I haven't run into any pain caused by that.  The trick for tomorrow, I think, is going to be "what's the minimum change we can put it at the last minute before b1" so that it gets tested sooner rather than later, but isn't overly risky.  It's entirely possible that the answer there is "nothing, we should slip this to b2."  More tomorrow...
Attached patch WIP patch, v1 (obsolete) — Splinter Review
(In reply to comment #30)
> Just about to submit this and collided with Faaborg's comment.  The patch I'm
> about to attach actually uses Consolas, at sid's suggestion from an earlier
> comment.

Calibri/Consolas may or may not be available on Windows XP or below, and looks particularly bad without Cleartype enabled, so we definitely need a version-specific statement somewhere. Maybe depending on the Windows version we could modify font prefs at first run?
(In reply to comment #30)
> 
> Short version is that I'll attach the patch, but it's not going to land
> tonight, I don't think.  It _might_ want to land tomorrow with a post-freeze
> approval, I'll talk to clarkbw and Standard8 about that in the morning.

That would be great and would give a second item to test on Thursday.
My day is looking crazy, and Sid has graciously offered to pick this up and run with it a bit to see if we end up with something that we can reasonably ship as part of 3.1b1.
Assignee: nobody → sid.bugzilla
blocking-thunderbird3.1: --- → beta2+
Flags: wanted-thunderbird+
(In reply to comment #28)
> Mac:
> 
> Default     : sans-serif
> Sans-Serif  : Lucida Grande : 15?
> Serif       : Times         : 16?
> Mono        : Monaco        : 12

For what it's worth, Menlo is a new monospaced font (based off Bitstream Vera Sans Mono) that's shipping with Snow Leopard:
http://arstechnica.com/apple/news/2009/06/font-changes-coming-to-mac-os-x-snow-leopard.ars

Granted, it wouldn't be available for 10.5 users, but I don't suppose there would be any way to have a font stack to specify Menlo if it's available, but with a fall-back to Monaco or the like?
We're resetting the blocking flag for 3.1 on this bug and instead setting the
wanted-thunderbird+ flag. We have too many blocking-3.1 bugs, to the point
where it doesn't mean much, and managing the list is making it hard to actually
work on closing bugs, which helps no one.

Thunderbird 3.1's primary purpose is to allow us to offer a prompted major
update to Thunderbird 2 users, to ensure their continued ability to safely use
Thunderbird.  Thunderbird 2 is built on an outdated version of Gecko, and our
long-term ability to maintain the users' safety for Thunderbird 2 users is
limited.

If you think this bug meets the requirements below, please renominate with a
detailed explanation of how it meets the following two criteria, and we will
reconsider.  To qualify, this bug must either:

a) make the upgrade experience from TB2 very painful for a large number of
users

or

b) be a new, reproducible, severe quality issue (eg dataloss, frequent crashes)

Just because this bug doesn't block TB3.1 doesn't mean it can't or won't make
the release.  Once they're done with their blockers (if any), we encourage
developers to keep working on non-blocking bugs, and to try to land them as
early in the cycle as possible, as non-blocking bugs will become increasingly
difficult to land in the later stages of the cycle.
blocking-thunderbird3.1: beta2+ → ---
Attached patch WIP (obsolete) — Splinter Review
OK, I've tried filling in some defaults -- I haven't looked at font sizes though.

The cleartype fonts seem to be slightly smaller than the non-cleartype fonts (see upcoming attachment). I've attempted to use whatever value the user currently has and apply a scaling factor to it (though I guess 1.2 is a bit too much). This generally works, but the problem is that the font size chooser in the display prefs isn't an editable menulist, so it can't display values like 19.

Editable menulists have their own problems -- you can type in anything -- all of which would be fixed if the "editable" were a textbox, since you could just specify type="number" and min/max values.

Options that I know of, in order from least to most work:
1. Don't scale at all.
2. Clobber the user font size pref if we're migrating the font.
3. Leave the editable menulists as they are -- it's The User's Fault (TM) if he enters the wrong thing in there.
4. Fix editable menulists, by possibly writing an XBL binding.

I'm currently leaning towards either option 1 or option 2, with the rationale that a relatively small number of people would choose a non-default font size. Bryan, thoughts?
Attachment #428598 - Attachment is obsolete: true
Best viewed in Firefox on a Vista/7 computer.
FWIW, I think Calibri at 16px and Consolas at 14px are fine.
Attachment #430266 - Attachment is patch: false
Attachment #430266 - Attachment mime type: text/plain → image/png
DirectWrite is the future, so we might want to keep this in mind.
Attachment #430262 - Flags: feedback?(clarkbw)
Comment on attachment 430262 [details] [diff] [review]
WIP

(In reply to comment #37)
> Created an attachment (id=430262) [details]
> WIP
> 
> OK, I've tried filling in some defaults -- I haven't looked at font sizes
> though.
> 
> The cleartype fonts seem to be slightly smaller than the non-cleartype fonts
> (see upcoming attachment). I've attempted to use whatever value the user
> currently has and apply a scaling factor to it (though I guess 1.2 is a bit too
> much). This generally works, but the problem is that the font size chooser in
> the display prefs isn't an editable menulist, so it can't display values like
> 19.
> 
> Editable menulists have their own problems -- you can type in anything -- all
> of which would be fixed if the "editable" were a textbox, since you could just
> specify type="number" and min/max values.
> 
> Options that I know of, in order from least to most work:
> 1. Don't scale at all.
> 2. Clobber the user font size pref if we're migrating the font.
> 3. Leave the editable menulists as they are -- it's The User's Fault (TM) if he
> enters the wrong thing in there.
> 4. Fix editable menulists, by possibly writing an XBL binding.
> 
> I'm currently leaning towards either option 1 or option 2, with the rationale
> that a relatively small number of people would choose a non-default font size.
> Bryan, thoughts?

This is awesome!

I would lean toward 2 and perhaps we just edit the menu list values to include the sizes we feel are appropriate.  I see that it just doesn't include the value 19.  There is a lot more we could do to improve font prefs choosing in a separate bug.

I'm wondering if we should be using the migrator for using Menlo as Alex mentioned in comment 35
Attachment #430262 - Flags: feedback?(clarkbw) → feedback+
(In reply to comment #41)
> I'm wondering if we should be using the migrator for using Menlo as Alex
> mentioned in comment 35

I was sort of hoping that the font-list pref would do that -- could someone on OS X apply and test this?
Attached patch WIP 2 (obsolete) — Splinter Review
OK, I've gone with 2. This needs further testing and signoff on
- Mac, including using Menlo on 10.6 and Monaco on 10.5
- Linux
- Font sizes.

The try server should hopefully spit out builds in a couple of hours.
Attachment #430262 - Attachment is obsolete: true
Attachment #430383 - Flags: review?
Attachment #430383 - Flags: feedback?(clarkbw)
Attachment #430383 - Flags: feedback?(bwinton)
Attachment #430383 - Flags: review?
Comment on attachment 430383 [details] [diff] [review]
WIP 2

Tested on the Mac, not snow leopard but the Monaco works great.  

I would bump each font size down by one point in order to remain consistent size with previous font sizes.  So I have 15 and 12 for sizes that work.
Attachment #430383 - Flags: feedback?(clarkbw) → feedback+
I like it on Snow Leopard.

And I downloaded the Linux version, but couldn't get it to run…

Later,
Blake.
Attachment #430383 - Flags: feedback?(bwinton) → feedback+
Looks good with the try build on my Debian stable computer. It actually looks exactly the same as before, as the default was already DejaVu Sans Mono.
You may take a look into bug 546877, before fixing this. We have regression, when TB doesn't render property monospace fonts.
Depends on: 550589
Sorry about not posting Windows try server builds.

Windows zip:

<http://s3.mozillamessaging.com/build/try-server/2010-03-04_13:00-sid.bugzilla@gmail.com-better-fonts-2/sid.bugzilla@gmail.com-better-fonts-2-mail-try-win32.zip>

Windows installer:

<http://s3.mozillamessaging.com/build/try-server/2010-03-04_13:00-sid.bugzilla@gmail.com-better-fonts-2/sid.bugzilla@gmail.com-better-fonts-2-mail-try-win32.installer.exe>

For those of you coming from the newsgroup, note that these builds do not yet have Menlo on Snow Leopard -- they'll still use Monaco. Bug 550589 will need to be fixed before we can do the switch.
I feel that with Calibri at 16 the font used for summaries becomes a little too small. It seems to be better at 17 -- I'll post pre/post screenshots to aid in ui-review.
Summary: Ship a better monospaced font for plain text email messages → Switch to better default fonts on platforms which have them
Attached image normal text, win7
Attached image Summary text, win7
Attached image Gloda search text, win7
Whiteboard: [has patch for review]
I've tested the Mac build (and the patch) on OS X 10.6 and I like it. Sometimes you have to look twice to see the difference, but all in all it looks more modern now, I think. :)
Comment on attachment 433709 [details] [diff] [review]
patch, v1

BenB beat me to the race to land a migration module, so I'll have to update my code to that. :)
Attachment #433709 - Flags: review?(bwinton)
Attached patch patch, v1.5 (obsolete) — Splinter Review
No changes apart from naming. bienvenu, could you please sr the name changes?
Attachment #433709 - Attachment is obsolete: true
Attachment #434955 - Flags: superreview?(bienvenu)
Attachment #434955 - Flags: review?(bwinton)
Attachment #433709 - Flags: ui-review?(clarkbw)
Attachment #434955 - Flags: ui-review?(clarkbw)
Attachment #434955 - Flags: superreview?(bienvenu) → superreview+
Attachment #434955 - Flags: ui-review?(clarkbw) → ui-review+
Attached patch patch, v1.5 unbitrotted (obsolete) — Splinter Review
Fixed a slight amount of bitrot.

(+ review ping :) )
Attachment #434955 - Attachment is obsolete: true
Attachment #436325 - Flags: review?(bwinton)
Attachment #434955 - Flags: review?(bwinton)
Blake, if you won't be able to get to this soon, please let me know.
Attachment #436325 - Attachment is obsolete: true
Attachment #436561 - Flags: review?(bwinton)
Attachment #436325 - Flags: review?(bwinton)
Comment on attachment 436561 [details] [diff] [review]
patch v1.5, unbitrotted x2

>+++ b/mail/app/profile/all-thunderbird.js
>@@ -498,16 +498,59 @@ pref("mailnews.migration.header_addons_u
>+#ifdef XP_MACOSX
[snip…]
>+#endif
>+
>+// Since different versions of Windows need different settings, we'll handle
>+// this in MailMigrator.js.
>+
>+// Linux, in other words.  Other OSes may wish to override.
>+#ifdef UNIX_BUT_NOT_MAC
[snip…]
>+#endif
>+
>+pref("mail.font.windows.version", 0);

Shouldn't this be in some sort of Windows-only block?

>+++ b/mail/base/content/msgMail3PaneWindow.js
>@@ -43,17 +43,17 @@
>+Components.utils.import("resource://app/modules/MailnewsMigrator.js");
>@@ -311,16 +311,18 @@ function LoadPostAccountWizard()
>+  Components.utils.import("resource://app/modules/MailMigrator.js");

Didn't you already do that up on line 43?

>+  MailMigrator.migrateMail();

Also, it's not really Mail you're migrating.  Can you think of a better
name for this, like migratePrefs, or something?

>+++ b/mail/base/modules/MailMigrator.js
>@@ -0,0 +1,131 @@
>+ * The Initial Developer of the Original Code is
>+ * the Mozilla Foundation.
>+ * Portions created by the Initial Developer are Copyright (C) 2010
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Siddharth Agarwal <sid.bugzilla@gmail.com>

Congratulations!  I think this is the first code I've reviewed that's
gotten these right first time.  ;)

>+var MailMigrator = {
>+  /**
>+   * Switch the given fonts to the given encodings, but only if the current fonts
>+   * are defaults.

You could break that at a better place to keep it under 80 characters.

>+   */
>+  _switchFonts: function MailMigrator__switchFonts(aFonts, aEncodings) {

What about naming it "_switchDefaultFonts"?

>+  /**
>+   * Migrate to ClearType fonts (Cambria, Calibri and Consolas) on Windows Vista
>+   * and above.
>+   */
>+  migrateToClearTypeFonts: function MailMigrator_migrateToClearTypeFonts() {
>+    // Windows...
>+    if ("@mozilla.org/windows-registry-key;1" in Components.classes) {
>+      // Only migrate on Vista (Windows version 6.0) and above
>+      let sysInfo = Cc["@mozilla.org/system-info;1"]
>+                      .getService(Ci.nsIPropertyBag2);
>+      if (sysInfo.getPropertyAsDouble("version") >= 6.0) {

So, we only migrate to ClearType fonts if we're on Vista and above, but
we're also migrating Mac and Linux.  Shouldn't we migrate older Windows
versions to something non-ClearType-but-still-nicer?

>+++ b/mail/base/modules/Makefile.in
>@@ -48,11 +48,12 @@ EXTRA_JS_MODULES = \
>   dbViewWrapper.js \
>   mailViewManager.js \
>   quickSearchManager.js \
>   searchSpec.js \
[snip…]
>   sessionStoreManager.js \
>+  MailMigrator.js \

Why MailMigrator.js instead of mailMigrator.js (a la mailViewManager.js)?

But really, all of these are fairly minor, so r=me, as long as this patch
is just an un-bit-rotted version of the previous one.  ;)

Thanks,
Blake.
Attachment #436561 - Flags: review?(bwinton) → review+
(In reply to comment #60)
> >+pref("mail.font.windows.version", 0);
> 
> Shouldn't this be in some sort of Windows-only block?

I considered doing that, but I think this makes sense in case we have such a situation on a different platform in the future (and we need to switch back and forth.)

> 
> >+++ b/mail/base/content/msgMail3PaneWindow.js
> >@@ -43,17 +43,17 @@
> >+Components.utils.import("resource://app/modules/MailnewsMigrator.js");
> >@@ -311,16 +311,18 @@ function LoadPostAccountWizard()
> >+  Components.utils.import("resource://app/modules/MailMigrator.js");
> 
> Didn't you already do that up on line 43?

One is MailnewsMigrator, the other is MailMigrator.

> 
> >+  MailMigrator.migrateMail();
> 
> Also, it's not really Mail you're migrating.  Can you think of a better
> name for this, like migratePrefs, or something?

The older version of the patch did have migratePrefs, but I switched it to migrateMail for two reasons:
1. we could in theory have non-pref migration at some point.
2. it's in line with migrateMailnews(), which is already in the tree.

> 
> Congratulations!  I think this is the first code I've reviewed that's
> gotten these right first time.  ;)

My previous patch got them right too! ;)

> >+   */
> >+  _switchFonts: function MailMigrator__switchFonts(aFonts, aEncodings) {
> 
> What about naming it "_switchDefaultFonts"?

Sounds good.

> So, we only migrate to ClearType fonts if we're on Vista and above, but
> we're also migrating Mac and Linux.  Shouldn't we migrate older Windows
> versions to something non-ClearType-but-still-nicer?

I'm not aware of our options there, and in any case that'll require additional UI review. I'd prefer to land this for now and deal with that if necessary in a followup.

> Why MailMigrator.js instead of mailMigrator.js (a la mailViewManager.js)?

We've been somewhat inconsistent with naming here -- some modules have the first letter a capital and some don't. I wouldn't object to changing it if you really want me to, but I prefer the first letter being capital because it matches up with the exported object.
Per discussion on IRC, I've changed the module names to lowercase.

http://hg.mozilla.org/comm-central/rev/f98d46450428
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [has patch for review]
Target Milestone: --- → Thunderbird 3.1b2
Depends on: 556651
Pushed a minor followup related to MozMill: http://hg.mozilla.org/comm-central/rev/70064eeec705
Depends on: 558607
No longer depends on: 558607
You need to log in before you can comment on or make changes to this bug.