Remove Page dependency from NetworkStateNotifier

third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp

Index: third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
diff --git a/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp b/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
index 4fda1a2b6a4a654a403b494de1c8245c100eab0b..d01122f28e752f35e6fcc756772159b2047ab802 100644
--- a/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
+++ b/third_party/WebKit/Source/core/frame/LocalDOMWindow.cpp
@@ -74,6 +74,7 @@
 #include "core/loader/appcache/ApplicationCache.h"
 #include "core/page/ChromeClient.h"
 #include "core/page/CreateWindow.h"
+#include "core/page/NetworkStateNotifier.h"
 #include "core/page/Page.h"
 #include "core/page/WindowFeatures.h"
 #include "core/page/scrolling/ScrollingCoordinator.h"
@@ -164,6 +165,41 @@ class PostMessageTimer final
   bool m_disposalAllowed;
 };
 
+class LocalDOMWindow::NetworkStateObserver final
+    : public GarbageCollected<LocalDOMWindow::NetworkStateObserver>,
+      public NetworkStateNotifier::NetworkStateObserver,
+      public ContextLifecycleObserver {
+  USING_GARBAGE_COLLECTED_MIXIN(LocalDOMWindow::NetworkStateObserver);
+
+ public:
+  NetworkStateObserver(LocalDOMWindow& window)

[jkarlin, 2017/02/22 18:38:08]

I don't have much blink experience. Should this be a pointer or a reference? It
doesn't seem like an out-param to me so perhaps a pointer would be better?

[kinuko, 2017/02/23 01:15:35]

Yeah I know how you feel, in my understanding we prefer reference when we're
sure the param is non-null in Blink.  (I can't find it quickly but we had a
discussion thread on blink-dev sometime ago)

(Btw I moved this class into Document in the newer PS, though the same
discussion applies)

+      : ContextLifecycleObserver(window.document()), m_window(&window) {
+    networkStateNotifier().addOnLineObserver(
+        this,
+        TaskRunnerHelper::get(TaskType::Networking, window.document()).get());
+  }
+
+  void onLineStateChange(bool onLine) override {
+    AtomicString eventName =
+        onLine ? EventTypeNames::online : EventTypeNames::offline;
+    m_window->dispatchEvent(Event::create(eventName));
+    InspectorInstrumentation::networkStateChanged(m_window->frame(), onLine);
+  }
+
+  void contextDestroyed(ExecutionContext* context) override {
+    networkStateNotifier().removeOnLineObserver(

[jkarlin, 2017/02/22 18:38:08]

I'm not familiar with this code. Are we guaranteed that contextDestroyed will be
called before destruction of this object? If not, should we add a destructor
which calls removeOnLineObserver?

[kinuko, 2017/02/23 01:15:35]

Yes, it's guaranteed.  (This is GC'ed class, adding cleanup in dtor is not
really safe)

+        this, TaskRunnerHelper::get(TaskType::Networking, context).get());
+  }
+
+  DEFINE_INLINE_VIRTUAL_TRACE() {
+    visitor->trace(m_window);
+    ContextLifecycleObserver::trace(visitor);
+  }
+
+ private:
+  Member<LocalDOMWindow> m_window;
+};
+
 static void updateSuddenTerminationStatus(
     LocalDOMWindow* domWindow,
     bool addedListener,
@@ -339,6 +375,7 @@ Document* LocalDOMWindow::installNewDocument(const String& mimeType,
 
   m_document = createDocument(mimeType, init, forceXHTML);
   m_eventQueue = DOMWindowEventQueue::create(m_document.get());
+  m_networkStateObserver = new NetworkStateObserver(*this);

[dcheng, 2017/02/22 08:05:13]

The main question I have about this CL is why NetworkStateObserver lives on
LocalDOMWindow if it is a ContextLifecycleObserver. Should it be a member of
Document instead?

[kinuko, 2017/02/22 08:38:27]

Done.

   m_document->initialize();
 
   if (!frame())
@@ -507,6 +544,7 @@ void LocalDOMWindow::reset() {
   m_media = nullptr;
   m_customElements = nullptr;
   m_applicationCache = nullptr;
+  m_networkStateObserver = nullptr;
 }
 
 void LocalDOMWindow::sendOrientationChangeEvent() {
@@ -1625,6 +1663,7 @@ DEFINE_TRACE(LocalDOMWindow) {
   visitor->trace(m_customElements);
   visitor->trace(m_external);
   visitor->trace(m_applicationCache);
+  visitor->trace(m_networkStateObserver);
   visitor->trace(m_eventQueue);
   visitor->trace(m_postMessageTimers);
   visitor->trace(m_visualViewport);