Webfonts should not block load event for more than one frame

Source/core/css/FontLoader.cpp

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "config.h"
 #include "core/css/FontLoader.h"
 
 #include "core/css/CSSFontSelector.h"
+#include "core/dom/Document.h"
 #include "core/fetch/FontResource.h"
 #include "core/fetch/ResourceFetcher.h"
 
 namespace blink {
 
 FontLoader::FontLoader(CSSFontSelector* fontSelector, ResourceFetcher* resourceFetcher)
     : m_beginLoadingTimer(this, &FontLoader::beginLoadTimerFired)
     , m_fontSelector(fontSelector)
     , m_resourceFetcher(resourceFetcher)
 {
@@ skipping 27 matching lines @@
 
 void FontLoader::beginLoadTimerFired(Timer<blink::FontLoader>*)
 {
     loadPendingFonts();
 }
 
 void FontLoader::loadPendingFonts()
 {
     ASSERT(m_resourceFetcher);
 
+    if (m_fontsToBeginLoading.isEmpty())
+        return;
+
     FontsToLoadVector fontsToBeginLoading;
     fontsToBeginLoading.swap(m_fontsToBeginLoading);
+    bool loaded = false;
     for (FontsToLoadVector::iterator it = fontsToBeginLoading.begin(); it != fontsToBeginLoading.end(); ++it) {
         FontResource* fontResource = it->first.get();
-        fontResource->beginLoadIfNeeded(m_resourceFetcher);
+        if (fontResource->stillNeedsLoad()) {
+            fontResource->beginLoadIfNeeded(m_resourceFetcher);
+            loaded = true;
+        }
+    }
+
+    if (!loaded) {
+        // Since we have prevented load event by RequestCountTracker but did not
+        // start any load, schedule a check for load event here.
+        Document* document = m_resourceFetcher->document();
+        if (document && !document->loadEventFinished() && !document->isDelayingLoadEvent())
+            document->checkLoadEventSoon();

[Nate Chapin, 2014/08/11 22:06:15]

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]

It worked this way for years and I think many websites depends on this behavior.
We don't add them; if a font is already started fontResource->stillNeedsLoad()
at line 39 returns false.
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?

     }
 
     // When the local fontsToBeginLoading vector goes out of scope it will
     // decrement the request counts on the ResourceFetcher for all the fonts
     // that were just loaded.
 }
 
 void FontLoader::fontFaceInvalidated()
 {
     if (m_fontSelector)
@@ skipping 15 matching lines @@
 }
 #endif
 
 void FontLoader::trace(Visitor* visitor)
 {
     visitor->trace(m_resourceFetcher);
     visitor->trace(m_fontSelector);
 }
 
 } // namespace blink