bindings: Supports [SameObject] extended attribute.

bindings: Supports [SameObject] extended attribute.

Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.

This CL stores the first returned value in the holder's
private value, and returns it for the second time.

BUG=462913
Committed: https://crrev.com/40d19a73c02f0185e5a49c4761860762896dec28
Cr-Commit-Position: refs/heads/master@{#393827}

[Yuki, 2016-05-16 09:13:44]

yukishiino@chromium.org changed reviewers:
+ bashi@chromium.org, haraken@chromium.org

[Yuki, 2016-05-16 09:13:45]

Could you guys review this CL?

[haraken, 2016-05-16 09:17:44]

LGTM on my side.

https://codereview.chromium.org/1980553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/1980553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp:137:
V8HiddenValue::setHiddenValue(ScriptState::current(info.GetIsolate()), holder,
propertyName, info.GetReturnValue().Get());

Can we merge the branch with '{% if attribute.cached_attribute_validation_method
%}' ?

[bashi, 2016-05-16 09:27:20]

lgtm. Could you update the description and BUG number (462913) ?

[Yuki, 2016-05-16 09:31:25]

Description was changed from

==========
bindings: Supports [SameObject] extended attribute.

BUG=
==========

to

==========
bindings: Supports [SameObject] extended attribute.

Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.

This CL stores the first returned value in the holder's
private value, and returns it for the second time.

BUG=462913
==========

[Yuki, 2016-05-16 09:32:14]

Oops, nice catch.  I've updated the description and BUG=.

https://codereview.chromium.org/1980553002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):

https://codereview.chromium.org/1980553002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp:137:
V8HiddenValue::setHiddenValue(ScriptState::current(info.GetIsolate()), holder,
propertyName, info.GetReturnValue().Get());
On 2016/05/16 09:17:44, haraken wrote:
> 
> Can we merge the branch with '{% if
attribute.cached_attribute_validation_method
> %}' ?

I had the same idea, but it turned out that it's not so easy or it doesn't make
the code cleaner.

The underlying difficulty is that
- the resulting value is sometimes stored in |v8Value|, but not always.
- since we're relying on attr.v8_set_return_value, we don't know exactly where
the return value is stored.  If it's not |v8Value|, where?
- v8SetReturnValueString, v8SetReturnValue and v8SetReturnValueFast take a
different number of arguments.

Sigh...

[haraken, 2016-05-16 09:41:44]

On 2016/05/16 09:32:14, Yuki wrote:
> Oops, nice catch.  I've updated the description and BUG=.
> 
>
https://codereview.chromium.org/1980553002/diff/20001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/templates/attributes.cpp (right):
> 
>
https://codereview.chromium.org/1980553002/diff/20001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/templates/attributes.cpp:137:
> V8HiddenValue::setHiddenValue(ScriptState::current(info.GetIsolate()), holder,
> propertyName, info.GetReturnValue().Get());
> On 2016/05/16 09:17:44, haraken wrote:
> > 
> > Can we merge the branch with '{% if
> attribute.cached_attribute_validation_method
> > %}' ?
> 
> I had the same idea, but it turned out that it's not so easy or it doesn't
make
> the code cleaner.
> 
> The underlying difficulty is that
> - the resulting value is sometimes stored in |v8Value|, but not always.
> - since we're relying on attr.v8_set_return_value, we don't know exactly where
> the return value is stored.  If it's not |v8Value|, where?
> - v8SetReturnValueString, v8SetReturnValue and v8SetReturnValueFast take a
> different number of arguments.
> 
> Sigh...

I'll defer the decision to you :)

[Yuki, 2016-05-16 09:45:28]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-05-16 09:45:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1980553002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1980553002/20001

[Yuki, 2016-05-16 09:46:15]

The CQ bit was unchecked by yukishiino@chromium.org

[Yuki, 2016-05-16 13:01:54]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-05-16 13:02:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1980553002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1980553002/20001

[commit-bot: I haz the power, 2016-05-16 13:45:53]

Description was changed from

==========
bindings: Supports [SameObject] extended attribute.

Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.

This CL stores the first returned value in the holder's
private value, and returns it for the second time.

BUG=462913
==========

to

==========
bindings: Supports [SameObject] extended attribute.

Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.

This CL stores the first returned value in the holder's
private value, and returns it for the second time.

BUG=462913
==========

[commit-bot: I haz the power, 2016-05-16 13:45:54]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-05-16 13:47:19]

Description was changed from

==========
bindings: Supports [SameObject] extended attribute.

Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.

This CL stores the first returned value in the holder's
private value, and returns it for the second time.

BUG=462913
==========

to

==========
bindings: Supports [SameObject] extended attribute.

Syntactically we've supported [SameObject] extended attribute,
but we've not had any implementation for it.

This CL stores the first returned value in the holder's
private value, and returns it for the second time.

BUG=462913

Committed: https://crrev.com/40d19a73c02f0185e5a49c4761860762896dec28
Cr-Commit-Position: refs/heads/master@{#393827}
==========

[commit-bot: I haz the power, 2016-05-16 13:47:20]

Patchset 2 (id:??) landed as
https://crrev.com/40d19a73c02f0185e5a49c4761860762896dec28
Cr-Commit-Position: refs/heads/master@{#393827}

[Yuki, 2016-05-20 11:17:30]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2000643002/ by yukishiino@chromium.org.

The reason for reverting is: This CL hits terrible performance regressions..