Bindings: use |holder| local variable in attribute getters and setters

Bindings: use |holder| local variable in attribute getters and setters

Current generated bindings for attribute getters has a
special case for |holder|, as this is sometimes stored as a
local variable.
This simplifies the template flow.

R=haraken
BUG=345503
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170143

[Nils Barth (inactive), 2014-03-27 03:47:29]

Another cleanup.

[haraken, 2014-03-27 04:40:36]

https://codereview.chromium.org/214143003/diff/1/Source/bindings/templates/at...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/214143003/diff/1/Source/bindings/templates/at...
Source/bindings/templates/attributes.cpp:20: attribute.is_keep_alive_for_gc %}

I'm not sure if this improves the IDL compiler. It's ugly to add a complicated
if branch just for avoid using info.GetIsolate().

Avoiding multiple info.GetIsolate()s is just for code beauty. It doesn't matter
for performance because compilers will eliminate the duplicated
info.GetIsolate()s. So we don't need to remove multiple info.GetIsolate()s
unless it improves code beauty.

https://codereview.chromium.org/214143003/diff/1/Source/bindings/templates/at...
Source/bindings/templates/attributes.cpp:227: (attribute.idl_type ==
'EventHandler' and not is_node) %}

Ditto.

[Nils Barth (inactive), 2014-03-27 05:19:45]

Revised to only use |holder|; PTAL.

https://codereview.chromium.org/214143003/diff/1/Source/bindings/templates/at...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/214143003/diff/1/Source/bindings/templates/at...
Source/bindings/templates/attributes.cpp:20: attribute.is_keep_alive_for_gc %}
On 2014/03/27 04:40:36, haraken wrote:
> I'm not sure if this improves the IDL compiler. It's ugly to add a complicated
> if branch just for avoid using info.GetIsolate().

Got it; revised CL to only have |holder|.

https://codereview.chromium.org/214143003/diff/20001/Source/bindings/template...
File Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/214143003/diff/20001/Source/bindings/template...
Source/bindings/templates/attributes.cpp:215: v8::Handle<v8::Object> holder =
info.Holder();
I've added this for consistency, and b/c it's short; WDYT?

[haraken, 2014-03-27 05:37:57]

LGTM. This looks like a good balancing point.

(Just to clarify: You don't need to try hard to all
info.GetIsolate()/info.Holder() duplication. Only if it brings code beauty, you
can remove the duplication.)

[Nils Barth (inactive), 2014-03-27 05:53:38]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-27 05:53:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/214143003/20001

[Nils Barth (inactive), 2014-03-27 05:53:47]

On 2014/03/27 05:37:57, haraken wrote:
> LGTM. This looks like a good balancing point.
> 
> (Just to clarify: You don't need to try hard to all
> info.GetIsolate()/info.Holder() duplication. Only if it brings code beauty,
you
> can remove the duplication.)

Got it, thanks!

[commit-bot: I haz the power, 2014-03-27 05:53:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-27 05:53:53]

Failed to apply patch for Source/bindings/tests/results/V8TestObject.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/bindings/tests/results/V8TestObject.cpp
  Hunk #69 FAILED at 1147.
  Hunk #71 FAILED at 1185.
  Hunk #73 FAILED at 1227.
  Hunk #75 FAILED at 1267.
  Hunk #77 FAILED at 1307.
  Hunk #79 FAILED at 1349.
  Hunk #81 FAILED at 1389.
  Hunk #82 FAILED at 1414.
  8 out of 194 hunks FAILED -- saving rejects to file
Source/bindings/tests/results/V8TestObject.cpp.rej

Patch:       Source/bindings/tests/results/V8TestObject.cpp
Index: Source/bindings/tests/results/V8TestObject.cpp
diff --git a/Source/bindings/tests/results/V8TestObject.cpp
b/Source/bindings/tests/results/V8TestObject.cpp
index
7fc50bb5eb6b34fae7a968382a0b380fc8d3f8a8..bdb574b50f75e10554685368746caf30da365fb6
100644
--- a/Source/bindings/tests/results/V8TestObject.cpp
+++ b/Source/bindings/tests/results/V8TestObject.cpp
@@ -68,7 +68,8 @@ template <typename T> void V8_USE(T) { }
 
 static void readOnlyLongAttrAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueInt(info, impl->readOnlyLongAttr());
 }
 
@@ -81,7 +82,8 @@ static void
readOnlyLongAttrAttributeGetterCallback(v8::Local<v8::String>, const
 
 static void readOnlyStringAttrAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueString(info, impl->readOnlyStringAttr(),
info.GetIsolate());
 }
 
@@ -94,13 +96,14 @@ static void
readOnlyStringAttrAttributeGetterCallback(v8::Local<v8::String>, con
 
 static void readOnlyTestObjectAttrAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     RefPtr<TestObject> result(impl->readOnlyTestObjectAttr());
     if (result &&
DOMDataStore::setReturnValueFromWrapper<V8TestObject>(info.GetReturnValue(),
result.get()))
         return;
-    v8::Handle<v8::Value> wrapper = toV8(result.get(), info.Holder(),
info.GetIsolate());
+    v8::Handle<v8::Value> wrapper = toV8(result.get(), holder,
info.GetIsolate());
     if (!wrapper.IsEmpty()) {
-        V8HiddenValue::setHiddenValue(info.GetIsolate(), info.Holder(),
v8AtomicString(info.GetIsolate(), "readOnlyTestObjectAttr"), wrapper);
+        V8HiddenValue::setHiddenValue(info.GetIsolate(), holder,
v8AtomicString(info.GetIsolate(), "readOnlyTestObjectAttr"), wrapper);
         v8SetReturnValue(info, wrapper);
     }
 }
@@ -151,7 +154,8 @@ static void
staticStringAttrAttributeSetterCallback(v8::Local<v8::String>, v8::L
 
 static void enumAttrAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueString(info, impl->enumAttr(), info.GetIsolate());
 }
 
@@ -164,7 +168,8 @@ static void
enumAttrAttributeGetterCallback(v8::Local<v8::String>, const v8::Pro
 
 static void enumAttrAttributeSetter(v8::Local<v8::Value> jsValue, const
v8::PropertyCallbackInfo<void>& info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     V8TRYCATCH_FOR_V8STRINGRESOURCE_VOID(V8StringResource<>, cppValue,
jsValue);
     String string = cppValue;
     if (!(string == "" || string == "EnumValue1" || string == "EnumValue2" ||
string == "EnumValue3"))
@@ -181,7 +186,8 @@ static void
enumAttrAttributeSetterCallback(v8::Local<v8::String>, v8::Local<v8:
 
 static void readOnlyEnumAttrAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueString(info, impl->readOnlyEnumAttr(), info.GetIsolate());
 }
 
@@ -194,7 +200,8 @@ static void
readOnlyEnumAttrAttributeGetterCallback(v8::Local<v8::String>, const
 
 static void byteAttrAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueInt(info, impl->byteAttr());
 }
 
@@ -207,8 +214,9 @@ static void
byteAttrAttributeGetterCallback(v8::Local<v8::String>, const v8::Pro
 
 static void byteAttrAttributeSetter(v8::Local<v8::Value> jsValue, const
v8::PropertyCallbackInfo<void>& info)
 {
-    ExceptionState exceptionState(ExceptionState::SetterContext, "byteAttr",
"TestObject", info.Holder(), info.GetIsolate());
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    ExceptionState exceptionState(ExceptionState::SetterContext, "byteAttr",
"TestObject", holder, info.GetIsolate());
+    TestObject* impl = V8TestObject::toNative(holder);
     V8TRYCATCH_EXCEPTION_VOID(int, cppValue, toInt8(jsValue, exceptionState),
exceptionState);
     impl->setByteAttr(cppValue);
 }
@@ -222,7 +230,8 @@ static void
byteAttrAttributeSetterCallback(v8::Local<v8::String>, v8::Local<v8:
 
 static void octetAttrAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueUnsigned(info, impl->octetAttr());
 }
 
@@ -235,8 +244,9 @@ static void
octetAttrAttributeGetterCallback(v8::Local<v8::String>, const v8::Pr
 
 static void octetAttrAttributeSetter(v8::Local<v8::Value> jsValue, const
v8::PropertyCallbackInfo<void>& info)
 {
-    ExceptionState exceptionState(ExceptionState::SetterContext, "octetAttr",
"TestObject", info.Holder(), info.GetIsolate());
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    ExceptionState exceptionState(ExceptionState::SetterContext, "octetAttr",
"TestObject", holder, info.GetIsolate());
+    TestObject* impl = V8TestObject::toNative(holder);
     V8TRYCATCH_EXCEPTION_VOID(unsigned, cppValue, toUInt8(jsValue,
exceptionState), exceptionState);
     impl->setOctetAttr(cppValue);
 }
@@ -250,7 +260,8 @@ static void
octetAttrAttributeSetterCallback(v8::Local<v8::String>, v8::Local<v8
 
 static void shortAttrAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueInt(info, impl->shortAttr());
 }
 
@@ -263,8 +274,9 @@ static void
shortAttrAttributeGetterCallback(v8::Local<v8::String>, const v8::Pr
 
 static void shortAttrAttributeSetter(v8::Local<v8::Value> jsValue, const
v8::PropertyCallbackInfo<void>& info)
 {
-    ExceptionState exceptionState(ExceptionState::SetterContext, "shortAttr",
"TestObject", info.Holder(), info.GetIsolate());
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    ExceptionState exceptionState(ExceptionState::SetterContext, "shortAttr",
"TestObject", holder, info.GetIsolate());
+    TestObject* impl = V8TestObject::toNative(holder);
     V8TRYCATCH_EXCEPTION_VOID(int, cppValue, toInt16(jsValue, exceptionState),
exceptionState);
     impl->setShortAttr(cppValue);
 }
@@ -278,7 +290,8 @@ static void
shortAttrAttributeSetterCallback(v8::Local<v8::String>, v8::Local<v8
 
 static void unsignedShortAttrAttributeGetter(const
v8::PropertyCallbackInfo<v8::Value>& info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueUnsigned(info, impl->unsignedShortAttr());
 }
 
@@ -291,8 +304,9 @@ static void
unsignedShortAttrAttributeGetterCallback(v8::Local<v8::String>, cons
 
 static void unsignedShortAttrAttributeSetter(v8::Local<v8::Value> jsValue,
const v8::PropertyCallbackInfo<void>& info)
 {
-    ExceptionState exceptionState(ExceptionState::SetterContext,
"unsignedShortAttr", "TestObject", info.Holder(), info.GetIsolate());
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    ExceptionState exceptionState(ExceptionState::SetterContext,
"unsignedShortAttr", "TestObject", holder, info.GetIsolate());
+    TestObject* impl = V8TestObject::toNative(holder);
     V8TRYCATCH_EXCEPTION_VOID(unsigned, cppValue, toUInt16(jsValue,
exceptionState), exceptionState);
     impl->setUnsignedShortAttr(cppValue);
 }
@@ -306,7 +320,8 @@ static void
unsignedShortAttrAttributeSetterCallback(v8::Local<v8::String>, v8::
 
 static void longAttrAttributeGetter(const v8::PropertyCallbackInfo<v8::Value>&
info)
 {
-    TestObject* impl = V8TestObject::toNative(info.Holder());
+    v8::Handle<v8::Object> holder = info.Holder();
+    TestObject* impl = V8TestObject::toNative(holder);
     v8SetReturnValueInt(info, impl->longAttr());
 }
 
@@ -319,8 +334,9 @@ static void
longAttrAttributeGetterCallback(v8::Local<v8::String>, const v8::Pro
 
 static void longAttrAttributeSetter(v8::Local<v8::Value> jsValue, const
v8::PropertyCallbackInfo<void>& info)
 {
-    ExceptionState exceptionState(ExceptionState::SetterContext, "longAttr",
"TestObject", info.H…
(message too large)

[Nils Barth (inactive), 2014-03-27 05:55:25]

The CQ bit was checked by nbarth@chromium.org

[commit-bot: I haz the power, 2014-03-27 05:55:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/214143003/10014

[commit-bot: I haz the power, 2014-03-27 06:59:08]

Change committed as 170143