[Bindings] Replace V8HiddenValue in generated code with V8PrivateProperty

Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use dynamic
strings as property keys.  (It is possible to list all currently used properties and cache
them in V8PrivateProperty, but it is not practical for future updates.)

As far as I investigate, a property key "state" was the only key that was used in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864
Review-Url: https://codereview.chromium.org/2794453002
Cr-Commit-Position: refs/heads/master@{#461388}
Committed: https://chromium.googlesource.com/chromium/src/+/330bfd236aa4574a1ee370d4b369e755c6fe034d

[peria, 2017-03-31 09:04:14]

Description was changed from

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods


BUG=611864
==========

to

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.

As far as I investigate, a property key "state" was used in generated code and
hand
written code (V8PopStateEventCustom.cpp), so this CL also updates the .cpp file
to share the property.


BUG=611864
==========

[peria, 2017-03-31 09:10:01]

Description was changed from

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.

As far as I investigate, a property key "state" was used in generated code and
hand
written code (V8PopStateEventCustom.cpp), so this CL also updates the .cpp file
to share the property.


BUG=611864
==========

to

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.
It is possible to list all currently used properties and cache them in
V8PrivateProperty,
but it is not practical for future updates.

As far as I investigate, a property key "state" was used in generated code and
hand
written code (V8PopStateEventCustom.cpp), so this CL also updates the .cpp file
to share the property.


BUG=611864
==========

[peria, 2017-03-31 09:17:56]

Patchset #1 (id:1) has been deleted

[peria, 2017-03-31 09:42:05]

Description was changed from

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.
It is possible to list all currently used properties and cache them in
V8PrivateProperty,
but it is not practical for future updates.

As far as I investigate, a property key "state" was used in generated code and
hand
written code (V8PopStateEventCustom.cpp), so this CL also updates the .cpp file
to share the property.


BUG=611864
==========

to

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.
It is possible to list all currently used properties and cache them in
V8PrivateProperty,
but it is not practical for future updates.

As far as I investigate, a property key "state" was the only key that was used
in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864
==========

[peria, 2017-03-31 09:46:01]

Description was changed from

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.
It is possible to list all currently used properties and cache them in
V8PrivateProperty,
but it is not practical for future updates.

As far as I investigate, a property key "state" was the only key that was used
in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864
==========

to

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.  (It is possible to list all currently used properties
and cache
them in V8PrivateProperty, but it is not practical for future updates. This is
the reason
why I keep the legacy behavior.)

As far as I investigate, a property key "state" was the only key that was used
in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864
==========

[peria, 2017-03-31 09:46:37]

Description was changed from

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.  (It is possible to list all currently used properties
and cache
them in V8PrivateProperty, but it is not practical for future updates. This is
the reason
why I keep the legacy behavior.)

As far as I investigate, a property key "state" was the only key that was used
in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864
==========

to

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.  (It is possible to list all currently used properties
and cache
them in V8PrivateProperty, but it is not practical for future updates.)

As far as I investigate, a property key "state" was the only key that was used
in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864
==========

[peria, 2017-03-31 09:55:15]

Patchset #1 (id:20001) has been deleted

[peria, 2017-03-31 09:56:00]

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

[peria, 2017-03-31 09:56:01]

PTL

[haraken, 2017-03-31 10:19:14]

LGTM

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

https://codereview.chromium.org/2794453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:405:
.set(holder, v8::Undefined(isolate));

Just to confirm: Is setting undefined equivalent to deleting that private
property?

[peria, 2017-03-31 11:34:08]

Patchset #2 (id:60001) has been deleted

[peria, 2017-03-31 11:34:21]

The CQ bit was checked by peria@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 11:34:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[peria, 2017-03-31 11:35:25]

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

https://codereview.chromium.org/2794453002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:405:
.set(holder, v8::Undefined(isolate));
On 2017/03/31 10:19:14, haraken wrote:
> 
> Just to confirm: Is setting undefined equivalent to deleting that private
> property?

Unfortunately, it is not equivalent.
However, I'd like to use this implementation because of following reasons.
1) V8HiddenValue::deleteHiddenValue() did it. So this implementation is
at least consistent with the previous one.
2) Deleting is unnecessarily slow. (See comments in
V8HiddenValue::deleteHiddenValue().

I put a comment with (2) to explain what we want to do and why its
implementation is not correct.

[commit-bot: I haz the power, 2017-03-31 12:57:16]

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

[commit-bot: I haz the power, 2017-03-31 12:57:17]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-03-31 13:38:30]

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:171: static
Symbol createSymbol(v8::Isolate* isolate, const char* symbol) {
This method should be renamed to getOrCreateSymbol or simply getSymbol, as we're
now using createCachedV8Private.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp
(right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:44:
constexpr char kSymbolKey[] = "History#State";
Could you add a TODO comment, here?
This is abuse or invalid use of [CachedAttribute].

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:52:
V8PrivateProperty::createSymbol(scriptState->isolate(), kSymbolKey)
We don't need to kSymbolKey for |popStateEvent| or |info.Holder()|.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:65:
if (!result->IsUndefined()) {
Are you sure that the state attribute never be undefined?

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:101:
if (!history->stateChanged() && propertySymbol.hasValue(v8History)) {
I don't think that this part is performance sensitive, and the spec doesn't
require that the attribute must return the same object as History.state.  So, in
a follow-up CL, we could stop using this.
https://html.spec.whatwg.org/multipage/browsers.html#the-popstateevent-interface

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

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:67: if
(!v8Value->IsUndefined()) {
With this change, we can no longer cache the value of |undefined|.  I don't know
if there is a case, but if the attribute value is |undefined|, we'd like to
cache |undefined|.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:404: //
Instead, we replace the cached value with "undefined".
Is this really happening so often?  I'd expect that it's rare and negligible.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:494:
v8::String::Utf8Value attributeName(name);
I'm uneasy with this name...  This is a setter callback for a method.  |name| is
a method name.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:501:
V8PrivateProperty::createSymbol(info.GetIsolate(), *attributeName)
Could you add a comment that attributeName must match with {{method.name}} in
FooOriginSafeMethodGetter in methods.cpp.tmpl?  (Actually opposite. 
{{method.name}} must match with |name|.

By the way, we already have |name| of type v8::String.
name => attributeName => v8String again
looks a little silly.  But this is not a critical path, so I'm okay with this.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:538:
V8PrivateProperty::createSymbol(info.GetIsolate(), "{{method.name}}")
Could you add a comment that {{method.name}} was chosen so that it matches with
|name| in FooOriginSafeMethodSetter in interface.cpp.tmpl.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:539:
.getOrUndefined(v8::Local<v8::Object>::Cast(info.Holder()));
nit: 4 space indent in addition to the level of V8PrivateProperty::...

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:540: if
(!propertyValue->IsUndefined()) {
The value can be |undefined|.

[peria, 2017-04-03 04:58:44]

done other than deleting cached property.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h (right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h:171: static
Symbol createSymbol(v8::Isolate* isolate, const char* symbol) {
On 2017/03/31 13:38:29, Yuki wrote:
> This method should be renamed to getOrCreateSymbol or simply getSymbol, as
we're
> now using createCachedV8Private.

Done.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp
(right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:44:
constexpr char kSymbolKey[] = "History#State";
On 2017/03/31 13:38:29, Yuki wrote:
> Could you add a TODO comment, here?
> This is abuse or invalid use of [CachedAttribute].

Done.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:52:
V8PrivateProperty::createSymbol(scriptState->isolate(), kSymbolKey)
On 2017/03/31 13:38:29, Yuki wrote:
> We don't need to kSymbolKey for |popStateEvent| or |info.Holder()|.

Done.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:65:
if (!result->IsUndefined()) {
On 2017/03/31 13:38:29, Yuki wrote:
> Are you sure that the state attribute never be undefined?

State must be a serializable object, and "undefined" is serializable.
Fixed the code.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/custom/V8PopStateEventCustom.cpp:101:
if (!history->stateChanged() && propertySymbol.hasValue(v8History)) {
On 2017/03/31 13:38:29, Yuki wrote:
> I don't think that this part is performance sensitive, and the spec doesn't
> require that the attribute must return the same object as History.state.  So,
in
> a follow-up CL, we could stop using this.
>
https://html.spec.whatwg.org/multipage/browsers.html#the-popstateevent-interface

Acknowledged.

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

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:67: if
(!v8Value->IsUndefined()) {
On 2017/03/31 13:38:29, Yuki wrote:
> With this change, we can no longer cache the value of |undefined|.  I don't
know
> if there is a case, but if the attribute value is |undefined|, we'd like to
> cache |undefined|.

IIUC, the old code also did not cache "undefined".

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/attributes.cpp.tmpl:404: //
Instead, we replace the cached value with "undefined".
On 2017/03/31 13:38:29, Yuki wrote:
> Is this really happening so often?  I'd expect that it's rare and negligible.

I agree. It happens rarely.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl (right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:494:
v8::String::Utf8Value attributeName(name);
On 2017/03/31 13:38:29, Yuki wrote:
> I'm uneasy with this name...  This is a setter callback for a method.  |name|
is
> a method name.

Done.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl:501:
V8PrivateProperty::createSymbol(info.GetIsolate(), *attributeName)
On 2017/03/31 13:38:29, Yuki wrote:
> Could you add a comment that attributeName must match with {{method.name}} in
> FooOriginSafeMethodGetter in methods.cpp.tmpl?  (Actually opposite. 
> {{method.name}} must match with |name|.
> 
> By the way, we already have |name| of type v8::String.
> name => attributeName => v8String again
> looks a little silly.  But this is not a critical path, so I'm okay with this.

Done.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:538:
V8PrivateProperty::createSymbol(info.GetIsolate(), "{{method.name}}")
On 2017/03/31 13:38:29, Yuki wrote:
> Could you add a comment that {{method.name}} was chosen so that it matches
with
> |name| in FooOriginSafeMethodSetter in interface.cpp.tmpl.

Done.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:539:
.getOrUndefined(v8::Local<v8::Object>::Cast(info.Holder()));
On 2017/03/31 13:38:30, Yuki wrote:
> nit: 4 space indent in addition to the level of V8PrivateProperty::...

Done.

https://codereview.chromium.org/2794453002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:540: if
(!propertyValue->IsUndefined()) {
On 2017/03/31 13:38:29, Yuki wrote:
> The value can be |undefined|.

Done.

[Yuki, 2017-04-03 07:20:39]

LGTM.

https://codereview.chromium.org/2794453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2794453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:544: if
(!propertyValue.IsEmpty()) {
nit: You may want to use hasValue and getOrUndefined instead.

[peria, 2017-04-03 07:44:01]

The CQ bit was checked by peria@chromium.org to run a CQ dry run

[peria, 2017-04-03 07:44:06]

https://codereview.chromium.org/2794453002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://codereview.chromium.org/2794453002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:544: if
(!propertyValue.IsEmpty()) {
On 2017/04/03 07:20:39, Yuki wrote:
> nit: You may want to use hasValue and getOrUndefined instead.

Done.

[commit-bot: I haz the power, 2017-04-03 07:44:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[peria, 2017-04-03 07:47:05]

The CQ bit was unchecked by peria@chromium.org

[peria, 2017-04-03 07:47:05]

The CQ bit was checked by peria@chromium.org

[peria, 2017-04-03 07:47:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2794453002/#ps140001
(title: ".")

[commit-bot: I haz the power, 2017-04-03 07:47:13]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-03 09:03:59]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1491205625934790,
"parent_rev": "0acda3f92769c2ac7e250231fa08ae39c3e27adc", "commit_rev":
"330bfd236aa4574a1ee370d4b369e755c6fe034d"}

[commit-bot: I haz the power, 2017-04-03 09:05:07]

Description was changed from

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.  (It is possible to list all currently used properties
and cache
them in V8PrivateProperty, but it is not practical for future updates.)

As far as I investigate, a property key "state" was the only key that was used
in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864
==========

to

==========
Replace V8HiddenValue methods in generated code with V8PrivateProperty methods.

This CL replaces V8HiddenValue with V8PrivateProperty, but it keeps to use
dynamic
strings as property keys.  (It is possible to list all currently used properties
and cache
them in V8PrivateProperty, but it is not practical for future updates.)

As far as I investigate, a property key "state" was the only key that was used
in both
generated code and hand written code (V8PopStateEventCustom.cpp).
So this CL also updates the .cpp file to share the property symbol.


BUG=611864

Review-Url: https://codereview.chromium.org/2794453002
Cr-Commit-Position: refs/heads/master@{#461388}
Committed:
https://chromium.googlesource.com/chromium/src/+/330bfd236aa4574a1ee370d4b369...
==========

[commit-bot: I haz the power, 2017-04-03 09:05:08]

Committed patchset #5 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/330bfd236aa4574a1ee370d4b369...