[Bindings] Simplify V8PrivateProperty::Symbol methods

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

 // Copyright 2016 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.
 
 #ifndef V8PrivateProperty_h
 #define V8PrivateProperty_h
 
 #include <memory>
 
 #include "bindings/core/v8/ScriptPromiseProperties.h"
 #include "bindings/core/v8/V8BindingMacros.h"
 #include "bindings/core/v8/V8PerIsolateData.h"
 #include "core/CoreExport.h"
 #include "v8/include/v8.h"
 #include "wtf/Allocator.h"
 #include "wtf/PtrUtil.h"
 
 namespace blink {
 
-class ScriptState;
 class ScriptWrappable;
 
 // Apply |X| for each pair of (InterfaceName, PrivateKeyName).
 #define V8_PRIVATE_PROPERTY_FOR_EACH(X)               \
   X(CustomEvent, Detail)                              \
   X(CustomElement, AdoptedCallback)                   \
   X(CustomElement, AttributeChangedCallback)          \
   X(CustomElement, ConnectedCallback)                 \
   X(CustomElement, DisconnectedCallback)              \
   X(CustomElement, Document)                          \
@@ skipping 50 matching lines @@
  public:
   // Provides fast access to V8's private properties.
   //
   // Retrieving/creating a global private symbol from a string is very
   // expensive compared to get or set a private property.  This class
   // provides a way to cache a private symbol and re-use it.
   class CORE_EXPORT Symbol {
     STACK_ALLOCATED();
 
    public:
-    bool hasValue(v8::Local<v8::Context> context,
-                  v8::Local<v8::Object> object) const {
-      return v8CallBoolean(object->HasPrivate(context, m_privateSymbol));
+    bool hasValue(v8::Local<v8::Object> object) const {
+      return object->HasPrivate(context(), m_privateSymbol).ToChecked();
     }
 
     // Returns the value of the private property if set, or undefined.
-    v8::Local<v8::Value> getOrUndefined(v8::Local<v8::Context> context,
-                                        v8::Local<v8::Object> object) const {
-      return object->GetPrivate(context, m_privateSymbol).ToLocalChecked();
+    v8::Local<v8::Value> getOrUndefined(v8::Local<v8::Object> object) const {

[haraken, 2017/03/30 13:53:34]

Not related to this CL, I might want to remove this method and unify all call
sites to getOrEmpty(). If the call site wants to get undefined when the private
property is not set, the call site can explicitly use hasValue() or
getOrEmpty().

[Yuki, 2017/03/30 14:18:14]

I'd like to do the opposite.  As V8 never returns the empty handle except for
cases that an exception is thrown (and the empty Maybe is returned), as haraken@
made ToV8() never return the empty handle, I don't want to return the empty
handle.

So, if possible, I'd like to remove getOrEmpty(), not getOrUndefined().  Also
note that getOrEmpty() is slower than getOrUndefined().

[haraken, 2017/03/30 14:21:37]

I'm fine as long as the two methods are unified into one :)

+      return object->GetPrivate(context(), m_privateSymbol).ToLocalChecked();
     }
 
     // Returns the value of the private property if set, or an empty handle.
-    v8::Local<v8::Value> get(v8::Local<v8::Context> context,
-                             v8::Local<v8::Object> object) const {
-      if (!v8CallBoolean(object->HasPrivate(context, m_privateSymbol)))
-        return v8::Local<v8::Value>();
-      v8::Local<v8::Value> value;
-      if (v8Call(object->GetPrivate(context, m_privateSymbol), value))
-        return value;
+    v8::Local<v8::Value> getOrEmpty(v8::Local<v8::Object> object) const {
+      if (hasValue(object))
+        return getOrUndefined(object);
       return v8::Local<v8::Value>();
     }
 
-    bool set(v8::Local<v8::Context> context,
-             v8::Local<v8::Object> object,
-             v8::Local<v8::Value> value) const {
-      return v8CallBoolean(object->SetPrivate(context, m_privateSymbol, value));
+    bool set(v8::Local<v8::Object> object, v8::Local<v8::Value> value) const {
+      return object->SetPrivate(context(), m_privateSymbol, value).ToChecked();
     }
 
-    v8::Local<v8::Private> getPrivate() { return m_privateSymbol; }
+    v8::Local<v8::Private> getPrivate() const { return m_privateSymbol; }
 
    private:
     friend class V8PrivateProperty;
     // The following classes are exceptionally allowed to call to
     // getFromMainWorld.
     friend class V8CustomEvent;
     friend class V8ServiceWorkerMessageEventInternal;
 
-    explicit Symbol(v8::Local<v8::Private> privateSymbol)
-        : m_privateSymbol(privateSymbol) {}
+    Symbol(v8::Isolate* isolate, v8::Local<v8::Private> privateSymbol)
+        : m_privateSymbol(privateSymbol), m_isolate(isolate) {}
+
+    v8::Local<v8::Context> context() const {

[haraken, 2017/03/30 13:53:34]

I'd prefer removing this method to prevent someone from using context() without
understanding what context she is in.

[Yuki, 2017/03/30 14:18:14]

This method is introduced in order to prevent someone from using the wrong
context.  Note that this method is private:, so client code cannot use this
method.

v8::Private::{Get,Set}Private never create a new object and the context
passed-in is only used for simple get / set operations otherwise for throwing an
exception.  In either case, the context we should use is the current context. 
We don't want call sites to pass in any other context.

This CL guarantees that the current context (i.e. the right context) is always
used.

As we think that (some of) V8 APIs do not need to take a v8::Context as the
first argument, this class does not need, too.  As we think that
v8::Private::{Get,Set}Private do not need to take a v8::Context, should it be
okay to always pass in the current context.

[haraken, 2017/03/30 14:21:37]

OK, then I'd like to rename it to currentContext() to clarify what context it
returns though.

[Yuki, 2017/03/30 15:02:43]

context() is a short and good name in this case, I think.  I'd rather propose to
add a comment why we always use the current context.

The important point is that context() returns the *right* context, it's not so
important that context() returns the current context.  If the call sites of this
helper function need to be aware of that it's *current* context, this function
helps nothing at all.  It's important that the call sites can use this helper
function without caring much about what the exact context is.

But, the explanation in a comment about why it's always the current context is
good for code readers.

[peria, 2017/03/30 15:53:24]

Thank you Shiino-san for the explanation.
I'll put a comment for this method.

+      return m_isolate->GetCurrentContext();
+    }
 
     // Only friend classes are allowed to use this API.
-    v8::Local<v8::Value> getFromMainWorld(ScriptState*, ScriptWrappable*);
+    v8::Local<v8::Value> getFromMainWorld(ScriptWrappable*);
 
     v8::Local<v8::Private> m_privateSymbol;
+    v8::Isolate* m_isolate;
   };
 
   static std::unique_ptr<V8PrivateProperty> create() {
     return WTF::wrapUnique(new V8PrivateProperty());
   }
 
-#define V8_PRIVATE_PROPERTY_DEFINE_GETTER(InterfaceName, KeyName)              \
-  static Symbol V8_PRIVATE_PROPERTY_GETTER_NAME(InterfaceName, KeyName)(       \
-      v8::Isolate * isolate) /* NOLINT(readability/naming/underscores) */      \
-  {                                                                            \
-    V8PrivateProperty* privateProp =                                           \
-        V8PerIsolateData::from(isolate)->privateProperty();                    \
-    if (UNLIKELY(privateProp                                                   \
-                     ->V8_PRIVATE_PROPERTY_MEMBER_NAME(InterfaceName, KeyName) \
-                     .IsEmpty())) {                                            \
-      privateProp->V8_PRIVATE_PROPERTY_MEMBER_NAME(InterfaceName, KeyName)     \
-          .Set(                                                                \
-              isolate,                                                         \
-              createV8Private(                                                 \
-                  isolate,                                                     \
-                  V8_PRIVATE_PROPERTY_SYMBOL_STRING(InterfaceName, KeyName),   \
-                  sizeof V8_PRIVATE_PROPERTY_SYMBOL_STRING(                    \
-                      InterfaceName,                                           \
-                      KeyName))); /* NOLINT(readability/naming/underscores) */ \
-    }                                                                          \
-    return Symbol(                                                             \
-        privateProp->V8_PRIVATE_PROPERTY_MEMBER_NAME(InterfaceName, KeyName)   \
-            .Get(isolate));                                                    \
+#define V8_PRIVATE_PROPERTY_DEFINE_GETTER(InterfaceName, KeyName)             \
+  static Symbol V8_PRIVATE_PROPERTY_GETTER_NAME(/* // NOLINT */               \
+                                                InterfaceName, KeyName)(      \
+      v8::Isolate * isolate) {                                                \
+    V8PrivateProperty* privateProp =                                          \
+        V8PerIsolateData::from(isolate)->privateProperty();                   \
+    v8::Eternal<v8::Private>& propertyHandle =                                \
+        privateProp->V8_PRIVATE_PROPERTY_MEMBER_NAME(InterfaceName, KeyName); \
+    if (UNLIKELY(propertyHandle.IsEmpty())) {                                 \
+      static constexpr char kSymbolName[] =                                   \
+          V8_PRIVATE_PROPERTY_SYMBOL_STRING(InterfaceName, KeyName);          \
+      propertyHandle.Set(isolate, createV8Private(isolate, kSymbolName,       \
+                                                  sizeof(kSymbolName)));      \
+    }                                                                         \
+    return Symbol(isolate, propertyHandle.Get(isolate));                      \
   }
+
   V8_PRIVATE_PROPERTY_FOR_EACH(V8_PRIVATE_PROPERTY_DEFINE_GETTER)
 #undef V8_PRIVATE_PROPERTY_DEFINE_GETTER
 
  private:
   V8PrivateProperty() {}
 
   static v8::Local<v8::Private> createV8Private(v8::Isolate*,
                                                 const char* symbol,
                                                 size_t length);
 
 #define V8_PRIVATE_PROPERTY_DECLARE_MEMBER(InterfaceName, KeyName) \
   v8::Eternal<v8::Private> V8_PRIVATE_PROPERTY_MEMBER_NAME(        \
       InterfaceName, KeyName);  // NOLINT(readability/naming/underscores)
   V8_PRIVATE_PROPERTY_FOR_EACH(V8_PRIVATE_PROPERTY_DECLARE_MEMBER)
 #undef V8_PRIVATE_PROPERTY_DECLARE_MEMBER
 };
 
 }  // namespace blink
 
 #endif  // V8PrivateProperty_h