[Bindings] Simplify V8PrivateProperty::Symbol methods

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

Index: third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
diff --git a/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h b/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
index 3284bfae7e63c4c310b2ebba2cac8f959d5f690d..b1a4af76018657ff9dfc6b9b5e9bb1d3afab5990 100644
--- a/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
+++ b/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
@@ -17,7 +17,6 @@
 
 namespace blink {
 
-class ScriptState;
 class ScriptWrappable;
 
 // Apply |X| for each pair of (InterfaceName, PrivateKeyName).
@@ -88,35 +87,27 @@ class CORE_EXPORT V8PrivateProperty {
     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;
@@ -125,42 +116,41 @@ class CORE_EXPORT V8PrivateProperty {
     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