Implement fallback cache

Implement fallback cache

Adds logic to font fallback to track recently used fallback fonts, so
that we can avoid doing an IPC when rendering a page with many text
blocks that all use the same fallback font.
Also adds metrics to track the effectiveness of font fallback.

BUG=603314
Committed: https://crrev.com/d1b7cb69dada3cdb1aafb237429fac639041e10e
Cr-Commit-Position: refs/heads/master@{#389145}

[Ilya Kulshin, 2016-04-21 19:33:57]

Patchset #3 (id:40001) has been deleted

[Ilya Kulshin, 2016-04-21 19:34:05]

Patchset #2 (id:20001) has been deleted

[Ilya Kulshin, 2016-04-21 19:34:12]

Patchset #1 (id:1) has been deleted

[Ilya Kulshin, 2016-04-21 20:17:27]

kulshin@chromium.org changed reviewers:
+ ananta@chromium.org, rkaplow@chromium.org, scottmg@chromium.org

[Ilya Kulshin, 2016-04-21 20:17:28]

ptal.

scottmg@/ananta@ - content/child/dwrite_font_proxy/*
rkaplow@ - tools/metrics/histograms/histograms.xml

[rkaplow, 2016-04-21 22:36:55]

lgtm

histograms lgtm

[scottmg, 2016-04-21 22:45:49]

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
File content/child/dwrite_font_proxy/font_fallback_win.cc (right):

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:72: if
(GetCachedFont(text_chunk, base_family_name, base_weight, base_style,
It looks like if text_chunk.size() == 0, this will return false. Can that
happen? and/or is that what you want?

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:75:
DCHECK_GT(*mapped_length, 0u);
You didn't cache or return *scale here. Are you certain that's not necessary?

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:173: // different from
|mapped_length| because ReadUnicodeCharacter can andvance
andvance -> advance

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:214:
family_list.resize(kMaxFamilyCacheSize);
Can this just be pop_back()? I know the size is only 10 currently, but O(N)
resize() makes me jumpy.

[Ilya Kulshin, 2016-04-22 00:48:38]

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
File content/child/dwrite_font_proxy/font_fallback_win.cc (right):

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:72: if
(GetCachedFont(text_chunk, base_family_name, base_weight, base_style,
On 2016/04/21 22:45:49, scottmg wrote:
> It looks like if text_chunk.size() == 0, this will return false. Can that
> happen? and/or is that what you want?

That would mean we were asked to map an empty string, and doesn't really make
sense. Added a check to bail on the whole thing if it happens.

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:75:
DCHECK_GT(*mapped_length, 0u);
On 2016/04/21 22:45:49, scottmg wrote:
> You didn't cache or return *scale here. Are you certain that's not necessary?

I've never (and by never I mean "in the past month or so") seen a scale other
than 1, and Skia ignores |scale| anyway (in fact, it ignores everything other
than the family name it obtains from the font). So while we could cache |scale|,
I don't think the added complexity is justified. In the meantime, I added a
DCHECK and a comment.

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:173: // different from
|mapped_length| because ReadUnicodeCharacter can andvance
On 2016/04/21 22:45:49, scottmg wrote:
> andvance -> advance

Done.

https://codereview.chromium.org/1906633003/diff/60001/content/child/dwrite_fo...
content/child/dwrite_font_proxy/font_fallback_win.cc:214:
family_list.resize(kMaxFamilyCacheSize);
On 2016/04/21 22:45:49, scottmg wrote:
> Can this just be pop_back()? I know the size is only 10 currently, but O(N)
> resize() makes me jumpy.

Done.

[scottmg, 2016-04-22 02:17:00]

lgtm

[Ilya Kulshin, 2016-04-22 16:46:00]

The CQ bit was checked by kulshin@chromium.org

[Ilya Kulshin, 2016-04-22 16:46:00]

The patchset sent to the CQ was uploaded after l-g-t-m from rkaplow@chromium.org
Link to the patchset: https://codereview.chromium.org/1906633003/#ps80001
(title: "Add some checks and comments")

[commit-bot: I haz the power, 2016-04-22 16:46:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1906633003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1906633003/80001

[commit-bot: I haz the power, 2016-04-22 17:04:36]

Committed patchset #2 (id:80001)

[commit-bot: I haz the power, 2016-04-22 19:48:59]

Description was changed from

==========
Implement fallback cache

Adds logic to font fallback to track recently used fallback fonts, so
that we can avoid doing an IPC when rendering a page with many text
blocks that all use the same fallback font.
Also adds metrics to track the effectiveness of font fallback.

BUG=603314
==========

to

==========
Implement fallback cache

Adds logic to font fallback to track recently used fallback fonts, so
that we can avoid doing an IPC when rendering a page with many text
blocks that all use the same fallback font.
Also adds metrics to track the effectiveness of font fallback.

BUG=603314

Committed: https://crrev.com/d1b7cb69dada3cdb1aafb237429fac639041e10e
Cr-Commit-Position: refs/heads/master@{#389145}
==========

[commit-bot: I haz the power, 2016-04-22 19:49:00]

Patchset 2 (id:??) landed as
https://crrev.com/d1b7cb69dada3cdb1aafb237429fac639041e10e
Cr-Commit-Position: refs/heads/master@{#389145}