bindings: Simplifies WindowProxyManager and its relation to Frame.

third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp

Index: third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp
diff --git a/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp b/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp
index a96d286f31687deeaf88dc932298b25d0dcbc2f4..8097cdb8d78b1b4d6a63a04a8e1eecc41eb9f76c 100644
--- a/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp
+++ b/third_party/WebKit/Source/bindings/core/v8/WindowProxyManager.cpp
@@ -8,81 +8,86 @@
 
 namespace blink {
 
-namespace {
-
-WindowProxy* createWindowProxyForFrame(v8::Isolate* isolate,
-                                       Frame& frame,
-
-                                       RefPtr<DOMWrapperWorld> world) {
-  if (frame.isLocalFrame()) {
-    return LocalWindowProxy::create(isolate, toLocalFrame(frame),
-                                    std::move(world));
-  }
-  return RemoteWindowProxy::create(isolate, toRemoteFrame(frame),
-                                   std::move(world));
-}
-}
-
-DEFINE_TRACE(WindowProxyManagerBase) {
+DEFINE_TRACE(WindowProxyManager) {
   visitor->trace(m_frame);
   visitor->trace(m_windowProxy);
   visitor->trace(m_isolatedWorlds);
 }
 
-WindowProxy* WindowProxyManagerBase::windowProxy(DOMWrapperWorld& world) {
-  WindowProxy* windowProxy = nullptr;
-  if (world.isMainWorld()) {
-    windowProxy = m_windowProxy.get();
-  } else {
-    IsolatedWorldMap::iterator iter = m_isolatedWorlds.find(world.worldId());
-    if (iter != m_isolatedWorlds.end()) {
-      windowProxy = iter->value.get();
-    } else {
-      windowProxy = createWindowProxyForFrame(m_isolate, *m_frame, &world);
-      m_isolatedWorlds.set(world.worldId(), windowProxy);
-    }
-  }
-  return windowProxy;
-}
-
-void WindowProxyManagerBase::clearForClose() {
+void WindowProxyManager::clearForClose() {
   m_windowProxy->clearForClose();
   for (auto& entry : m_isolatedWorlds)
     entry.value->clearForClose();
 }
 
-void WindowProxyManagerBase::clearForNavigation() {
+void WindowProxyManager::clearForNavigation() {
   m_windowProxy->clearForNavigation();
   for (auto& entry : m_isolatedWorlds)
     entry.value->clearForNavigation();
 }
 
-void WindowProxyManagerBase::releaseGlobals(GlobalsVector& globals) {
+void WindowProxyManager::releaseGlobals(GlobalsVector& globals) {
   globals.reserveInitialCapacity(1 + m_isolatedWorlds.size());
   globals.emplace_back(&m_windowProxy->world(), m_windowProxy->releaseGlobal());
   for (auto& entry : m_isolatedWorlds) {
-    globals.emplace_back(&entry.value->world(),
-                         windowProxy(entry.value->world())->releaseGlobal());
+    globals.emplace_back(
+        &entry.value->world(),
+        windowProxyMaybeUninitialized(entry.value->world())->releaseGlobal());
   }
 }
 
-void WindowProxyManagerBase::setGlobals(const GlobalsVector& globals) {
+void WindowProxyManager::setGlobals(const GlobalsVector& globals) {
   for (const auto& entry : globals)
-    windowProxy(*entry.first)->setGlobal(entry.second);
+    windowProxyMaybeUninitialized(*entry.first)->setGlobal(entry.second);
 }
 
-WindowProxyManagerBase::WindowProxyManagerBase(Frame& frame)
+WindowProxyManager::WindowProxyManager(Frame& frame, FrameType frameType)
     : m_isolate(v8::Isolate::GetCurrent()),
       m_frame(&frame),
-      m_windowProxy(createWindowProxyForFrame(m_isolate,
-                                              frame,
-                                              &DOMWrapperWorld::mainWorld())) {}
+      m_frameType(frameType),
+      m_windowProxy(createWindowProxy(DOMWrapperWorld::mainWorld())) {}
+
+WindowProxy* WindowProxyManager::createWindowProxy(DOMWrapperWorld& world) {
+  switch (m_frameType) {
+    case FrameType::Local:
+      // Directly use static_cast instead of toLocalFrame because
+      // WindowProxyManager gets instantiated during a construction of
+      // LocalFrame and at that time virtual member functions are not yet
+      // available (we cannot use LocalFrame::isLocalFrame).  Ditto for
+      // RemoteFrame.

[dcheng, 2017/03/16 08:43:16]

This makes me a little nervous: I remember hitting similar issues in the past
(see
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/Page...),
and I'm worried that LocalWindowProxy/RemoteWindowProxy might try to call
virtual functions on Frame in the future.

One alternative, which is a little less nice, is to make m_windowProxyManager a
protected member, and have the subclasses (which are final) set it directly in
the body of the constructor, when the vtables are completely initialized. WDYT?

[Yuki, 2017/03/16 12:41:21]

I personally prefer making Frame::m_windowProxyManager const, also making
WindowProxyManager::m_frame const.  When I see "const" there, I'm very happy
knowing "oh, it's guaranteed to be the same for the whole lifetime.  no need to
worry about any change nor lifetime cycle."

So, if we're going to set it in the body of the constructor, I'd like to use
const_cast, but I'm not really happy with const_cast.

I agree that this is a little hacky and not super nice.  I'm on the fence about
what to do, but I'd slightly prefer this way (static_cast with m_frameType) to
const_cast.

+      return LocalWindowProxy::create(
+          m_isolate, *static_cast<LocalFrame*>(m_frame.get()), &world);
+    case FrameType::Remote:
+      return RemoteWindowProxy::create(
+          m_isolate, *static_cast<RemoteFrame*>(m_frame.get()), &world);
+  }
+  NOTREACHED();
+  return nullptr;
+}
+
+WindowProxy* WindowProxyManager::windowProxyMaybeUninitialized(
+    DOMWrapperWorld& world) {
+  WindowProxy* windowProxy = nullptr;
+  if (world.isMainWorld()) {
+    windowProxy = m_windowProxy.get();
+  } else {
+    IsolatedWorldMap::iterator iter = m_isolatedWorlds.find(world.worldId());
+    if (iter != m_isolatedWorlds.end()) {
+      windowProxy = iter->value.get();
+    } else {
+      windowProxy = createWindowProxy(world);
+      m_isolatedWorlds.set(world.worldId(), windowProxy);
+    }
+  }
+  return windowProxy;
+}
 
 void LocalWindowProxyManager::updateSecurityOrigin(
     SecurityOrigin* securityOrigin) {
-  static_cast<LocalWindowProxy*>(mainWorldProxy())
+  static_cast<LocalWindowProxy*>(m_windowProxy.get())
       ->updateSecurityOrigin(securityOrigin);
-  for (auto& entry : isolatedWorlds()) {
+
+  for (auto& entry : m_isolatedWorlds) {
     auto* isolatedWindowProxy =
         static_cast<LocalWindowProxy*>(entry.value.get());
     SecurityOrigin* isolatedSecurityOrigin =