Remove Page dependency from NetworkStateNotifier

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

 /*
  * Copyright (C) 2006, 2007, 2008, 2010 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Nokia Corporation and/or its subsidiary(-ies)
  *
  * 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
@@ skipping 56 matching lines @@
 #include "core/input/EventHandler.h"
 #include "core/inspector/ConsoleMessage.h"
 #include "core/inspector/InspectorInstrumentation.h"
 #include "core/inspector/InspectorTraceEvents.h"
 #include "core/loader/DocumentLoader.h"
 #include "core/loader/FrameLoaderClient.h"
 #include "core/loader/SinkDocument.h"
 #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"
 #include "core/timing/DOMWindowPerformance.h"
 #include "core/timing/Performance.h"
 #include "platform/EventDispatchForbiddenScope.h"
 #include "platform/WebFrameScheduler.h"
 #include "platform/loader/fetch/ResourceFetcher.h"
 #include "platform/weborigin/SecurityOrigin.h"
 #include "platform/weborigin/Suborigin.h"
@@ skipping 70 matching lines @@
   void dispose() { m_window->removePostMessageTimer(this); }
 
   Member<MessageEvent> m_event;
   Member<LocalDOMWindow> m_window;
   RefPtr<SecurityOrigin> m_targetOrigin;
   std::unique_ptr<SourceLocation> m_location;
   RefPtr<UserGestureToken> m_userGestureToken;
   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,
     FrameLoaderClient::SuddenTerminationDisablerType disablerType) {
   Platform::current()->suddenTerminationChanged(!addedListener);
   if (domWindow->frame() && domWindow->frame()->loader().client())
     domWindow->frame()->loader().client()->suddenTerminationDisablerChanged(
         addedListener, disablerType);
 }
 
@@ skipping 155 matching lines @@
 
 Document* LocalDOMWindow::installNewDocument(const String& mimeType,
                                              const DocumentInit& init,
                                              bool forceXHTML) {
   ASSERT(init.frame() == frame());
 
   clearDocument();
 
   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())
     return m_document;
 
   frame()->script().updateDocument();
   m_document->updateViewportDescription();
 
   if (frame()->page() && frame()->view()) {
     if (ScrollingCoordinator* scrollingCoordinator =
@@ skipping 148 matching lines @@
   m_locationbar = nullptr;
   m_menubar = nullptr;
   m_personalbar = nullptr;
   m_scrollbars = nullptr;
   m_statusbar = nullptr;
   m_toolbar = nullptr;
   m_navigator = nullptr;
   m_media = nullptr;
   m_customElements = nullptr;
   m_applicationCache = nullptr;
+  m_networkStateObserver = nullptr;
 }
 
 void LocalDOMWindow::sendOrientationChangeEvent() {
   ASSERT(RuntimeEnabledFeatures::orientationEventEnabled());
   ASSERT(frame()->isMainFrame());
 
   // Before dispatching the event, build a list of all frames in the page
   // to send the event to, to mitigate side effects from event handlers
   // potentially interfering with others.
   HeapVector<Member<Frame>> frames;
@@ skipping 1098 matching lines @@
   visitor->trace(m_menubar);
   visitor->trace(m_personalbar);
   visitor->trace(m_scrollbars);
   visitor->trace(m_statusbar);
   visitor->trace(m_toolbar);
   visitor->trace(m_navigator);
   visitor->trace(m_media);
   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);
   visitor->trace(m_eventListenerObservers);
   DOMWindow::trace(visitor);
   Supplementable<LocalDOMWindow>::trace(visitor);
 }
 
 DEFINE_TRACE_WRAPPERS(LocalDOMWindow) {
   visitor->traceWrappers(m_customElements);
   DOMWindow::traceWrappers(visitor);
 }
 
 }  // namespace blink