Webfonts should not block load event for more than one frame

Webfonts should not block load event for more than one frame

This patch fixes a bug that frame's load event does not fire when
FontLoader::loadPendingFonts does not start any load because all the
pending fonts are already started loading. This happens when a webfont
is used in multiple iframes; only one of them actually starts the load.

After this patch, if a font is used in multiple frames, subsequent
frames do not schedule a load (hence do not block the load event).

BUG=401846
TEST=fast/css/font-face-iframe-onload.html

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

[Nate Chapin, 2014-08-11 22:06:16]

https://codereview.chromium.org/457273002/diff/40001/Source/core/css/FontLoad...
File Source/core/css/FontLoader.cpp (right):

https://codereview.chromium.org/457273002/diff/40001/Source/core/css/FontLoad...
Source/core/css/FontLoader.cpp:76: document->checkLoadEventSoon();
Do we need to block the load event on webfonts in general?

Assuming we do, this is not very intuitive. Why are we putting fonts in
m_fontsToBeginLoading if we don't need to load them?

Would it be possible to ensure we don't add fonts to m_fontsToBeginLoading if
they are already started, and to remove them if another frame starts them
loading? That would presumably require changing m_fontsToBeginLoading to a
HashMap instead of a Vector, but probably more readable in the long run.

[Kunihiko Sakamoto, 2014-08-12 03:12:09]

Thank you for the review.

https://codereview.chromium.org/457273002/diff/40001/Source/core/css/FontLoad...
File Source/core/css/FontLoader.cpp (right):

https://codereview.chromium.org/457273002/diff/40001/Source/core/css/FontLoad...
Source/core/css/FontLoader.cpp:76: document->checkLoadEventSoon();
> Do we need to block the load event on webfonts in general?
It worked this way for years and I think many websites depends on this behavior.

> Would it be possible to ensure we don't add fonts to m_fontsToBeginLoading if
they are already started,
We don't add them; if a font is already started fontResource->stillNeedsLoad()
at line 39 returns false.

> and to remove them if another frame starts them loading?
Adding an entry to m_fontsToBeginLoading creates an instance of
ResourceLoader::RequestCountTracker, that prevent the load event. The destructor
of ResourceLoader::RequestCountTracker just decrements the request count and
doesn't trigger the load event.

Perhaps we should schedule load event in ResourceLoader::~RequestCountTracker()
if the request count became zero?

[Nate Chapin, 2014-08-12 16:08:22]

On 2014/08/12 03:12:09, Kunihiko Sakamoto wrote:
> Thank you for the review.
> 
>
https://codereview.chromium.org/457273002/diff/40001/Source/core/css/FontLoad...
> File Source/core/css/FontLoader.cpp (right):
> 
>
https://codereview.chromium.org/457273002/diff/40001/Source/core/css/FontLoad...
> Source/core/css/FontLoader.cpp:76: document->checkLoadEventSoon();
> > Do we need to block the load event on webfonts in general?
> It worked this way for years and I think many websites depends on this
behavior.
> 
> > Would it be possible to ensure we don't add fonts to m_fontsToBeginLoading
if
> they are already started,
> We don't add them; if a font is already started fontResource->stillNeedsLoad()
> at line 39 returns false.
> 
> > and to remove them if another frame starts them loading?
> Adding an entry to m_fontsToBeginLoading creates an instance of
> ResourceLoader::RequestCountTracker, that prevent the load event. The
destructor
> of ResourceLoader::RequestCountTracker just decrements the request count and
> doesn't trigger the load event.
> 
> Perhaps we should schedule load event in
ResourceLoader::~RequestCountTracker()
> if the request count became zero?

I am worried that this is broad fix for a narrow bug, and that we already check
the load event in too many places :)

What if RemoteFontFaceSource::didStartFontLoad() called a new function named
FontLoader::didStartFontLoad()? FontLoader::didStartFontLoad() would then remove
that FontResource from m_fontsToBeginLoading. So we only destruct a
RequestCountTracker in FontLoader when we also create a new one in
ResoureceLoader, and we don't need to call checkLoadEventSoon() in FontLoader.

Would that work?

[Kunihiko Sakamoto, 2014-08-14 03:44:01]

Calling checkLoadEventSoon() in FontLoader seems not a good idea, so I've
rewritten my patch in another approach.

Now FontResource has a flag which represents whether the load is scheduled by a
FontLoader. If a font is used in multiple frames, subsequent frames do not
schedule a load (hence do not block the load event).

PTAL?

[Nate Chapin, 2014-08-14 17:33:06]

LGTM. This is no worse than the current situation with subresources.

It's a known issue (in ResourceLoader and RequestCountTracker) that if multiple
frames request the same resource before the request completes, we will only
block the load event for the frame that requested the resource first.

We should fix that someday, but not in FontLoader :)

https://codereview.chromium.org/457273002/diff/80001/Source/core/fetch/FontRe...
File Source/core/fetch/FontResource.h (right):

https://codereview.chromium.org/457273002/diff/80001/Source/core/fetch/FontRe...
Source/core/fetch/FontResource.h:83: bool m_loadScheduled;
Should m_loadScheduled and m_loadInitiated be merged into an enum?

[Kunihiko Sakamoto, 2014-08-15 01:30:23]

Thanks!

https://codereview.chromium.org/457273002/diff/80001/Source/core/fetch/FontRe...
File Source/core/fetch/FontResource.h (right):

https://codereview.chromium.org/457273002/diff/80001/Source/core/fetch/FontRe...
Source/core/fetch/FontResource.h:83: bool m_loadScheduled;
On 2014/08/14 17:33:06, Nate Chapin wrote:
> Should m_loadScheduled and m_loadInitiated be merged into an enum?

Done.

[Kunihiko Sakamoto, 2014-08-15 01:35:19]

The CQ bit was checked by ksakamoto@chromium.org

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

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

[commit-bot: I haz the power, 2014-08-15 02:36:22]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  mac_gpu_retina_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu_retina_tr...)

[commit-bot: I haz the power, 2014-08-15 03:04:50]

Committed patchset #3 (100001) as 180321