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>

[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"
 #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);
 
  public:
@@ skipping 22 matching lines @@
     }
     ~UseCounterDisabledScope() {
       m_perIsolateData->m_useCounterDisabled = m_originalUseCounterDisabled;
     }
 
    private:
     V8PerIsolateData* m_perIsolateData;
     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) {
     ASSERT(isolate);
     ASSERT(isolate->GetData(gin::kEmbedderBlink));
     return static_cast<V8PerIsolateData*>(
         isolate->GetData(gin::kEmbedderBlink));
   }
 
   static void willBeDestroyed(v8::Isolate*);
@@ skipping 12 matching lines @@
   bool isHandlingRecursionLevelError() const {
     return m_isHandlingRecursionLevelError;
   }
   void setIsHandlingRecursionLevelError(bool value) {
     m_isHandlingRecursionLevelError = value;
   }
 
   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&,
                                                         const void* key);
   void setInterfaceTemplate(const DOMWrapperWorld&,
                             const void* key,
                             v8::Local<v8::FunctionTemplate>);
 
   // Accessor to the cache of cross-origin accessible operation's templates.
   // Created templates get automatically cached.
@@ skipping 13 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();
-
   using ActiveScriptWrappableSet =
       HeapHashSet<WeakMember<ActiveScriptWrappableBase>>;
   void addActiveScriptWrappable(ActiveScriptWrappableBase*);
   const ActiveScriptWrappableSet* activeScriptWrappables() const {
     return m_activeScriptWrappables.get();
   }
 
   class TemporaryScriptWrappableVisitorScope {
     WTF_MAKE_NONCOPYABLE(TemporaryScriptWrappableVisitorScope);
     STACK_ALLOCATED();
@@ skipping 50 matching lines @@
 
   // m_interfaceTemplateMapFor{,Non}MainWorld holds function templates for
   // the inerface objects.
   V8FunctionTemplateMap m_interfaceTemplateMapForMainWorld;
   V8FunctionTemplateMap m_interfaceTemplateMapForNonMainWorld;
   // m_operationTemplateMapFor{,Non}MainWorld holds function templates for
   // the cross-origin accessible DOM operations.
   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;
 
   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;
 
   Persistent<ActiveScriptWrappableSet> m_activeScriptWrappables;
   std::unique_ptr<ScriptWrappableVisitor> m_scriptWrappableVisitor;
 };
 
 }  // namespace blink
 
 #endif  // V8PerIsolateData_h