Rework gfx::Font by moving platform-specific code into inner classes....

Rework gfx::Font by moving platform-specific code into inner classes.

gfx::Font is a platform-neutral API shim that exists as a wrapper object to allow for the creation and lifetime of gfx::Font objects to remain consistent with past usage.

gfx::PlatformFont is an interface implemented by the platform-specific inner classes (gfx::PlatformFontWin,Mac,Gtk).

BUG=none
TEST=existing unittests


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

[Ben Goodger (Google), 2010-08-06 20:35:27]

Review tip:

Review the gfx/ bits first, the rest is just changing to the new API.

The API changes can best be described as:

- most methods now get StudlyCaps names since they are now no longer pure
accessors.
- font creation is defined by a few standard constructors instead of a series of
common and platform specific CreateFont methods.

[sky, 2010-08-06 23:23:42]

I had one general comment before I continue the review. Let me know what you
think.

http://codereview.chromium.org/3083022/diff/46001/21052
File gfx/platform_font.h (right):

http://codereview.chromium.org/3083022/diff/46001/21052#newcode19
gfx/platform_font.h:19: // Create an appropriate PlatformFont implementation.
Need a virtual destructor.

http://codereview.chromium.org/3083022/diff/46001/21057
File gfx/platform_font_win.cc (right):

http://codereview.chromium.org/3083022/diff/46001/21057#newcode85
gfx/platform_font_win.cc:85: owner_ = owner;
Should this DCHECK that owner_ == NULL?

http://codereview.chromium.org/3083022/diff/46001/21058
File gfx/platform_font_win.h (right):

http://codereview.chromium.org/3083022/diff/46001/21058#newcode14
gfx/platform_font_win.h:14: class PlatformFontWin : public PlatformFont {
Did you consider making PlatformFont ref counted so that coping is really cheap?
Font is meant to be immutable, so it should work. The only thing preventing that
now is the use of owner_ in SizeStringInt, which could be replaced by
Font(this).

http://codereview.chromium.org/3083022/diff/46001/21058#newcode17
gfx/platform_font_win.h:17: PlatformFontWin(const Font& other);
explicit

[Ben Goodger (Google), 2010-08-09 18:27:19]

OK, I updated with the refcounting changes.

[sky, 2010-08-09 20:35:10]

http://codereview.chromium.org/3083022/diff/20006/32012
File chrome/browser/autocomplete/autocomplete_popup_view_mac.mm (right):

http://codereview.chromium.org/3083022/diff/20006/32012#newcode409
chrome/browser/autocomplete/autocomplete_popup_view_mac.mm:409:
(int)resultFontSize);
static_cast

http://codereview.chromium.org/3083022/diff/20006/32017
File chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm (right):

http://codereview.chromium.org/3083022/diff/20006/32017#newcode32
chrome/browser/cocoa/bookmark_menu_cocoa_controller.mm:32: (int)[nsfont
pointSize]);
static_cast

http://codereview.chromium.org/3083022/diff/20006/32035
File chrome/browser/views/options/fonts_page_view.cc (right):

http://codereview.chromium.org/3083022/diff/20006/32035#newcode210
chrome/browser/views/options/fonts_page_view.cc:210: gfx::Font font(const_font);
This should be nuked and const_font used.

http://codereview.chromium.org/3083022/diff/20006/32035#newcode211
chrome/browser/views/options/fonts_page_view.cc:211: if
(gfx::Font(font).GetFontName().empty())
gfx::Font(font).GetFontName().empty() -> const_font.getFontName().empty()

http://codereview.chromium.org/3083022/diff/20006/32035#newcode213
chrome/browser/views/options/fonts_page_view.cc:213: int font_size =
gfx::Font(font).GetFontSize();
const_font.GetFontSize

http://codereview.chromium.org/3083022/diff/20006/32035#newcode220
chrome/browser/views/options/fonts_page_view.cc:220: font.GetFontName(),
const_font.GetFontName()

http://codereview.chromium.org/3083022/diff/20006/32035#newcode228
chrome/browser/views/options/fonts_page_view.cc:228: font.GetFontName(),
const_font

http://codereview.chromium.org/3083022/diff/20006/32035#newcode235
chrome/browser/views/options/fonts_page_view.cc:235:
fixed_width_font_display_view_->SetFontType(font.GetFontName(), font_size);
const_font

http://codereview.chromium.org/3083022/diff/20006/32043
File gfx/canvas_skia_linux.cc (right):

http://codereview.chromium.org/3083022/diff/20006/32043#newcode16
gfx/canvas_skia_linux.cc:16: #include "gfx/platform_font_gtk.h"
sort

http://codereview.chromium.org/3083022/diff/20006/32065
File views/controls/button/native_button.cc (right):

http://codereview.chromium.org/3083022/diff/20006/32065#newcode147
views/controls/button/native_button.cc:147: /*
Is this intentional?

[sky, 2010-08-09 21:02:41]

LGTM

http://codereview.chromium.org/3083022/diff/78002/85032
File chrome/browser/views/options/fonts_page_view.cc (right):

http://codereview.chromium.org/3083022/diff/78002/85032#newcode212
chrome/browser/views/options/fonts_page_view.cc:212: int font_size =
gfx::Font(font).GetFontSize();
font.GetFontSize