Abstract out ThreadDebugger from V8PerIsolateData

third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  *
  * 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
  *    notice, this list of conditions and the following disclaimer in the
  *    documentation and/or other materials provided with the distribution.
  *
  * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
  * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
  * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
  * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
  * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
  * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
  * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
  * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
  * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
  * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
  * THE POSSIBILITY OF SUCH DAMAGE.
  */
 
 #ifndef V8PerIsolateData_h
 #define V8PerIsolateData_h
 
+#include <v8.h>
+#include <memory>
 #include "bindings/core/v8/ScopedPersistent.h"
 #include "bindings/core/v8/ScriptState.h"
 #include "bindings/core/v8/ScriptWrappableVisitor.h"
 #include "bindings/core/v8/V8HiddenValue.h"
 #include "bindings/core/v8/WrapperTypeInfo.h"
 #include "core/CoreExport.h"
 #include "gin/public/isolate_holder.h"
 #include "gin/public/v8_idle_task_runner.h"
 #include "platform/heap/Handle.h"
 #include "wtf/HashMap.h"
 #include "wtf/Noncopyable.h"
 #include "wtf/Vector.h"
-#include <memory>
-#include <v8.h>
 
 namespace blink {
 
 class ActiveScriptWrappableBase;
 class DOMDataStore;
 class StringCache;
-class ThreadDebugger;
 class V8PrivateProperty;
 class WebTaskRunner;
 struct WrapperTypeInfo;
 
 typedef WTF::Vector<DOMDataStore*> DOMDataStoreList;
 
 class CORE_EXPORT V8PerIsolateData {
   USING_FAST_MALLOC(V8PerIsolateData);
   WTF_MAKE_NONCOPYABLE(V8PerIsolateData);
 
@@ skipping 23 matching lines @@
     }
     ~UseCounterDisabledScope() {
       m_perIsolateData->m_useCounterDisabled = m_originalUseCounterDisabled;
     }
 
    private:
     V8PerIsolateData* m_perIsolateData;
     const bool m_originalUseCounterDisabled;
   };
 
+  // Use this class to abstract away types of members that are pointers to core/
+  // objects, which are simply owned and released by V8PerIsolateData (see
+  // m_threadDebugger for an example).
+  class CORE_EXPORT Data {
+   public:
+    virtual ~Data() = default;
+  };
+
   static v8::Isolate* initialize(WebTaskRunner*);
 
   static V8PerIsolateData* from(v8::Isolate* isolate) {
     ASSERT(isolate);
     ASSERT(isolate->GetData(gin::kEmbedderBlink));
     return static_cast<V8PerIsolateData*>(
         isolate->GetData(gin::kEmbedderBlink));
   }
 
   static void willBeDestroyed(v8::Isolate*);
@@ skipping 49 matching lines @@
   void clearScriptRegexpContext();
 
   // EndOfScopeTasks are run when control is returning
   // to C++ from script, after executing a script task (e.g. callback,
   // event) or microtasks (e.g. promise). This is explicitly needed for
   // Indexed DB transactions per spec, but should in general be avoided.
   void addEndOfScopeTask(std::unique_ptr<EndOfScopeTask>);
   void runEndOfScopeTasks();
   void clearEndOfScopeTasks();
 
-  void setThreadDebugger(std::unique_ptr<ThreadDebugger>);
-  ThreadDebugger* threadDebugger();
+  void setThreadDebugger(std::unique_ptr<Data>);
+  Data* threadDebugger();

[dgozman, 2017/02/18 06:11:36]

Sorry for coming late, but I'm curios what are we trying to hide here? The
method is called |threadDebugger|, but returns some abstract data, which sounds
like we are tricking ourselves.
If we introduce layering, can we have an actual abstraction so that anyone can
create an object which lifetime is coupled with isolate? I think current
solution is fighting with compiler/DEPS rules, but not the actual semantics.

[adithyas, 2017/02/21 15:43:04]

You are correct, the current solution simply removes a dependency to core/
rather than actually abstracting away the object. Abstracting away these objects
with a map (like what Supplementable does) would be a cleaner solution. But, I
don't know if this is necessary considering there is only one object
(threadDebugger) that will be abstracted away by this. Members like
privateProperty and hiddenValue could be abstracted away too, but they are
accessed very frequently, so adding a map access to retrieve them might result
in a performance hit.

There was some bikeshedding earlier in this CL and the consensus was that the
Data abstraction was okay just for this case.

 
   using ActiveScriptWrappableSet =
       HeapHashSet<WeakMember<ActiveScriptWrappableBase>>;
   void addActiveScriptWrappable(ActiveScriptWrappableBase*);
   const ActiveScriptWrappableSet* activeScriptWrappables() const {
     return m_activeScriptWrappables.get();
   }
 
   class TemporaryScriptWrappableVisitorScope {
     WTF_MAKE_NONCOPYABLE(TemporaryScriptWrappableVisitorScope);
@@ skipping 65 matching lines @@
   bool m_constructorMode;
   friend class ConstructorMode;
 
   bool m_useCounterDisabled;
   friend class UseCounterDisabledScope;
 
   bool m_isHandlingRecursionLevelError;
   bool m_isReportingException;
 
   Vector<std::unique_ptr<EndOfScopeTask>> m_endOfScopeTasks;
-  std::unique_ptr<ThreadDebugger> m_threadDebugger;
+  std::unique_ptr<Data> m_threadDebugger;
 
   Persistent<ActiveScriptWrappableSet> m_activeScriptWrappables;
   std::unique_ptr<ScriptWrappableVisitor> m_scriptWrappableVisitor;
 };
 
 }  // namespace blink
 
 #endif  // V8PerIsolateData_h