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

Source/core/css/FontFaceSet.cpp

 /*
  * Copyright (C) 2013 Google Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are met:
  *
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 96 matching lines @@
     }
 }
 
 DEFINE_TRACE(LoadFontPromiseResolver)
 {
     visitor->trace(m_fontFaces);
     visitor->trace(m_resolver);
     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)
     , m_isLoading(false)
     , m_asyncRunner(this, &FontFaceSet::handlePendingEventsAndPromises)
 {
     suspendIfNeeded();
 }
 
 FontFaceSet::~FontFaceSet()
@@ skipping 37 matching lines @@
 void FontFaceSet::handlePendingEventsAndPromisesSoon()
 {
     // m_asyncRunner will be automatically stopped on destruction.
     m_asyncRunner.runAsync();
 }
 
 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();
     fireDoneEventIfPossible();
 }
 
 void FontFaceSet::fireLoadingEvent()
 {
     if (m_shouldFireLoadingEvent) {
         m_shouldFireLoadingEvent = false;
@@ skipping 34 matching lines @@
     m_histogram.updateStatus(fontFace);
     m_failedFonts.append(fontFace);
     removeFromLoadingFonts(fontFace);
 }
 
 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);
 }
 
 void FontFaceSet::removeFromLoadingFonts(PassRefPtrWillBeRawPtr<FontFace> fontFace)
 {
     m_loadingFonts.remove(fontFace);
     if (m_loadingFonts.isEmpty())
         handlePendingEventsAndPromisesSoon();
 }
 
 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)
 {
     if (!inActiveDocumentContext())
         return;
     if (!fontFace) {
         exceptionState.throwTypeError("The argument is not a FontFace.");
         return;
     }
@@ skipping 107 matching lines @@
 {
     if (!inActiveDocumentContext())
         return m_nonCSSConnectedFaces.size();
     return cssConnectedFontFaceList().size() + m_nonCSSConnectedFaces.size();
 }
 
 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
     // was updated and when we're ready to fire the event, just wait
     // until after the next layout before firing events.
     Document* d = document();
     if (!d->view() || d->view()->needsLayout())
         return;
 
     if (m_isLoading) {
         RefPtrWillBeRawPtr<FontFaceSetLoadEvent> doneEvent = nullptr;
         RefPtrWillBeRawPtr<FontFaceSetLoadEvent> errorEvent = nullptr;
         doneEvent = FontFaceSetLoadEvent::createForFontFaces(EventTypeNames::loadingdone, m_loadedFonts);
         m_loadedFonts.clear();
         if (!m_failedFonts.isEmpty()) {
             errorEvent = FontFaceSetLoadEvent::createForFontFaces(EventTypeNames::loadingerror, m_failedFonts);
             m_failedFonts.clear();
         }
         m_isLoading = false;
         dispatchEvent(doneEvent);
         if (errorEvent)
             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)
 {
     if (!inActiveDocumentContext())
         return ScriptPromise();
 
     Font font;
     if (!resolveFontStyle(fontString, font)) {
         ScriptPromiseResolver* resolver = ScriptPromiseResolver::create(scriptState);
@@ skipping 125 matching lines @@
 
 void FontFaceSet::didLayout(Document& document)
 {
     if (FontFaceSet* fonts = static_cast<FontFaceSet*>(SupplementType::from(document, supplementName())))
         fonts->didLayout();
 }
 
 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);
     HeapSupplement<Document>::trace(visitor);
 #endif
     EventTargetWithInlineData::trace(visitor);
     ActiveDOMObject::trace(visitor);
 }
 
 } // namespace blink