bindings: Simplifies WindowProxyManager and its relation to Frame.

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "bindings/core/v8/WindowProxyManager.h"
 
 #include "bindings/core/v8/DOMWrapperWorld.h"
 
 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) {
+void WindowProxyManager::clearForClose() {
+  m_windowProxy->clearForClose();
+  for (auto& entry : m_isolatedWorlds)
+    entry.value->clearForClose();
+}
+
+void WindowProxyManager::clearForNavigation() {
+  m_windowProxy->clearForNavigation();
+  for (auto& entry : m_isolatedWorlds)
+    entry.value->clearForNavigation();
+}
+
+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(),
+        windowProxyMaybeUninitialized(entry.value->world())->releaseGlobal());
+  }
+}
+
+void WindowProxyManager::setGlobals(const GlobalsVector& globals) {
+  for (const auto& entry : globals)
+    windowProxyMaybeUninitialized(*entry.first)->setGlobal(entry.second);
+}
+
+WindowProxyManager::WindowProxyManager(Frame& frame, FrameType frameType)
+    : m_isolate(v8::Isolate::GetCurrent()),
+      m_frame(&frame),
+      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 = createWindowProxyForFrame(m_isolate, *m_frame, &world);
+      windowProxy = createWindowProxy(world);
       m_isolatedWorlds.set(world.worldId(), windowProxy);
     }
   }
   return windowProxy;
 }
 
-void WindowProxyManagerBase::clearForClose() {
-  m_windowProxy->clearForClose();
-  for (auto& entry : m_isolatedWorlds)
-    entry.value->clearForClose();
-}
-
-void WindowProxyManagerBase::clearForNavigation() {
-  m_windowProxy->clearForNavigation();
-  for (auto& entry : m_isolatedWorlds)
-    entry.value->clearForNavigation();
-}
-
-void WindowProxyManagerBase::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());
-  }
-}
-
-void WindowProxyManagerBase::setGlobals(const GlobalsVector& globals) {
-  for (const auto& entry : globals)
-    windowProxy(*entry.first)->setGlobal(entry.second);
-}
-
-WindowProxyManagerBase::WindowProxyManagerBase(Frame& frame)
-    : m_isolate(v8::Isolate::GetCurrent()),
-      m_frame(&frame),
-      m_windowProxy(createWindowProxyForFrame(m_isolate,
-                                              frame,
-                                              &DOMWrapperWorld::mainWorld())) {}
-
 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 =
         isolatedWindowProxy->world().isolatedWorldSecurityOrigin();
     isolatedWindowProxy->updateSecurityOrigin(isolatedSecurityOrigin);
   }
 }
 
 }  // namespace blink