[Bindings] Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

This change makes V8HiddenValue class stateless, and we can remove
m_hiddenValue in V8PerIsolate.


BUG=611864
Review-Url: https://codereview.chromium.org/2774233007
Cr-Commit-Position: refs/heads/master@{#462837}
Committed: https://chromium.googlesource.com/chromium/src/+/5daaa56488f8ae678e47661e861e8e0c6db8e2a8

[peria, 2017-03-30 10:26:04]

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

[commit-bot: I haz the power, 2017-03-30 10:26:16]

Dry run: 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-03-30 10:51:53]

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

[commit-bot: I haz the power, 2017-03-30 10:51:54]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[peria, 2017-04-06 03:22:52]

Description was changed from

==========
Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

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

to

==========
Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

This change makes V8HiddenValue class static, and we can remove
m_hiddenValues in V8PerIsolate.


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

[peria, 2017-04-06 03:23:02]

Patchset #1 (id:1) has been deleted

[peria, 2017-04-06 03:23:12]

Patchset #1 (id:20001) has been deleted

[peria, 2017-04-06 03:23:18]

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

[peria, 2017-04-06 03:23:23]

Patchset #1 (id:40001) has been deleted

[peria, 2017-04-06 03:23:30]

Patchset #1 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-04-06 03:23:32]

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-06 03:23:37]

Patchset #1 (id:80001) has been deleted

[commit-bot: I haz the power, 2017-04-06 03:48:30]

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

[commit-bot: I haz the power, 2017-04-06 03:48:31]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[peria, 2017-04-06 04:16:52]

Patchset #1 (id:100001) has been deleted

[peria, 2017-04-06 04:16:57]

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

[commit-bot: I haz the power, 2017-04-06 04:17:14]

Dry run: 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-06 05:02:42]

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

[commit-bot: I haz the power, 2017-04-06 05:02:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[peria, 2017-04-06 05:50:19]

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

[commit-bot: I haz the power, 2017-04-06 05:51:12]

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-06 07:09:05]

Patchset #1 (id:120001) has been deleted

[peria, 2017-04-06 07:09:12]

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

[peria, 2017-04-06 07:09:21]

Patchset #1 (id:140001) has been deleted

[commit-bot: I haz the power, 2017-04-06 07:09:27]

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-06 07:09:55]

Description was changed from

==========
Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

This change makes V8HiddenValue class static, and we can remove
m_hiddenValues in V8PerIsolate.


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

to

==========
Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

This change makes V8HiddenValue class stateless, and we can remove
m_hiddenValue in V8PerIsolate.


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

[peria, 2017-04-06 07:12:27]

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

[peria, 2017-04-06 07:12:28]

PTL

[commit-bot: I haz the power, 2017-04-06 07:18:07]

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

[commit-bot: I haz the power, 2017-04-06 07:18:08]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Yuki, 2017-04-06 07:32:03]

LGTM.

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp
(right):

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:43:
v8::Local<v8::Value> cachedPromise = promiseSymbol().getOrEmpty(wrapper);
You can use getOrUndefined here.

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:186:
return V8PrivateProperty::get##Interface##Name##Promise(m_isolate);
Use V8_PRIVATE_PROPERTY_GETTER_NAME.

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:200:
return V8PrivateProperty::get##Interface##Name##Resolver(m_isolate);
ditto

[haraken, 2017-04-06 07:45:54]

LGTM

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

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp
(right):

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:43:
v8::Local<v8::Value> cachedPromise = promiseSymbol().getOrEmpty(wrapper);
On 2017/04/06 07:32:03, Yuki wrote:
> You can use getOrUndefined here.

Done.

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:186:
return V8PrivateProperty::get##Interface##Name##Promise(m_isolate);
On 2017/04/06 07:32:03, Yuki wrote:
> Use V8_PRIVATE_PROPERTY_GETTER_NAME.

Done.

https://codereview.chromium.org/2774233007/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:200:
return V8PrivateProperty::get##Interface##Name##Resolver(m_isolate);
On 2017/04/06 07:32:03, Yuki wrote:
> ditto

Done.

[peria, 2017-04-06 07:56:45]

The CQ bit was checked by peria@chromium.org

[peria, 2017-04-06 07:56:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2774233007/#ps180001
(title: "work for comments")

[commit-bot: I haz the power, 2017-04-06 07:57:01]

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-06 10:09:31]

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

[commit-bot: I haz the power, 2017-04-06 10:09:33]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[peria, 2017-04-06 10:18:08]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2017-04-06 10:18:20]

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-06 12:31:06]

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

[commit-bot: I haz the power, 2017-04-06 12:31:07]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[peria, 2017-04-07 07:07:34]

The CQ bit was checked by peria@chromium.org

[commit-bot: I haz the power, 2017-04-07 07:07:46]

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-07 09:08:04]

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

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

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[peria, 2017-04-07 09:39:01]

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

[commit-bot: I haz the power, 2017-04-07 09:39:19]

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-07 09:43:37]

As far as I investigated, the only test failure on MacOSX seems to be caused by
V8.

I change the triggering line as a workaround and trying again.
Can I land this as PS3, if the test passes?

https://codereview.chromium.org/2774233007/diff/200001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp
(right):

https://codereview.chromium.org/2774233007/diff/200001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:166:
promiseSymbol().set(wrapper, v8::Undefined(m_isolate));
v8::Object::DeleteProperty() seems to have a bug if we build with
dcheck_always_on (and some other flags may be needed), and the behavior of
V8HiddenValue::deleteHiddenValue() was to set v8::Undefined(), so I set
Undefined here as a workaround.

[haraken, 2017-04-07 09:45:42]

On 2017/04/07 09:43:37, peria wrote:
> As far as I investigated, the only test failure on MacOSX seems to be caused
by
> V8.
> 
> I change the triggering line as a workaround and trying again.
> Can I land this as PS3, if the test passes?
>
>
https://codereview.chromium.org/2774233007/diff/200001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp
> (right):
> 
>
https://codereview.chromium.org/2774233007/diff/200001/third_party/WebKit/Sou...
> third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:166:
> promiseSymbol().set(wrapper, v8::Undefined(m_isolate));
> v8::Object::DeleteProperty() seems to have a bug if we build with
> dcheck_always_on (and some other flags may be needed), and the behavior of
> V8HiddenValue::deleteHiddenValue() was to set v8::Undefined(), so I set
> Undefined here as a workaround.

Would you file a bug to V8 and get some help from them?

[Yuki, 2017-04-07 09:46:48]

LGTM.

A bug is already filed.
https://bugs.chromium.org/p/v8/issues/detail?id=6227

[peria, 2017-04-07 09:47:56]

On 2017/04/07 09:45:42, haraken wrote:
> On 2017/04/07 09:43:37, peria wrote:
> > As far as I investigated, the only test failure on MacOSX seems to be caused
> by
> > V8.
> > 
> > I change the triggering line as a workaround and trying again.
> > Can I land this as PS3, if the test passes?
> >
> >
>
https://codereview.chromium.org/2774233007/diff/200001/third_party/WebKit/Sou...
> > File
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp
> > (right):
> > 
> >
>
https://codereview.chromium.org/2774233007/diff/200001/third_party/WebKit/Sou...
> >
third_party/WebKit/Source/bindings/core/v8/ScriptPromisePropertyBase.cpp:166:
> > promiseSymbol().set(wrapper, v8::Undefined(m_isolate));
> > v8::Object::DeleteProperty() seems to have a bug if we build with
> > dcheck_always_on (and some other flags may be needed), and the behavior of
> > V8HiddenValue::deleteHiddenValue() was to set v8::Undefined(), so I set
> > Undefined here as a workaround.
> 
> Would you file a bug to V8 and get some help from them?

I filed http://crbug.com/v8/6227, and it is commented in the code.
I did not ask them directly yet.

[haraken, 2017-04-07 09:49:57]

LGTM

[peria, 2017-04-07 09:50:55]

The CQ bit was unchecked by peria@chromium.org

[peria, 2017-04-07 09:50:56]

The CQ bit was checked by peria@chromium.org

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

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-07 11:38:35]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1491558656161690,
"parent_rev": "f714bc0e41605712134e1a5268a56fcda79a2c0f", "commit_rev":
"5daaa56488f8ae678e47661e861e8e0c6db8e2a8"}

[commit-bot: I haz the power, 2017-04-07 11:39:26]

Description was changed from

==========
Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

This change makes V8HiddenValue class stateless, and we can remove
m_hiddenValue in V8PerIsolate.


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

to

==========
Move ScriptPromiseProperties from V8HiddenValue to V8PrivateProperty

This change makes V8HiddenValue class stateless, and we can remove
m_hiddenValue in V8PerIsolate.


BUG=611864

Review-Url: https://codereview.chromium.org/2774233007
Cr-Commit-Position: refs/heads/master@{#462837}
Committed:
https://chromium.googlesource.com/chromium/src/+/5daaa56488f8ae678e47661e861e...
==========

[commit-bot: I haz the power, 2017-04-07 11:39:27]

Committed patchset #3 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/5daaa56488f8ae678e47661e861e...