CSS Font Loading: FontFaceSet.ready should return the same promise each time

Source/core/css/FontFaceSet.cpp

Index: Source/core/css/FontFaceSet.cpp
diff --git a/Source/core/css/FontFaceSet.cpp b/Source/core/css/FontFaceSet.cpp
index 6f61116886ce53aa0277e4e6510263de054e691e..3bf58c463c4690cbb5a02005041a7d7ccecb99a8 100644
--- a/Source/core/css/FontFaceSet.cpp
+++ b/Source/core/css/FontFaceSet.cpp
@@ -114,34 +114,6 @@ DEFINE_TRACE(LoadFontPromiseResolver)
     LoadFontCallback::trace(visitor);
 }
 
-class FontsReadyPromiseResolver final : public GarbageCollected<FontsReadyPromiseResolver> {
-public:
-    static FontsReadyPromiseResolver* create(ScriptState* scriptState)
-    {
-        return new FontsReadyPromiseResolver(scriptState);
-    }
-
-    void resolve(PassRefPtrWillBeRawPtr<FontFaceSet> fontFaceSet)
-    {
-        m_resolver->resolve(fontFaceSet);
-    }
-
-    ScriptPromise promise() { return m_resolver->promise(); }
-
-    DEFINE_INLINE_TRACE()
-    {
-        visitor->trace(m_resolver);
-    }
-
-private:
-    explicit FontsReadyPromiseResolver(ScriptState* scriptState)
-        : m_resolver(ScriptPromiseResolver::create(scriptState))
-    {
-    }
-
-    Member<ScriptPromiseResolver> m_resolver;
-};
-
 FontFaceSet::FontFaceSet(Document& document)
     : ActiveDOMObject(&document)
     , m_shouldFireLoadingEvent(false)
@@ -199,11 +171,18 @@ void FontFaceSet::didLayout()
 {
     if (document()->frame()->isMainFrame() && m_loadingFonts.isEmpty())
         m_histogram.record();
-    if (!m_loadingFonts.isEmpty() || (!m_isLoading && m_readyResolvers.isEmpty()))
+    if (!shouldSignalReady())
         return;
     handlePendingEventsAndPromisesSoon();
 }
 
+bool FontFaceSet::shouldSignalReady() const
+{
+    if (!m_loadingFonts.isEmpty())
+        return false;
+    return m_isLoading || (m_ready && m_ready->state() == ReadyProperty::Pending);
+}
+
 void FontFaceSet::handlePendingEventsAndPromises()
 {
     fireLoadingEvent();
@@ -258,6 +237,8 @@ void FontFaceSet::addToLoadingFonts(PassRefPtrWillBeRawPtr<FontFace> fontFace)
     if (!m_isLoading) {
         m_isLoading = true;
         m_shouldFireLoadingEvent = true;
+        if (m_ready && m_ready->state() != ReadyProperty::Pending)
+            m_ready->reset();
         handlePendingEventsAndPromisesSoon();
     }
     m_loadingFonts.add(fontFace);
@@ -274,11 +255,11 @@ ScriptPromise FontFaceSet::ready(ScriptState* scriptState)
 {
     if (!inActiveDocumentContext())
         return ScriptPromise();
-    FontsReadyPromiseResolver* resolver = FontsReadyPromiseResolver::create(scriptState);
-    ScriptPromise promise = resolver->promise();
-    m_readyResolvers.append(resolver);
+
+    if (!m_ready)

[dominicc (has gone to gerrit), 2015/09/02 00:14:06]

Pretty much everyone creates these lazily, and while they're to *free*, is it
worth it? Could you always initialize m_ready and simplify your code (no if
(m_ready); no m_ready &&). You might even be able to make the implementation of
status just check the state of the Promise for some things (eg if pending, then
status is loading) and simplify things even more.

[Kunihiko Sakamoto, 2015/09/02 05:05:42]

Done.
I think m_isLoading is still needed, since FontFaceSet.status is iniitally
"loaded" while .ready starts with a pending promise.

+        m_ready = new ReadyProperty(executionContext(), this, ReadyProperty::Ready);
     handlePendingEventsAndPromisesSoon();
-    return promise;
+    return m_ready->promise(scriptState->world());
 }
 
 void FontFaceSet::add(FontFace* fontFace, ExceptionState& exceptionState)
@@ -406,7 +387,7 @@ void FontFaceSet::fireDoneEventIfPossible()
 {
     if (m_shouldFireLoadingEvent)
         return;
-    if (!m_loadingFonts.isEmpty() || (!m_isLoading && m_readyResolvers.isEmpty()))
+    if (!shouldSignalReady())
         return;
 
     // If the layout was invalidated in between when we thought layout
@@ -431,12 +412,8 @@ void FontFaceSet::fireDoneEventIfPossible()
             dispatchEvent(errorEvent);
     }
 
-    if (!m_readyResolvers.isEmpty()) {
-        HeapVector<Member<FontsReadyPromiseResolver>> resolvers;
-        m_readyResolvers.swap(resolvers);
-        for (size_t index = 0; index < resolvers.size(); ++index)
-            resolvers[index]->resolve(this);
-    }
+    if (m_ready && m_ready->state() == ReadyProperty::Pending)
+        m_ready->resolve(this);
 }
 
 ScriptPromise FontFaceSet::load(ScriptState* scriptState, const String& fontString, const String& text)
@@ -582,8 +559,8 @@ void FontFaceSet::didLayout(Document& document)
 DEFINE_TRACE(FontFaceSet)
 {
 #if ENABLE(OILPAN)
+    visitor->trace(m_ready);
     visitor->trace(m_loadingFonts);
-    visitor->trace(m_readyResolvers);
     visitor->trace(m_loadedFonts);
     visitor->trace(m_failedFonts);
     visitor->trace(m_nonCSSConnectedFaces);