Adds type information to the NetworkStateNotifier. Also allows for registration of observers.

Source/core/page/NetworkStateNotifier.cpp

Index: Source/core/page/NetworkStateNotifier.cpp
diff --git a/Source/core/page/NetworkStateNotifier.cpp b/Source/core/page/NetworkStateNotifier.cpp
index 02cc4615c96408f4abcfc60bcea128654b09758d..1de39f5224beba2addf9055c7e6d8afbd0053ce1 100644
--- a/Source/core/page/NetworkStateNotifier.cpp
+++ b/Source/core/page/NetworkStateNotifier.cpp
@@ -26,14 +26,21 @@
 #include "config.h"
 #include "core/page/NetworkStateNotifier.h"
 
+#include "core/dom/ExecutionContext.h"
 #include "core/page/Page.h"
 #include "wtf/Assertions.h"
+#include "wtf/Functional.h"
 #include "wtf/MainThread.h"
 #include "wtf/StdLibExtras.h"
 #include "wtf/Threading.h"
 
 namespace WebCore {
 
+NetworkStateNotifier::~NetworkStateNotifier()
+{
+    deleteAllValues(m_observers);

[adamk, 2014/05/21 09:21:31]

This won't be needed if you use OwnPtrs as values.

[jkarlin, 2014/05/21 14:23:17]

Done.

+}
+
 NetworkStateNotifier& networkStateNotifier()
 {
     AtomicallyInitializedStatic(NetworkStateNotifier*, networkStateNotifier = new NetworkStateNotifier);
@@ -55,4 +62,106 @@ void NetworkStateNotifier::setOnLine(bool onLine)
     Page::networkStateChanged(onLine);
 }
 
+void NetworkStateNotifier::setWebConnectionType(blink::WebNetworkConnection::ConnectionType type)
+{
+    ASSERT(isMainThread());
+
+    MutexLocker locker(m_mutex);
+    if (m_type == type)
+        return;
+    m_type = type;
+
+    for (ObserverListMap::iterator it = m_observers.begin(); it != m_observers.end(); ++it) {
+        ExecutionContext* context = it->key;
+        context->postTask(bind(&NetworkStateNotifier::notifyObserversOnContext, this, context, type));
+    }
+}
+
+void NetworkStateNotifier::addObserver(NetworkStateObserver* observer, ExecutionContext* context)
+{
+    ASSERT(context->isContextThread());
+    ASSERT(observer);
+
+    MutexLocker locker(m_mutex);
+    ObserverListMap::iterator it = m_observers.find(context);
+    if (it == m_observers.end()) {
+        m_observers.set(context, new ObserverList());
+        it = m_observers.find(context);

[adamk, 2014/05/21 09:21:31]

The idiomatic way to handle this song and dance in a WTF HashMap is:

ObserverListMap::AddResult result = m_observers.add(context, nullptr);
if (result.isNewEntry)
  result.storedValue->value = adoptPtr(new ObserverList);
result.storedValue->value->observers.append(observer);

(which does only a single hash lookup instead of the 3 here; I realize
addObserver is probably not performance sensitive, but being idiomatic is easier
to read for me)

[jkarlin, 2014/05/21 14:23:17]

Done.

+    }
+
+    it->value->observers.append(observer);

[adamk, 2014/05/21 09:21:31]

Maybe add an ASSERT that the observer isn't already in the list.

[jkarlin, 2014/05/21 14:23:17]

Done.

+}
+
+void NetworkStateNotifier::removeObserver(NetworkStateObserver* observer, ExecutionContext* context)
+{
+    ASSERT(context->isContextThread());
+    ASSERT(observer);
+
+    ObserverList* observerList = lockAndFindObserverList(context);
+    if (!observerList)
+        return;
+
+    Vector<NetworkStateObserver*>& observers = observerList->observers;
+    size_t index = observers.find(observer);
+    if (index != kNotFound) {
+        observers[index] = 0;
+        observerList->zeroedObservers.append(index);
+    }
+
+    if (!observerList->iterating && !observerList->zeroedObservers.isEmpty())
+        collectZeroedObservers(context);

[adamk, 2014/05/21 09:21:31]

All this zeroed observers logic is pretty tricky. Is there a way to write
unittests for it?

[jkarlin, 2014/05/21 14:23:17]

Done.

+}
+
+void NetworkStateNotifier::notifyObserversOnContext(ExecutionContext* context, blink::WebNetworkConnection::ConnectionType type)
+{
+    ASSERT(context->isContextThread());
+
+    ObserverList* observerList = lockAndFindObserverList(context);
+
+    // The context could have been removed before the notification task got to run.

[adamk, 2014/05/21 09:21:31]

If this happens then the ASSERT above could crash. In general, I think you have
to first do a lookup in m_observers before accessing context (unless it's passed
in addObserver or removeObserver, naturally).

[jkarlin, 2014/05/21 14:23:17]

Done.

+    if (!observerList)
+        return;
+
+    observerList->iterating = true;
+
+    for (size_t i = 0; i < observerList->observers.size(); ++i) {
+        // Observers removed during iteration are zeroed out, skip them.
+        if (observerList->observers[i])
+            observerList->observers[i]->connectionTypeChange(type);
+    }
+
+    observerList->iterating = false;
+
+    if (!observerList->zeroedObservers.isEmpty())
+        collectZeroedObservers(context);
+}
+
+NetworkStateNotifier::ObserverList* NetworkStateNotifier::lockAndFindObserverList(ExecutionContext* context)
+{
+    MutexLocker locker(m_mutex);
+    ObserverListMap::iterator it = m_observers.find(context);
+    return it == m_observers.end() ? 0 : it->value;
 }
+
+void NetworkStateNotifier::collectZeroedObservers(ExecutionContext* context)

[adamk, 2014/05/21 09:21:31]

Please add a context thread assertion here.

[jkarlin, 2014/05/21 14:23:17]

Done.

+{
+    // If any observers were removed during the iteration they will have
+    // 0 values, clean them up.
+    ObserverList* list = lockAndFindObserverList(context);

[adamk, 2014/05/21 09:21:31]

Maybe the callers should just pass in the list?

[jkarlin, 2014/05/21 14:23:17]

Done.

+    ASSERT(list);

[adamk, 2014/05/21 09:21:31]

I think you also want to ASSERT(!list->iterating)

[jkarlin, 2014/05/21 14:23:17]

Done.

+
+    for (size_t i = 0; i < list->zeroedObservers.size(); ++i)
+        list->observers.remove(list->zeroedObservers[i]);
+
+    list->zeroedObservers.clear();
+
+    {
+        MutexLocker locker(m_mutex);
+        if (list->observers.isEmpty()) {
+            m_observers.remove(context);
+            delete list;

[adamk, 2014/05/21 09:21:31]

This delete can also go away if you use OwnPtrs as values.

[jkarlin, 2014/05/21 14:23:17]

Done.

+        }
+    }
+}
+
+} // namespace WebCore