Enables font-related unittests again.

Enables font-related unittests again.

Enables the unittests which got disabled because Symbol font was missing on trybots.

See http://crrev.com/67523002

Symbol font has been installed on trybots.  See http://crbug.com/320391

BUG=316955
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=240048

[Yuki, 2013-11-21 07:59:12]

Could you review this CL?

[msw, 2013-11-21 17:07:36]

LGTM, but please add ASSERTs to these tests to ensure that the symbol font was
indeed loaded; bonus points if you add a test for GetFontName behavior with bad
font names, if such a test does not exist.

https://codereview.chromium.org/79263004/diff/1/ui/gfx/font_list_unittest.cc
File ui/gfx/font_list_unittest.cc (right):

https://codereview.chromium.org/79263004/diff/1/ui/gfx/font_list_unittest.cc#...
ui/gfx/font_list_unittest.cc:301: Font font2("Symbol", 16);
ASSERT_EQ("Symbol", font2.GetFontName()) or similar for all these tests.
Please make sure that has the intended effect too; Font::GetFontName should
return the actual font resolved, not the font name supplied to the constructor
if such a font was not found. Ensure that instantiating a
Font("TotallyFakeFontName", 16) doesn't return that garbage string from
GetFontName(). We should probably have a test for that somewhere too...

[sadrul, 2013-11-21 19:31:05]

LGTM

[Yuki, 2013-12-06 11:37:40]

Sorry for leaving this CL so long.
I added some more tests as Mike advised.

Mike, could you take another look at this CL?

[msw, 2013-12-09 19:27:14]

GetFontName is not used in that many places, I think it should simply adopt this
new behavior of providing the actual font name instead of the specified string
wherever possible. If you cannot do that, this new function should be called
GetActualFontNameForTest, since it's only called in unit tests, I guess...

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font.h#newcode102
ui/gfx/font.h:102: // Returns the actually-used font name in UTF-8.
nit: Hyphen is unnecessary.

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font_unittest.cc
File ui/gfx/font_unittest.cc (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font_unittest.cc#...
ui/gfx/font_unittest.cc:130: // On Windows, Font::GetActualFontName() doesn't
work well for now.
nit: file an issue to follow up.

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font_unittest.cc#...
ui/gfx/font_unittest.cc:137: const char* const invalid_font_name =
"no_such_a_font_name";
nit: consider "invalid_font_name" or "no_such_font_name"

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font.h#n...
ui/gfx/platform_font.h:66: // Returns the actually-used font name in UTF-8.
nit: ditto hyphen not necessary.

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:172: // the font name taken from HFONT handle, but
it's not the name comes from
nit: "from the HFONT", "name that comes", "metadata", file an issue to follow up
and cite it here.

[Yuki, 2013-12-10 09:58:58]

For GetFontName vs GetActualFontName(ForTest), I think it's better to keep the
current spec of GetFontName().  See the following example.

  Font f1("ABC", 16);
  Font f2(f1.GetFontName(), 20);

In this case, we want |f2| to be "ABC 20px" font or its fallback, not "XYZ 20px"
or its fallback even if "ABC 16px" fallbacked to "XYZ 16px".  Bold and Italic
matter, too.

I'm afraid that changing the semantics of GetFontName() would cause issues.

So let me make a new method GetActuallFontNameForTest().

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font.h
File ui/gfx/font.h (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font.h#newcode102
ui/gfx/font.h:102: // Returns the actually-used font name in UTF-8.
On 2013/12/09 19:27:15, msw wrote:
> nit: Hyphen is unnecessary.

Done.

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font_unittest.cc
File ui/gfx/font_unittest.cc (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font_unittest.cc#...
ui/gfx/font_unittest.cc:130: // On Windows, Font::GetActualFontName() doesn't
work well for now.
On 2013/12/09 19:27:15, msw wrote:
> nit: file an issue to follow up.

Done.

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/font_unittest.cc#...
ui/gfx/font_unittest.cc:137: const char* const invalid_font_name =
"no_such_a_font_name";
On 2013/12/09 19:27:15, msw wrote:
> nit: consider "invalid_font_name" or "no_such_font_name"

Done.

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font.h#n...
ui/gfx/platform_font.h:66: // Returns the actually-used font name in UTF-8.
On 2013/12/09 19:27:15, msw wrote:
> nit: ditto hyphen not necessary.

Done.

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font_win.cc
File ui/gfx/platform_font_win.cc (right):

https://codereview.chromium.org/79263004/diff/240001/ui/gfx/platform_font_win...
ui/gfx/platform_font_win.cc:172: // the font name taken from HFONT handle, but
it's not the name comes from
On 2013/12/09 19:27:15, msw wrote:
> nit: "from the HFONT", "name that comes", "metadata", file an issue to follow
up
> and cite it here.

Done.

[msw, 2013-12-10 16:22:12]

LGTM, but I'd like to have Alexei review too.

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/font_unittest.cc
File ui/gfx/font_unittest.cc (right):

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/font_unittest.cc#...
ui/gfx/font_unittest.cc:130: // On Windows, Font::GetActualFontName() doesn't
work well for now.
nit: GetActualFontNameForTest

[Alexei Svitkine (slow), 2013-12-10 16:30:30]

LGTM % nit

Thanks for cleaning this up!

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/platform_font.h#n...
ui/gfx/platform_font.h:67: virtual std::string GetActualFontNameForTest() const
= 0;
Nit: I think the convention for the suffix is "ForTesting". Please fix
throughout.

[Yuki, 2013-12-11 05:05:57]

Thanks for the review.  Committing.

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/font_unittest.cc
File ui/gfx/font_unittest.cc (right):

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/font_unittest.cc#...
ui/gfx/font_unittest.cc:130: // On Windows, Font::GetActualFontName() doesn't
work well for now.
On 2013/12/10 16:22:12, msw wrote:
> nit: GetActualFontNameForTest

Done.

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/platform_font.h
File ui/gfx/platform_font.h (right):

https://codereview.chromium.org/79263004/diff/280001/ui/gfx/platform_font.h#n...
ui/gfx/platform_font.h:67: virtual std::string GetActualFontNameForTest() const
= 0;
On 2013/12/10 16:30:30, Alexei Svitkine wrote:
> Nit: I think the convention for the suffix is "ForTesting". Please fix
> throughout.

Done.

[commit-bot: I haz the power, 2013-12-11 05:07:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/yukishiino@chromium.org/79263004/320001

[commit-bot: I haz the power, 2013-12-11 08:43:08]

Change committed as 240048