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 0f3ff9ccfa3ac425d6f5a9549c2e387825da9034..898b6c2f3bee02629de0184a546b59f2c9581a50 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
+++ b/third_party/WebKit/Source/bindings/core/v8/V8PerIsolateData.h
@@ -26,10 +26,11 @@
 #ifndef V8PerIsolateData_h
 #define V8PerIsolateData_h
 
+#include <v8.h>

[Yuki, 2017/02/15 10:00:40]

Please use the following instead.

#include "v8/include/v8.h"

We're going to treat v8.h as a user-defined header.

+#include <memory>

[Yuki, 2017/02/15 10:00:40]

Chromium's style guide recommends to put an empty line just after system
headers.  Ditto for all other files.

[adithyas, 2017/02/15 18:12:12]

Running git cl format reordered the headers like this, so maybe we need to
change clang-format to add an empty line.

 #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"
@@ -38,16 +39,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 +87,14 @@ class CORE_EXPORT V8PerIsolateData {
     const bool m_originalUseCounterDisabled;
   };
 
+  class CORE_EXPORT Data {
+    WTF_MAKE_NONCOPYABLE(Data);

[Yuki, 2017/02/15 10:00:40]

Why do we want Data to be non-copyable?
I know subclasses are actually non-copyable, but I see no reason to make Data
non-copyable from the point of view of V8PerIsolateData.

[adithyas, 2017/02/15 18:12:12]

Yeah it doesn't need to be non-copyable, I've changed it now.

+
+   public:
+    explicit Data() {}

[jbroman, 2017/02/14 18:10:48]

nit: if you do keep this interface, you don't need the constructor (and if you
did declare one, it doesn't need to be explicit if it takes no arguments).

[adithyas, 2017/02/15 18:12:12]

Removed.

+    virtual ~Data() {}
+  };
+
   static v8::Isolate* initialize(WebTaskRunner*);
 
   static V8PerIsolateData* from(v8::Isolate* isolate) {
@@ -122,8 +127,18 @@ class CORE_EXPORT V8PerIsolateData {
   bool isReportingException() const { return m_isReportingException; }
   void setReportingException(bool value) { m_isReportingException = value; }
 
-  V8HiddenValue* hiddenValue() { return m_hiddenValue.get(); }
-  V8PrivateProperty* privateProperty() { return m_privateProperty.get(); }
+  Data* hiddenValue() { return m_hiddenValue.get(); }
+  void setHiddenValue(Data* hiddenValue) { m_hiddenValue.reset(hiddenValue); }
+
+  Data* privateProperty() { return m_privateProperty.get(); }
+  void setPrivateProperty(Data* privateProperty) {
+    m_privateProperty.reset(privateProperty);
+  }
+
+  Data* threadDebugger() { return m_threadDebugger.get(); }
+  void setThreadDebugger(Data* threadDebugger) {

[jbroman, 2017/02/14 18:10:48]

Please pass ownership by std::unique_ptr, not raw pointer.

[adithyas, 2017/02/15 18:12:12]

Done.

+    m_threadDebugger.reset(threadDebugger);
+  }
 
   // Accessors to the cache of interface templates.
   v8::Local<v8::FunctionTemplate> findInterfaceTemplate(const DOMWrapperWorld&,
@@ -157,9 +172,6 @@ class CORE_EXPORT V8PerIsolateData {
   void runEndOfScopeTasks();
   void clearEndOfScopeTasks();
 
-  void setThreadDebugger(std::unique_ptr<ThreadDebugger>);
-  ThreadDebugger* threadDebugger();
-
   using ActiveScriptWrappableSet =
       HeapHashSet<WeakMember<ActiveScriptWrappableBase>>;
   void addActiveScriptWrappable(ActiveScriptWrappableBase*);
@@ -230,9 +242,11 @@ class CORE_EXPORT V8PerIsolateData {
   V8FunctionTemplateMap m_operationTemplateMapForMainWorld;
   V8FunctionTemplateMap m_operationTemplateMapForNonMainWorld;
 
+  std::unique_ptr<Data> m_hiddenValue;

[jbroman, 2017/02/14 18:10:48]

V8HiddenValue is going away, and V8PrivateProperty seems straightforward to
generalize enough to move to platform/. I'd prefer not to abstract them just for
the sake of having more than one thing using Data.

[haraken, 2017/02/15 00:06:01]

Yeah, agreed.

[adithyas, 2017/02/15 18:12:12]

Okay, I reverted the changes to V8PrivateProperty and V8HiddenValue.

+  std::unique_ptr<Data> m_privateProperty;
+  std::unique_ptr<Data> m_threadDebugger;

[jbroman, 2017/02/14 18:10:48]

I'm of two minds on this.

On the short-term-fix side, we could (ab)use v8_inspector::V8InspectorClient,
which has a virtual destructor, which would at least give some type information
about what we expect to here. On the other hand, we don't actually call any of
its other methods.

On the other hand, I think my longer term preference (at least for those things
that aren't hot) is a generic map that erases the information about this. In
Blink we'd ordinarily use Supplementable for this (but that's now restricted to
GC objects), or in Chromium they'd use base::SupportsUserData. Since this isn't
hot, that seems okay to me (though it might be a little overkill for now).

[haraken, 2017/02/15 00:06:01]

I don't have any strong opinion. The current Data pattern looks fine to me. We
use the Data pattern in some public APIs to let Blink own an thing in Chromium.

[jbroman, 2017/02/15 02:25:23]

Alright, okay to leave m_threadDebugger as-is (modulo the comment about using
std::unique_ptr<Data> in the setter rather than Data*).

[adithyas, 2017/02/15 18:12:12]

Done!

+
   std::unique_ptr<StringCache> m_stringCache;
-  std::unique_ptr<V8HiddenValue> m_hiddenValue;
-  std::unique_ptr<V8PrivateProperty> m_privateProperty;
   ScopedPersistent<v8::Value> m_liveRoot;
   RefPtr<ScriptState> m_scriptRegexpScriptState;
 
@@ -246,7 +260,6 @@ class CORE_EXPORT V8PerIsolateData {
   bool m_isReportingException;
 
   Vector<std::unique_ptr<EndOfScopeTask>> m_endOfScopeTasks;
-  std::unique_ptr<ThreadDebugger> m_threadDebugger;
 
   Persistent<ActiveScriptWrappableSet> m_activeScriptWrappables;
   std::unique_ptr<ScriptWrappableVisitor> m_scriptWrappableVisitor;