Make text visible when font loading takes longer than 3 seconds

Make text visible when font loading takes longer than 3 seconds

When a font resource takes a long time to download, currently Blink
keeps the text invisible until the font is ready. After this patch,
Blink behaves similar to FireFox; keeps the text invisible until 3
seconds, then fallback to the next available font in the fallback
list. When the webfont finally loads, change the text to use it.

- Add a Timer to FontResource that fires after 3 seconds since load start.
- fontLoadWaitLimitExceeded() callback invalidates the fallback font and
  triggers style recalc.
- Fallback fonts have m_skipDrawing boolean in CustomFontData, that is
  false if the font load exceeds the wait limit.
- FontFallbackList::shouldSkipDrawing() returns true if it contains at
  least one FontData with m_skipDrawing == true.

BUG=235303
TEST=http/tests/webfont/slow-loading.html

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=168258

[eae, 2014-02-20 16:51:46]

The logic and tests looks good to me I'm not sure what our policy on adding new
timers is though and whether there is a better way to do that. Dimitri might
know.

https://codereview.chromium.org/171823002/diff/130001/LayoutTests/http/tests/...
File LayoutTests/http/tests/webfont/slow-loading.html (right):

https://codereview.chromium.org/171823002/diff/130001/LayoutTests/http/tests/...
LayoutTests/http/tests/webfont/slow-loading.html:67:
displayElementAfter("span2", 0);
What is the purpose of the msec argument here and why is it different for the
three spans?

[dglazkov, 2014-02-20 17:45:40]

I am not up to speed on the latest thinking in regard to timers, but I know that
jamesr or/and abarth can give you great advice.

Can we not simply treat this limit as a loading error and treat them the same
way in the code? I guess I don't understand why we need a special case for
timeout.

https://codereview.chromium.org/171823002/diff/130001/Source/core/css/CSSCust...
File Source/core/css/CSSCustomFontData.h (right):

https://codereview.chromium.org/171823002/diff/130001/Source/core/css/CSSCust...
Source/core/css/CSSCustomFontData.h:31: static PassRefPtr<CSSCustomFontData>
create(bool isLoadingFallback = false, bool skipDrawing = false)
We really should avoid adding bools as params. A long while, at WebKit and then
Blink we agreed that enum flags work better here.

[Kunihiko Sakamoto, 2014-02-21 03:38:05]

On 2014/02/20 17:45:40, Dimitri Glazkov wrote:
> Can we not simply treat this limit as a loading error and treat them the same
> way in the code? I guess I don't understand why we need a special case for
> timeout.

We don't want to abort font download in 3 seconds - it's too short for download
timeout.
The load should be continued, and we need to update text when the font loads.

[Kunihiko Sakamoto, 2014-02-21 03:38:24]

https://codereview.chromium.org/171823002/diff/130001/LayoutTests/http/tests/...
File LayoutTests/http/tests/webfont/slow-loading.html (right):

https://codereview.chromium.org/171823002/diff/130001/LayoutTests/http/tests/...
LayoutTests/http/tests/webfont/slow-loading.html:67:
displayElementAfter("span2", 0);
On 2014/02/20 16:51:47, eae wrote:
> What is the purpose of the msec argument here and why is it different for the
> three spans?

For span3, we want to start the loading at T=3(sec) so the load does not exceed
the wait limit at the point of test completion (T=5).
The zero timeouts for span1 and span2 are subtle: we need to start loading them
*after* onload(), otherwise TestRunner waits for these loads to complete.

https://codereview.chromium.org/171823002/diff/130001/Source/core/css/CSSCust...
File Source/core/css/CSSCustomFontData.h (right):

https://codereview.chromium.org/171823002/diff/130001/Source/core/css/CSSCust...
Source/core/css/CSSCustomFontData.h:31: static PassRefPtr<CSSCustomFontData>
create(bool isLoadingFallback = false, bool skipDrawing = false)
On 2014/02/20 17:45:41, Dimitri Glazkov wrote:
> We really should avoid adding bools as params. A long while, at WebKit and
then
> Blink we agreed that enum flags work better here.

Done.

[Kunihiko Sakamoto, 2014-02-24 06:58:27]

James and Adam, any advice on the use of timers?

[abarth-chromium, 2014-02-24 08:28:23]

On 2014/02/24 06:58:27, Kunihiko Sakamoto wrote:
> James and Adam, any advice on the use of timers?

We still have lots of timers.  I wouldn't worry too much about it.

[Kunihiko Sakamoto, 2014-02-25 01:19:09]

On 2014/02/24 08:28:23, abarth wrote:
> On 2014/02/24 06:58:27, Kunihiko Sakamoto wrote:
> > James and Adam, any advice on the use of timers?
> 
> We still have lots of timers.  I wouldn't worry too much about it.

Thanks Adam.

eae/dglazkov, could you take another look?

[Kunihiko Sakamoto, 2014-02-27 15:14:38]

On 2014/02/25 01:19:09, Kunihiko Sakamoto wrote:
> On 2014/02/24 08:28:23, abarth wrote:
> > On 2014/02/24 06:58:27, Kunihiko Sakamoto wrote:
> > > James and Adam, any advice on the use of timers?
> > 
> > We still have lots of timers.  I wouldn't worry too much about it.
> 
> Thanks Adam.
> 
> eae/dglazkov, could you take another look?

PTAL?

[Kunihiko Sakamoto, 2014-03-01 00:19:28]

ping...

[dglazkov, 2014-03-01 03:44:23]

lgtm.

[Kunihiko Sakamoto, 2014-03-03 00:49:47]

The CQ bit was checked by ksakamoto@chromium.org

[commit-bot: I haz the power, 2014-03-03 00:49:52]

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

[commit-bot: I haz the power, 2014-03-03 03:15:16]

Change committed as 168258