Respect the system ClearType setting in RenderTextWin.

Respect the system ClearType setting in RenderTextWin.

BUG=113184, 105550
TEST=Build Chrome Windows with gyp flag use_canvas_skia_skia=1. Open Chrome and observe text rendering in the tab strip, bookmarks bar, etc. Change Windows ClearType settings while Chrome is running. Text rendering should be updated appropriately.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=122514

[Alexei Svitkine (slow), 2012-02-09 17:44:21]

Note: This depends on http://codereview.chromium.org/9359019/ and also is
missing the ui.gypi changes to add the new files - I'll add those later.

[sky, 2012-02-09 19:33:01]

Don't you need to update some gyp files?

[Alexei Svitkine (slow), 2012-02-13 23:15:06]

On 2012/02/09 19:33:01, sky wrote:
> Don't you need to update some gyp files?

I've now added the gyp files along with singleton_hwnd.cc from
http://codereview.chromium.org/9359019/ (which I've closed).

(I'll be removing ui/gfx/screen_compatible_dc_win.cc in a separate CL to move
the cached HDC to RenderTextWin directly.)

[Alexei Svitkine (slow), 2012-02-13 23:15:45]

+cpu

[tfarina, 2012-02-13 23:19:48]

http://codereview.chromium.org/9370026/diff/4001/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9370026/diff/4001/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:14: #include "base/memory/singleton.h"
nit: we usually forward declare this.

template <typename T> struct DefaultSingletonTraits;

and include the singleton.h in the source file instead.

http://codereview.chromium.org/9370026/diff/4001/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:43: friend struct
DefaultSingletonTraits<SingletonHwnd>;
nit: I think we usually put this right after the private: declaration.

[Alexei Svitkine (slow), 2012-02-14 15:27:05]

http://codereview.chromium.org/9370026/diff/4001/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9370026/diff/4001/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:14: #include "base/memory/singleton.h"
On 2012/02/13 23:19:48, tfarina wrote:
> nit: we usually forward declare this.
> 
> template <typename T> struct DefaultSingletonTraits;
> 
> and include the singleton.h in the source file instead.

Done.

http://codereview.chromium.org/9370026/diff/4001/ui/base/win/singleton_hwnd.h...
ui/base/win/singleton_hwnd.h:43: friend struct
DefaultSingletonTraits<SingletonHwnd>;
On 2012/02/13 23:19:48, tfarina wrote:
> nit: I think we usually put this right after the private: declaration.

Done.

[cpu_(ooo_6.6-7.5), 2012-02-14 20:53:30]

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd.cc
File ui/base/win/singleton_hwnd.cc (right):

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:41: HWND_MESSAGE, 0, hinst, 0);
same comment: I don't think message windows get the broadcast messages.

[asvitkine_google, 2012-02-14 20:58:00]

On 2012/02/14 20:53:30, cpu wrote:
>
http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd.cc
> File ui/base/win/singleton_hwnd.cc (right):
> 
>
http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
> ui/base/win/singleton_hwnd.cc:41: HWND_MESSAGE, 0, hinst, 0);
> same comment: I don't think message windows get the broadcast messages.

I have verified that WM_SETTINGCHANGE is indeed sent. It depends on the message.

[msw, 2012-02-15 00:33:16]

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:22: // notifications.
Note that not all WM_* messages are supported; point to doc?

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:23: class SingletonHwnd {
Why not use/extend existing code like MessagePumpObserver?

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:25: // Singleton getter.
nit: remove obvious comment.

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:28: // Registers a listener callback for WM_*
notifications.
Did you consider using [Add|Remove]Observer terminology?

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:29: void RegisterListener(const WndProcCallback&
callback);
Is there no way to UnregisterListener?

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.cc
File ui/gfx/font_smoothing_win.cc (right):

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:71: // Returns the cached screen compatible DC or
creates one if needed.
This comment just seems incorrect.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:81: listener_registered_ = true;
Bad advice?: I don't expect re-entrance here, but you can help win a race by
putting this at the top of the block.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:91: BOOL enabled;
You should explicitly init this local.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:92: if (SystemParametersInfo(SPI_GETFONTSMOOTHING,
0, &enabled, 0) && enabled) {
Should we explicitly check for the BOOL to != 0?

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:95: UINT smooth_type;
You should explicitly init this local.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:108: smoothing_enabled, cleartype_enabled);
These params should each be on their own lines.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.h
File ui/gfx/font_smoothing_win.h (right):

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.h...
ui/gfx/font_smoothing_win.h:11: // Returns the Windows system font smoothing
settings: whether font smoothing
nit: verbosity, how about: "Returns the Windows system font smoothing  and
ClearType settings."

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:446:
renderer.SetFontSmoothingSettings(smoothing_enabled, cleartype_enabled);
Perhaps comment on how enable_lcd_text == cleartype_enabled?
I guess if you're familiar with ClearType you'll understand.

[asvitkine_google, 2012-02-15 15:10:44]

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:22: // notifications.
On 2012/02/15 00:33:16, msw wrote:
> Note that not all WM_* messages are supported; point to doc?

Done.

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:23: class SingletonHwnd {
On 2012/02/15 00:33:16, msw wrote:
> Why not use/extend existing code like MessagePumpObserver?

I'm not familiar with that class... from a quick glance at it, it's not really
clear to me how it can help here. Can you elaborate on your suggestion?

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:25: // Singleton getter.
On 2012/02/15 00:33:16, msw wrote:
> nit: remove obvious comment.

Done.

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:28: // Registers a listener callback for WM_*
notifications.
On 2012/02/15 00:33:16, msw wrote:
> Did you consider using [Add|Remove]Observer terminology?

You're right - I think base/observer_list.h is the right thing to use here.
Thanks.

I've updated the code to use it.

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:29: void RegisterListener(const WndProcCallback&
callback);
On 2012/02/15 00:33:16, msw wrote:
> Is there no way to UnregisterListener?

Added.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.cc
File ui/gfx/font_smoothing_win.cc (right):

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:71: // Returns the cached screen compatible DC or
creates one if needed.
On 2012/02/15 00:33:16, msw wrote:
> This comment just seems incorrect.

Oops. Fixed.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:81: listener_registered_ = true;
On 2012/02/15 00:33:16, msw wrote:
> Bad advice?: I don't expect re-entrance here, but you can help win a race by
> putting this at the top of the block.

If folks are calling this on different threads, there's bigger problems than
this race. But since this is in ui, the assumption is that this will only be
called on the UI thread.

(I would add a DCHECK() for the thread affinity, but knowledge of browser
threads is in content/, which ui/ can't depend on.)

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:91: BOOL enabled;
On 2012/02/15 00:33:16, msw wrote:
> You should explicitly init this local.

Done.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:92: if (SystemParametersInfo(SPI_GETFONTSMOOTHING,
0, &enabled, 0) && enabled) {
On 2012/02/15 00:33:16, msw wrote:
> Should we explicitly check for the BOOL to != 0?

If we treat it like 'bool', we shouldn't..

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:95: UINT smooth_type;
On 2012/02/15 00:33:16, msw wrote:
> You should explicitly init this local.

Done.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:108: smoothing_enabled, cleartype_enabled);
On 2012/02/15 00:33:16, msw wrote:
> These params should each be on their own lines.

Done.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.h
File ui/gfx/font_smoothing_win.h (right):

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/font_smoothing_win.h...
ui/gfx/font_smoothing_win.h:11: // Returns the Windows system font smoothing
settings: whether font smoothing
On 2012/02/15 00:33:16, msw wrote:
> nit: verbosity, how about: "Returns the Windows system font smoothing  and
> ClearType settings."

Done.

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/render_text_win.cc
File ui/gfx/render_text_win.cc (right):

http://codereview.chromium.org/9370026/diff/11001/ui/gfx/render_text_win.cc#n...
ui/gfx/render_text_win.cc:446:
renderer.SetFontSmoothingSettings(smoothing_enabled, cleartype_enabled);
On 2012/02/15 00:33:16, msw wrote:
> Perhaps comment on how enable_lcd_text == cleartype_enabled?
> I guess if you're familiar with ClearType you'll understand.

Done.

[msw, 2012-02-15 19:23:28]

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9370026/diff/11001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:23: class SingletonHwnd {
On 2012/02/15 15:10:44, asvitkine wrote:
> On 2012/02/15 00:33:16, msw wrote:
> > Why not use/extend existing code like MessagePumpObserver?
> 
> I'm not familiar with that class... from a quick glance at it, it's not really
> clear to me how it can help here. Can you elaborate on your suggestion?

You're creating an HWND to listen for arbitrary WM_* MSGs.
MessagePumpForUI in base/message_pump_win.cc does the same thing.
MessagePumpObservers are called for each WM_* MSG sent to that HWND.
Make CachedFontSmoothingSettings a MessagePumpObserver and nix this class.

http://codereview.chromium.org/9370026/diff/18001/ui/base/win/singleton_hwnd.cc
File ui/base/win/singleton_hwnd.cc (right):

http://codereview.chromium.org/9370026/diff/18001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:21: LRESULT CALLBACK ListenerWindowProc(HWND hwnd,
This is similar to MessagePumpForUI::ProcessMessageHelper

http://codereview.chromium.org/9370026/diff/18001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:30: HWND CreateListenerWindow() {
This is just like MessagePumpForUI::InitMessageWnd

http://codereview.chromium.org/9370026/diff/18001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:53: void SingletonHwnd::AddObserver(Observer*
observer) {
These are like MessagePumpWin::AddObserver and RemoveObserver.

http://codereview.chromium.org/9370026/diff/18001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.cc:65: void SingletonHwnd::OnWndProc(HWND hwnd,
This is like MessagePumpWin::WillProcessMessage.

http://codereview.chromium.org/9370026/diff/18001/ui/base/win/singleton_hwnd.h
File ui/base/win/singleton_hwnd.h (right):

http://codereview.chromium.org/9370026/diff/18001/ui/base/win/singleton_hwnd....
ui/base/win/singleton_hwnd.h:33: virtual void OnWndProc(HWND hwnd,
Consider passing a MSG struct; although that might incur overhead.

http://codereview.chromium.org/9370026/diff/18001/ui/gfx/font_smoothing_win.cc
File ui/gfx/font_smoothing_win.cc (right):

http://codereview.chromium.org/9370026/diff/18001/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:16: class CachedFontSmoothingSettings : public
ui::SingletonHwnd::Observer {
See my reply about MessagePumpObserver.
Make this class a MessagePumpObserver and nix SingletonHwnd.

[Alexei Svitkine (slow), 2012-02-15 21:35:46]

On Wed, Feb 15, 2012 at 3:47 PM, Alexei Svitkine <asvitkine@chromium.org> wrote:
>> I have verified that WM_SETTINGCHANGE is indeed sent. It depends on the
>> message.
>
> Hmm. I am not sure what I was seeing before (I thought I specifically
> verified this both in the code and seeing the new behaviour when
> toggling this), but I am no longer able to reproduce this. :(
>
> It seems I can't trigger WM_SETTINGCHANGE to be sent by toggling
> ClearType settings in Windows.
>
> Neither through my SingletonHwnd nor by observing MessageLoopForUI per
> Mike's suggestion...

I'm also unable to even trigger NativeWidgetWin's OnSettingChange()
code (which is also listening to WM_SETTINGCHANGE). Very strange!

[msw, 2012-02-15 22:32:44]

> > It seems I can't trigger WM_SETTINGCHANGE to be sent by toggling
> > ClearType settings in Windows.
Is it possible that the HWND we're creating isn't top level? From MSDN:
"A message that is sent to all top-level windows..."
Do you get other MSGs or is WrappedWindowProc broken?

[Alexei Svitkine (slow), 2012-02-15 23:05:26]

On Wed, Feb 15, 2012 at 4:35 PM, Alexei Svitkine <asvitkine@chromium.org>wrote:

> On Wed, Feb 15, 2012 at 3:47 PM, Alexei Svitkine <asvitkine@chromium.org>
> wrote:
> >> I have verified that WM_SETTINGCHANGE is indeed sent. It depends on the
> >> message.
> >
> > Hmm. I am not sure what I was seeing before (I thought I specifically
> > verified this both in the code and seeing the new behaviour when
> > toggling this), but I am no longer able to reproduce this. :(
> >
> > It seems I can't trigger WM_SETTINGCHANGE to be sent by toggling
> > ClearType settings in Windows.
> >
> > Neither through my SingletonHwnd nor by observing MessageLoopForUI per
> > Mike's suggestion...
>
> I'm also unable to even trigger NativeWidgetWin's OnSettingChange()
> code (which is also listening to WM_SETTINGCHANGE). Very strange!
>

I was able to trigger NativeWidgetWin's OnSettingChange() listener by
resizing the task bar, but not by changing the system
clear ClearType setting. It seems that perhaps changing the system
ClearType setting doesn't trigger a WM_SETTINGCHANGE? Although this would
be contrary to SystemParametersInfo() docs... It seems changing the
ClearType setting only triggers paints.

I'll do some benchmarking tomorrow to see how slow SystemParametersInfo()
really is - perhaps it's fast enough that querying these each time would
not be noticeable. Otherwise, we could listen for paint messages and query
it on those...

[Alexei Svitkine (slow), 2012-02-16 16:31:40]

I've updated the CL to use MessagePumpObserver and to invalidate the cached
settings on WM_PAINT messages.

[msw, 2012-02-16 18:09:37]

What's the point of caching the values if they're invalidated on every paint?
It seems worth investigating why you aren't getting WM_SETTINGCHANGE.
I just did a quick test with Spy++ on Chrome while toggling ClearType and saw: 
<00001> 00070256 S WM_SETTINGCHANGE wFlag:SPI_SETFONTSMOOTHINGORIENTATION
pszMetrics:0026DB58
<00002> 00070256 R WM_SETTINGCHANGE
<00003> 00070256 S WM_SETTINGCHANGE wFlag:SPI_SETFONTSMOOTHINGORIENTATION
pszMetrics:0026DB58
<00004> 00070256 R WM_SETTINGCHANGE

[asvitkine_google, 2012-02-16 18:14:09]

On 2012/02/16 18:09:37, msw wrote:
> What's the point of caching the values if they're invalidated on every paint?

Every hwnd paint != every RenderTextWin paint.

> It seems worth investigating why you aren't getting WM_SETTINGCHANGE.
> I just did a quick test with Spy++ on Chrome while toggling ClearType and saw:

> <00001> 00070256 S WM_SETTINGCHANGE wFlag:SPI_SETFONTSMOOTHINGORIENTATION
> pszMetrics:0026DB58
> <00002> 00070256 R WM_SETTINGCHANGE
> <00003> 00070256 S WM_SETTINGCHANGE wFlag:SPI_SETFONTSMOOTHINGORIENTATION
> pszMetrics:0026DB58
> <00004> 00070256 R WM_SETTINGCHANGE

Hmm, I will have to dig deeper into this then. It seems consistent with what I
was seeing before too...

[Alexei Svitkine (slow), 2012-02-16 20:00:05]

After more digging with Mike's help, I found my earlier cause of confusion.

It turns out that the Windows ClearType settings UI doesn't actually send the
WM_SETTINGCHANGE notification until you complete the wizard and hit Finish. Just
toggling the checkbox does not cause that notification to be sent, but does
cause WM_PAINT messages to be sent and updated values to be returned from the
SystemParametersInfo() API.

So I've changed it back to listen to WM_SETTINGCHANGE notifications.

The other thing we found was that MessagePumpForUI was not quite the same as the
singleton_hwnd.cc in my CL. The HWND created there only look for kMsgHaveWork
and WM_TIMER and the observer interface it provides is unrelated to that HWND,
so it's not appropriate for this use.

So I've re-added singleton_hwnd.cc back to the CL.

I also realised that we have some other similar code in
src/content/browser/system_message_win.cc that can be simply made an observer of
singleton_hwnd.cc after this CL.

[msw, 2012-02-16 20:54:53]

I should have mentioned this earlier, but I'm generally dubious that we're
gaining much with this approach. I presume we actually want to support switching
these while chrome is running (so just checking once isn't an option). Still, is
handling every WM_* and using one such MSG to invalidate really better than
checking on every paint, or on every Nth paint as a sort of lazy sampling
method? I won't speculate too much on performance, but I'd like to know what
numbers you're seeing, and it'd be great to know what Webkit and other Skia
clients do.

http://codereview.chromium.org/9370026/diff/30004/ui/gfx/font_smoothing_win.cc
File ui/gfx/font_smoothing_win.cc (right):

http://codereview.chromium.org/9370026/diff/30004/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:8: #include "base/message_loop.h"
Is this still needed?

[asvitkine_google, 2012-02-16 21:15:05]

On 2012/02/16 20:54:53, msw wrote:
> I should have mentioned this earlier, but I'm generally dubious that we're
> gaining much with this approach. I presume we actually want to support
switching
> these while chrome is running (so just checking once isn't an option). Still,
is
> handling every WM_* and using one such MSG to invalidate really better than
> checking on every paint, or on every Nth paint as a sort of lazy sampling
> method?

I guess the idea is that this singleton that's offscreen shouldn't be receiving
many messages. Particularly, it won't be getting paint messages, since its not
visible.

On the other hand, I think the other approach of observing the message pump
_was_ indeed receiving all WM_* messages sent to the all the hwnds in the app,
including paint messages and such.

Furthermore, if content/browser/system_message_win.cc will be made to hook into
this singleton instead of listening on its own HWND (which I plan to do), then
the additional overhead of this is even less.

The real expensive operation is querying these settings from the system, not
receiving messages...

>  I won't speculate too much on performance, but I'd like to know what
> numbers you're seeing, and it'd be great to know what Webkit and other Skia
> clients do.

When I made RenderTextWin query these on every Draw() (i.e. no caching), the
Skia drawText() was accounting for 15% of the time spent in
RenderTextWin::Draw() and querying the font smoothing values accounted for 85%
percent of that time. (This was not counting the Layout() time...)

As for what Skia and WebKit do, Skia just expects the user of the API to specify
the values, as we're doing with this CL.

The WebKit code that uses Skia just queries it once and caches those values
forever. But it's per renderer process, which come and go and so would
eventually pick up the new settings... whereas the browser process lifetime is
obviously longer. More importantly, if we didn't update these, the
canvas_skia_skia.cc path would be regressing w.r.t. canvas_skia_win.cc behavior.

[msw, 2012-02-16 22:31:23]

Okay, thanks for the explanation. LGTM with my two comments.

http://codereview.chromium.org/9370026/diff/30004/ui/gfx/font_smoothing_win.cc
File ui/gfx/font_smoothing_win.cc (right):

http://codereview.chromium.org/9370026/diff/30004/ui/gfx/font_smoothing_win.c...
ui/gfx/font_smoothing_win.cc:84:
CachedFontSmoothingSettings::~CachedFontSmoothingSettings() {
Should this remove itself from the SingletonHwnd observer list?

[sky, 2012-02-16 22:49:57]

Rubber stamp LGTM

[asvitkine_google, 2012-02-16 22:57:42]

>
http://codereview.chromium.org/9370026/diff/30004/ui/gfx/font_smoothing_win.c...
> ui/gfx/font_smoothing_win.cc:84:
> CachedFontSmoothingSettings::~CachedFontSmoothingSettings() {
> Should this remove itself from the SingletonHwnd observer list?

They're both singletons and we don't know which will be destroyed
first. So we can't do this, as SingletonHwnd may have already been
destroyed.

I guess the hypothetical problem is what happens if
CachedFontSmoothingSettings gets destroyed but SingletonHwnd is still
alive and we get a message. I don't know if that's possible (e.g. this
late during shutdown, do we still have a messageloop pumping UI thread
events)?

If it is, this may need to use WeakPtr's to prevent this case...

>
> http://codereview.chromium.org/9370026/

[commit-bot: I haz the power, 2012-02-17 14:39:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/asvitkine@chromium.org/9370026/30009

[commit-bot: I haz the power, 2012-02-17 15:53:04]

Change committed as 122514