Abstract out ThreadDebugger from V8PerIsolateData

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

Index: third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h b/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
index 5e933530022c1135788a84e4aaea4045c817d768..305bab8632d8c10ff2417c8c4d30df725a0ce13c 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
+++ b/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
@@ -26,6 +26,8 @@
 #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"
@@ -38,15 +40,12 @@
 #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;
@@ -90,6 +89,14 @@ class CORE_EXPORT V8PerIsolateData {
     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) {
@@ -159,8 +166,8 @@ class CORE_EXPORT V8PerIsolateData {
   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>>;
@@ -246,7 +253,7 @@ class CORE_EXPORT V8PerIsolateData {
   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;