Rework MediaQueryMatcher to batch up listener notification

Source/core/css/MediaQueryMatcher.cpp

Index: Source/core/css/MediaQueryMatcher.cpp
diff --git a/Source/core/css/MediaQueryMatcher.cpp b/Source/core/css/MediaQueryMatcher.cpp
index 1821634634a933a8d05996901e702342f4529896..04785f07dccda0e94b15baab3bc2d793ef311e0e 100644
--- a/Source/core/css/MediaQueryMatcher.cpp
+++ b/Source/core/css/MediaQueryMatcher.cpp
@@ -31,37 +31,25 @@
 
 namespace WebCore {
 
-MediaQueryMatcher::Listener::Listener(PassRefPtrWillBeRawPtr<MediaQueryListListener> listener, PassRefPtrWillBeRawPtr<MediaQueryList> query)
-    : m_listener(listener)
-    , m_query(query)
-{
-}
-
-void MediaQueryMatcher::Listener::evaluate(MediaQueryEvaluator* evaluator)
-{
-    if (m_query->evaluate(evaluator))
-        m_listener->queryChanged(m_query.get());
-}
-
-void MediaQueryMatcher::Listener::trace(Visitor* visitor)
-{
-    visitor->trace(m_listener);
-    visitor->trace(m_query);
-}
-
-MediaQueryMatcher::MediaQueryMatcher(Document* document)
-    : m_document(document)
-    , m_evaluationRound(1)
+MediaQueryMatcher::MediaQueryMatcher(Document& document)
+    : m_document(&document)
 {
     ASSERT(m_document);
 }
 
 DEFINE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED(MediaQueryMatcher)
 
-void MediaQueryMatcher::documentDestroyed()
+void MediaQueryMatcher::documentDetached()
 {
-    m_listeners.clear();
     m_document = nullptr;
+
+    // Take a ref to each MediaQuerylist as removing the listeners in documentDetached
+    // could release the last ref and mutate the m_mediaLists.
+    WillBeHeapVector<RefPtrWillBeMember<MediaQueryList> > lists;
+    copyToVector(m_mediaLists, lists);
+
+    for (size_t i = 0; i < lists.size(); ++i)
+        lists[i]->documentDetached();
 }
 
 AtomicString MediaQueryMatcher::mediaType() const
@@ -72,7 +60,7 @@ AtomicString MediaQueryMatcher::mediaType() const
     return m_document->frame()->view()->mediaType();
 }
 
-PassOwnPtr<MediaQueryEvaluator> MediaQueryMatcher::prepareEvaluator() const
+PassOwnPtr<MediaQueryEvaluator> MediaQueryMatcher::createEvaluator() const
 {
     if (!m_document || !m_document->frame())
         return nullptr;
@@ -85,8 +73,13 @@ bool MediaQueryMatcher::evaluate(const MediaQuerySet* media)
     if (!media)
         return false;
 
-    OwnPtr<MediaQueryEvaluator> evaluator(prepareEvaluator());
-    return evaluator && evaluator->eval(media);
+    if (m_evaluator)
+        return m_evaluator->eval(media);
+
+    if (OwnPtr<MediaQueryEvaluator> evaluator = createEvaluator())
+        return evaluator->eval(media);

[Yoav Weiss, 2014/06/26 04:43:19]

Why aren't you updating m_evaluator here?

+
+    return false;
 }
 
 PassRefPtrWillBeRawPtr<MediaQueryList> MediaQueryMatcher::matchMedia(const String& query)
@@ -97,57 +90,47 @@ PassRefPtrWillBeRawPtr<MediaQueryList> MediaQueryMatcher::matchMedia(const Strin
     RefPtrWillBeRawPtr<MediaQuerySet> media = MediaQuerySet::create(query);
     // Add warning message to inspector whenever dpi/dpcm values are used for "screen" media.
     reportMediaQueryWarningIfNeeded(m_document, media.get());
-    return MediaQueryList::create(this, media, evaluate(media.get()));
+    return MediaQueryList::create(this, media);
 }
 
-void MediaQueryMatcher::addListener(PassRefPtrWillBeRawPtr<MediaQueryListListener> listener, PassRefPtrWillBeRawPtr<MediaQueryList> query)
+void MediaQueryMatcher::addMediaQueryList(MediaQueryList* query)
 {
     if (!m_document)
         return;
-
-    for (size_t i = 0; i < m_listeners.size(); ++i) {
-        if (*m_listeners[i]->listener() == *listener && m_listeners[i]->query() == query)
-            return;
-    }
-
-    m_listeners.append(adoptPtrWillBeNoop(new Listener(listener, query)));
+    m_mediaLists.add(query);
 }
 
-void MediaQueryMatcher::removeListener(MediaQueryListListener* listener, MediaQueryList* query)
+void MediaQueryMatcher::removeMediaQueryList(MediaQueryList* query)
 {
     if (!m_document)
         return;
-
-    for (size_t i = 0; i < m_listeners.size(); ++i) {
-        if (*m_listeners[i]->listener() == *listener && m_listeners[i]->query() == query) {
-            m_listeners.remove(i);
-            return;
-        }
-    }
+    m_mediaLists.remove(query);
 }
 
-void MediaQueryMatcher::styleResolverChanged()
+void MediaQueryMatcher::mediaFeaturesChanged()
 {
     if (!m_document)
         return;
 
-    ++m_evaluationRound;
-    OwnPtr<MediaQueryEvaluator> evaluator = prepareEvaluator();
-    if (!evaluator)
+    // Cache an evaluator so we don't allocate one for each list below.
+    m_evaluator = createEvaluator();
+    if (!m_evaluator)
         return;
 
-    for (size_t i = 0; i < m_listeners.size(); ++i)
-        m_listeners[i]->evaluate(evaluator.get());
+    WillBeHeapVector<RefPtrWillBeMember<MediaQueryListListener> > listenersToNotify;
+    for (MediaQueryListSet::iterator it = m_mediaLists.begin(); it != m_mediaLists.end(); ++it)
+        (*it)->mediaChanged(&listenersToNotify);
+
+    m_evaluator = nullptr;

[Yoav Weiss, 2014/06/26 04:43:19]

Why create and destroy this evaluator on each change? I believe you can keep
this evaluator around, and since it contains a reference to the LocaLFrame, it'd
get updated automagically if the media conditions change.

[esprehn, 2014/06/26 05:01:46]

The mediaType() is stored in the evaluator and won't get updated. It's not clear
why the thing wants both the frame and the mediaType() since the frame is there
the mediaType() comes from... we should fix the Evaulator to not need both and
just always ask the frame and then you can probably keep it around.

+    // FIXME: This should be async! We're running script inside ::layout() or ::updateRenderTree().
+    for (size_t i = 0; i < listenersToNotify.size(); ++i)
+        listenersToNotify[i]->call();
 }
 
 void MediaQueryMatcher::trace(Visitor* visitor)
 {
     visitor->trace(m_document);
-    // We don't support tracing of vectors of OwnPtrs (ie. Vector<OwnPtr<Listener> >).
-    // Since this is a transitional object we are just ifdef'ing it out when oilpan is not enabled.
-#if ENABLE(OILPAN)
-    visitor->trace(m_listeners);
-#endif
+    visitor->trace(m_mediaLists);
 }
 
 }