Skip redundant fonts returned by FontFallbackIterator

Skip redundant fonts returned by FontFallbackIterator

HarfBuzzShaper asks FontFallbackIterator::next for a font by calling
next() each time it is done shaping with this font and still has
non-rendered glyphs. Depending on what the font-stack, user-preference
font, and fallback fonts returned by the system are, the shaper may try
to shape with the same font several times, and still end up not finding a
result before the LastResort font.

Since HarfBuzzFace objects are cached internally to that class and
unified using Skia unique SkTypeface IDs, we can filter for SkTypeFace
uniqueIds earlier in FontFallbackIterator and skip redundant shaping.

Examples would be:
Empty font-family CSS, but CSS code seems to set the default serif font
before we reach shaping, then we try with that, and afterwards attempt
to shape with the users default font again, which is the same, Times New
Roman in both cases. Similarly, a font-family: that has Times or Times
New Roman which we alias to the same font, etc.

BUG=636347
TEST=Manually tested this by adding debug code to HarfBuzzShaper and
detecting incoming duplicates.

Committed: https://crrev.com/5fdb26babe2d64e6f09175edffd5efe4abf220d8
Cr-Commit-Position: refs/heads/master@{#411164}

[drott, 2016-08-10 13:22:11]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 13:22:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-08-10 14:05:53]

drott@chromium.org changed reviewers:
+ behdad@chromium.org, eae@chromium.org, kojii@chromium.org

[eae, 2016-08-10 14:13:24]

LGTM

[commit-bot: I haz the power, 2016-08-10 14:30:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 14:30:29]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[drott, 2016-08-10 14:42:18]

Hmh, I was hoping the Linux failure was due to my local font configuration but
looks like I do need to compare the previously used UnicodeRangeSet as well.
Will update the CL.

[drott, 2016-08-10 15:02:52]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 15:03:06]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-08-10 15:03:35]

The CQ bit was checked by drott@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 15:03:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[drott, 2016-08-10 15:05:53]

On 2016/08/10 at 14:42:18, drott wrote:
> Hmh, I was hoping the Linux failure was due to my local font configuration but
looks like I do need to compare the previously used UnicodeRangeSet as well.
Will update the CL.

Turns out it makes more sense not to skip subsetted redundant ranges as the
shaping behavior for those differs. We mainly want to avoid shaping with the
same system font several times.

[commit-bot: I haz the power, 2016-08-10 15:13:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 15:13:42]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[drott, 2016-08-10 20:48:44]

The CQ bit was checked by drott@chromium.org

[drott, 2016-08-10 20:48:45]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/2230233002/#ps40001
(title: "Logging removed")

[commit-bot: I haz the power, 2016-08-10 20:50:10]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 22:29:51]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-10 22:31:38]

Description was changed from

==========
Skip redundant fonts returned by FontFallbackIterator

HarfBuzzShaper asks FontFallbackIterator::next for a font by calling
next() each time it is done shaping with this font and still has
non-rendered glyphs. Depending on what the font-stack, user-preference
font, and fallback fonts returned by the system are, the shaper may try
to shape with the same font several times, and still end up not finding a
result before the LastResort font.

Since HarfBuzzFace objects are cached internally to that class and
unified using Skia unique SkTypeface IDs, we can filter for SkTypeFace
uniqueIds earlier in FontFallbackIterator and skip redundant shaping.

Examples would be:
Empty font-family CSS, but CSS code seems to set the default serif font
before we reach shaping, then we try with that, and afterwards attempt
to shape with the users default font again, which is the same, Times New
Roman in both cases. Similarly, a font-family: that has Times or Times
New Roman which we alias to the same font, etc.

BUG=636347
TEST=Manually tested this by adding debug code to HarfBuzzShaper and
detecting incoming duplicates.
==========

to

==========
Skip redundant fonts returned by FontFallbackIterator

HarfBuzzShaper asks FontFallbackIterator::next for a font by calling
next() each time it is done shaping with this font and still has
non-rendered glyphs. Depending on what the font-stack, user-preference
font, and fallback fonts returned by the system are, the shaper may try
to shape with the same font several times, and still end up not finding a
result before the LastResort font.

Since HarfBuzzFace objects are cached internally to that class and
unified using Skia unique SkTypeface IDs, we can filter for SkTypeFace
uniqueIds earlier in FontFallbackIterator and skip redundant shaping.

Examples would be:
Empty font-family CSS, but CSS code seems to set the default serif font
before we reach shaping, then we try with that, and afterwards attempt
to shape with the users default font again, which is the same, Times New
Roman in both cases. Similarly, a font-family: that has Times or Times
New Roman which we alias to the same font, etc.

BUG=636347
TEST=Manually tested this by adding debug code to HarfBuzzShaper and
detecting incoming duplicates.

Committed: https://crrev.com/5fdb26babe2d64e6f09175edffd5efe4abf220d8
Cr-Commit-Position: refs/heads/master@{#411164}
==========

[commit-bot: I haz the power, 2016-08-10 22:31:39]

Patchset 3 (id:??) landed as
https://crrev.com/5fdb26babe2d64e6f09175edffd5efe4abf220d8
Cr-Commit-Position: refs/heads/master@{#411164}

[behdad, 2016-08-15 21:09:19]

https://codereview.chromium.org/2230233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontFallbackIterator.cpp (right):

https://codereview.chromium.org/2230233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontFallbackIterator.cpp:74: if
(m_uniqueFontDataForRangeSetsReturned.contains(candidateId)) {
Shouldn't this be a while loop, skipping until something unseen is seen?

[drott, 2016-08-23 18:20:31]

https://codereview.chromium.org/2230233002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/fonts/FontFallbackIterator.cpp (right):

https://codereview.chromium.org/2230233002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/fonts/FontFallbackIterator.cpp:74: if
(m_uniqueFontDataForRangeSetsReturned.contains(candidateId)) {
On 2016/08/15 at 21:09:19, behdad wrote:
> Shouldn't this be a while loop, skipping until something unseen is seen?

Can you explain a bit more what you were expecting instead? I am using the
uniqueOrNext() as a wrapper for the return of the next() iterator call. If the
next() iterator has found a font, but filtering it in uniqueOrNext() finds that
this font has already been returned, we continue the recursion and call next()
to continue searching for a new font.