Make CSSFontFace::willUseFontData() load fonts with unicode-range

Make CSSFontFace::willUseFontData() load fonts with unicode-range

Before this patch CSSFontFace::willUseFontData() loads font faces that
have no unicode-range. Since font faces with no unicode-range tends to
be used as fallback font of segmented font family, this behavior leads
to unnecessary font downloads.

This patch makes willUseFontData() loads the first unloaded font face
whose unicode-range intersects with given text. That check does not
need to be 100% precise (false negative is ok), so it only checks the
first character of the text, for speed.

TEST=fast/css/font-face-unicode-range-overlap-load.html
BUG=247920, 246492
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172943

[Kunihiko Sakamoto, 2014-04-28 10:19:41]

Friendly ping :)

[dglazkov, 2014-04-29 16:37:00]

lgtm.

I am going to rant a little bit that willUseFontData being called in
didChangeStyle seems like a wrong abstraction, but since I don't have a clue on
how to fix it, this rant will be short :)

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
File Source/core/css/CSSFontFace.cpp (right):

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
Source/core/css/CSSFontFace.cpp:134: if (!text.isEmpty() &&
m_ranges.contains(text.characterStartingAt(0))) {
text.isEmpty check is not necessary, right? The RenderText::styleDidChange
already checks for this with text().containsOnlyWhitespace().

Which makes me wonder if we could just pass the first character?

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
File Source/core/css/CSSFontFace.h (right):

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
Source/core/css/CSSFontFace.h:110: bool willUseFontData(const FontDescription&,
const String& text);
Might need to rename this into something that makes sense to return a boolean.
This function is really trying to opportunistically start a load, rather than
"use font data" :)

[Kunihiko Sakamoto, 2014-04-30 03:31:21]

> I am going to rant a little bit that willUseFontData being called in
> didChangeStyle seems like a wrong abstraction, but since I don't have a clue
on
> how to fix it, this rant will be short :)

Yeah, I do have the same feeling, but the benefit of this optimization is too
huge to give up... :)

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
File Source/core/css/CSSFontFace.cpp (right):

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
Source/core/css/CSSFontFace.cpp:134: if (!text.isEmpty() &&
m_ranges.contains(text.characterStartingAt(0))) {
On 2014/04/29 16:37:00, dglazkov wrote:
> text.isEmpty check is not necessary, right? The RenderText::styleDidChange
> already checks for this with text().containsOnlyWhitespace().
> 
> Which makes me wonder if we could just pass the first character?

Ah, you're right.
Changed to pass the first character.

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
File Source/core/css/CSSFontFace.h (right):

https://codereview.chromium.org/248473005/diff/60001/Source/core/css/CSSFontF...
Source/core/css/CSSFontFace.h:110: bool willUseFontData(const FontDescription&,
const String& text);
On 2014/04/29 16:37:00, dglazkov wrote:
> Might need to rename this into something that makes sense to return a boolean.
> This function is really trying to opportunistically start a load, rather than
> "use font data" :)

Renamed to maybeScheduleFontLoad.

[Kunihiko Sakamoto, 2014-04-30 03:45:13]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-04-30 03:45:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ksakamoto@chromium.org/248473005/100001

[commit-bot: I haz the power, 2014-04-30 05:15:37]

Change committed as 172943

[enne (OOO), 2014-05-02 21:42:16]

A revert of this CL has been created in
https://codereview.chromium.org/261753011/ by enne@chromium.org.

The reason for reverting is: Causes off-by-one text errors, e.g. SHOP->RGNO

BUG=369633.