Makes the default ctor of gfx::FontList copy the pre-calculated metrics.

Makes the default ctor of gfx::FontList copy the pre-calculated metrics.

Every instance of default-constructed FontList has been calling CacheCommonFontHeightAndBaseline() and CacheFontStyleAndSize(), and it's inefficient.  This CL creates an instance of FontList which holds pre-calculated metrics, and copy the pre-calculated metrics in the default ctor.  This change improves the performance very much.

BUG=330975
TEST=Run unit_tests, views_unittests, ui_unittests.

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

[Yuki, 2014-01-07 09:59:53]

Could you guys review this CL?

tony@ as the owner of /ui/base/resource/resource_bundle.cc

asvitkine@ and msw@ as the main reviewers

[tony, 2014-01-07 17:30:00]

ui/base/resource LGTM

[msw, 2014-01-07 17:51:38]

LGTM with nits.

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:88: // Copy the default font list specification as well as
the pre-calculated
nit: s/as well as the/and its/

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:147: static FontList default_font_list((Font()));
nit: remove extra parens around Font().

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:150: default_font_list = font_description.empty() ?
nit: make this if (!font_description.empty()) default_font_list =
FontList(font_description);

[Alexei Svitkine (slow), 2014-01-07 21:56:23]

lgtm

[Yuki, 2014-01-08 06:32:18]

Thanks for the reviews.
If you have more comments, let me address them in a follow-up CL.  I'll submit
this CL.

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:88: // Copy the default font list specification as well as
the pre-calculated
On 2014/01/07 17:51:38, msw wrote:
> nit: s/as well as the/and its/

Done.

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:147: static FontList default_font_list((Font()));
On 2014/01/07 17:51:38, msw wrote:
> nit: remove extra parens around Font().

This is necessary.  Without them, it's ambiguous if it's a function declaration
or a variable declaration.  default_font_list may look like a function which
returns a FontList and takes a Font.

https://codereview.chromium.org/124693003/diff/20001/ui/gfx/font_list.cc#newc...
ui/gfx/font_list.cc:150: default_font_list = font_description.empty() ?
On 2014/01/07 17:51:38, msw wrote:
> nit: make this if (!font_description.empty()) default_font_list =
> FontList(font_description);

When ui::ResourceBundle reloads the resources including fonts,
SetDefaultFontDescription() was called twice and more.  So we have to reset
|default_font_list| according to the given |font_description|.  If
font_description.empty(), we should reset |default_font_list| with a font list
which has a default Font.

[commit-bot: I haz the power, 2014-01-08 06:33:11]

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

[commit-bot: I haz the power, 2014-01-08 06:47:27]

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

[commit-bot: I haz the power, 2014-01-08 08:29:22]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, content_browsertests, interactive_ui_tests, nacl_integration,
net_unittests, sync_integration_tests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-08 09:00:10]

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

[commit-bot: I haz the power, 2014-01-08 09:46:18]

Retried try job too often on linux_rel for step(s) sync_integration_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-08 12:28:05]

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

[commit-bot: I haz the power, 2014-01-08 13:33:07]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-08 15:53:13]

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

[commit-bot: I haz the power, 2014-01-08 16:55:37]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-09 02:59:04]

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

[commit-bot: I haz the power, 2014-01-09 03:42:48]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-09 05:19:08]

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

[commit-bot: I haz the power, 2014-01-09 06:23:00]

Retried try job too often on linux_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-09 15:18:53]

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

[commit-bot: I haz the power, 2014-01-09 15:53:46]

Retried try job too often on linux_rel for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[Yuki, 2014-01-09 18:02:12]

I found that some of unittests don't expect that
FontList::SetDefaultFontDescription() creates a new Font.  Also Linux on GTK
doesn't expect it.

So I moved the creation of the default FontList into the default ctor of
FontList.

If you guys find any issues, please let me know.  I'll address them.  Otherwise,
I'll commit this change tomorrow.

[Alexei Svitkine (slow), 2014-01-09 20:58:12]

On 2014/01/09 18:02:12, Yuki wrote:
> I found that some of unittests don't expect that
> FontList::SetDefaultFontDescription() creates a new Font.  Also Linux on GTK
> doesn't expect it.

Can you elaborate on what happens in those cases? I'm not sure what "don't
expect" means and it seems surprising to me that it would cause problems.

https://codereview.chromium.org/124693003/diff/1430001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/124693003/diff/1430001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:101: if (default_font_list.GetPrimaryFont().platform_font())
{
Can you add a comment explaining this if statement?

[Yuki, 2014-01-10 04:57:09]

I found the following two cases that matter.  There could be more.

On Linux GTK, PlatformFontPango::GetDefaultFont() depends on GTK+ APIs, so GTK+
must have been initialized in advance.  However, GTK+ seems not yet initialized
at the time that SetDefaultFontDescription() is called.  We cannot get
"gtk-font-name" at the time, so we cannot create the default Font.

Some of unittests don't have a resource pack, so they cannot get a proper
default font description.  It's okay as long as they don't use Font.  However,
some of unittests depends on ui::ResourceBundle (though a resource pack is not
built), so SetDefaultFontDescription() is called from ui::ResourceBundle without
a resource pack despite that they actually don't use Font.
On CrOS, the empty default font description string makes PlatformFontPango
crash.  Unlike other OSs, CrOS doesn't have a way to get the OS's default font. 
CrOS assumes that a non-empty default font description is always supplied.

In summary from my point of view, SetDefaultFontDescription() was designed just
to specify a font description string, not to create an instance of Font or
FontList.  It's too early to create a Font when SetDefaultFontDescription() is
called.

https://codereview.chromium.org/124693003/diff/1430001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/124693003/diff/1430001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:101: if (default_font_list.GetPrimaryFont().platform_font())
{
On 2014/01/09 20:58:13, Alexei Svitkine wrote:
> Can you add a comment explaining this if statement?

Added a comment.

Android and Linux + Ozone don't support PlatformFont for now.  We cannot get any
font metrics on those platforms.

[msw, 2014-01-10 17:54:50]

https://codereview.chromium.org/124693003/diff/1590001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/124693003/diff/1590001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:101: // Note that not all the platforms support
PlatformFont.
What happens when the metics caching functions run in those scenarios? Is it
unsafe to run them and they should check this themselves, otherwise, why not
just run them here?

[Yuki, 2014-01-11 09:29:06]

https://codereview.chromium.org/124693003/diff/1590001/ui/gfx/font_list.cc
File ui/gfx/font_list.cc (right):

https://codereview.chromium.org/124693003/diff/1590001/ui/gfx/font_list.cc#ne...
ui/gfx/font_list.cc:101: // Note that not all the platforms support
PlatformFont.
On 2014/01/10 17:54:51, msw wrote:
> What happens when the metics caching functions run in those scenarios? Is it
> unsafe to run them and they should check this themselves, otherwise, why not
> just run them here?

If PlatformFont::CreateDefault() returns NULL, all metrics getter functions are
unsafe.  For example, Font::GetHeight(), Font::GetExpectedTextWidth(), etc. 
It's not only the metrics caching functions or FontList, but also Font.

Having said that, in that case, the metrics getter functions always crash
because of null pointer access and it's easy to detect.  So I don't think we
need to check the availability of PlatformFont every time.

[msw, 2014-01-11 18:50:59]

On 2014/01/11 09:29:06, Yuki wrote:
> https://codereview.chromium.org/124693003/diff/1590001/ui/gfx/font_list.cc
> File ui/gfx/font_list.cc (right):
> 
>
https://codereview.chromium.org/124693003/diff/1590001/ui/gfx/font_list.cc#ne...
> ui/gfx/font_list.cc:101: // Note that not all the platforms support
> PlatformFont.
> On 2014/01/10 17:54:51, msw wrote:
> > What happens when the metics caching functions run in those scenarios? Is it
> > unsafe to run them and they should check this themselves, otherwise, why not
> > just run them here?
> 
> If PlatformFont::CreateDefault() returns NULL, all metrics getter functions
are
> unsafe.  For example, Font::GetHeight(), Font::GetExpectedTextWidth(), etc. 
> It's not only the metrics caching functions or FontList, but also Font.
> 
> Having said that, in that case, the metrics getter functions always crash
> because of null pointer access and it's easy to detect.  So I don't think we
> need to check the availability of PlatformFont every time.

Ok, lgtm

[commit-bot: I haz the power, 2014-01-12 13:06:53]

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

[commit-bot: I haz the power, 2014-01-12 18:21:33]

Change committed as 244446