binding: Moves hasPendingActivity from ActiveScriptWrappable to ScriptWrappable.

binding: Moves hasPendingActivity from ActiveScriptWrappable to ScriptWrappable.

BUG=

Committed: https://crrev.com/4e4184a6c895d3fc49dbd00800c6af7ec6b5093e
Cr-Commit-Position: refs/heads/master@{#413958}

[Yuki, 2016-08-22 09:55:39]

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

[commit-bot: I haz the power, 2016-08-22 09:55:51]

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, 2016-08-22 10:17:12]

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

[commit-bot: I haz the power, 2016-08-22 10:17:12]

Dry run: Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Yuki, 2016-08-22 13:53:09]

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

[commit-bot: I haz the power, 2016-08-22 13:53:16]

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

[Yuki, 2016-08-22 15:26:29]

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

[commit-bot: I haz the power, 2016-08-22 15:26:54]

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, 2016-08-22 20:09:29]

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

[commit-bot: I haz the power, 2016-08-22 20:09:30]

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

[Yuki, 2016-08-23 07:37:35]

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

[commit-bot: I haz the power, 2016-08-23 07:37:58]

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

[Yuki, 2016-08-23 07:39:42]

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

[Yuki, 2016-08-23 07:39:43]

Could you review this CL?

[haraken, 2016-08-23 07:55:40]

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:46: // V8 object
alive.

This is not really correct. ScriptWrappable keeps a weak v8::Persistent to the
DOM wrapper. It doesn't keep the DOM wrapper alive.

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:85: virtual bool
hasPendingActivity() const { return false; }

Add a comment.

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:119: if
(toWrapperTypeInfo(wrapper)->isActiveScriptWrappable()

Do we need to check isActiveScriptWrappable()? I'm assuming that if
isActiveScriptWrappable() is false,
toScriptWrappable(wrapper)->hasPendingActivity() will just return false.

Also can we add a DCHECK about the fact?

BTW, if we don't need to check isActiveScriptWrappable(), maybe can we entirely
remove [ActiveScriptWrappable]?

[commit-bot: I haz the power, 2016-08-23 09:18:07]

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

[commit-bot: I haz the power, 2016-08-23 09:18:08]

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...)

[Yuki, 2016-08-23 09:49:42]

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

[Yuki, 2016-08-23 09:49:52]

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:46: // V8 object
alive.
On 2016/08/23 07:55:40, haraken wrote:
> 
> This is not really correct. ScriptWrappable keeps a weak v8::Persistent to the
> DOM wrapper. It doesn't keep the DOM wrapper alive.

I've rewritten the class comment.

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):

https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:119: if
(toWrapperTypeInfo(wrapper)->isActiveScriptWrappable()
On 2016/08/23 07:55:40, haraken wrote:
> 
> Do we need to check isActiveScriptWrappable()? I'm assuming that if
> isActiveScriptWrappable() is false,
> toScriptWrappable(wrapper)->hasPendingActivity() will just return false.

Not necessary, but I'd expect isActiveScriptWrappable() is faster than
toScriptWrappable(wrapper)->hasPendingActivity() and isActiveScriptWrappable()
returns false for most cases.

I was afraid of a performance regression since the old implementation first
checked a similar thing.  If you think it should be okay, I'm fine to remove
isActiveScriptWrappable() check.


> Also can we add a DCHECK about the fact?

I've already added static_assert() in interface_base.cpp.  It verifies that, if
the interface does not have [ActiveScriptWrappable], hasPendingActivity() must
not be overridden, vice versa.


> BTW, if we don't need to check isActiveScriptWrappable(), maybe can we
entirely
> remove [ActiveScriptWrappable]?

That's true, and it means there will be no way for the code generator to
recognize the interface as 'active', i.e. no way to check the followings.

1. [ASW] must accompany with [DependentLifetime].
2. [ASW] C++ class must inherit from ActiveScriptWrappable class.
3. non-[ASW] C++ class must not inherit from ActiveScriptWrappable class.
4. [ASW] C++ class must override hasPendingActivity().
5. non-[ASW] C++ class must not override hasPendingActivity().

See that this CL detected ImageCapture.idl and Sensor.idl were not specifying
[DependentLifetime], and a previous CL detected 3 IDLs were missing
[ActiveScriptWrappable].  It's better to keep it as is.

[commit-bot: I haz the power, 2016-08-23 09:50:35]

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

[haraken, 2016-08-23 11:06:46]

On 2016/08/23 09:49:52, Yuki wrote:
>
https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h (right):
> 
>
https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h:46: // V8 object
> alive.
> On 2016/08/23 07:55:40, haraken wrote:
> > 
> > This is not really correct. ScriptWrappable keeps a weak v8::Persistent to
the
> > DOM wrapper. It doesn't keep the DOM wrapper alive.
> 
> I've rewritten the class comment.
> 
>
https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp (right):
> 
>
https://codereview.chromium.org/2265873003/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:119: if
> (toWrapperTypeInfo(wrapper)->isActiveScriptWrappable()
> On 2016/08/23 07:55:40, haraken wrote:
> > 
> > Do we need to check isActiveScriptWrappable()? I'm assuming that if
> > isActiveScriptWrappable() is false,
> > toScriptWrappable(wrapper)->hasPendingActivity() will just return false.
> 
> Not necessary, but I'd expect isActiveScriptWrappable() is faster than
> toScriptWrappable(wrapper)->hasPendingActivity() and isActiveScriptWrappable()
> returns false for most cases.
> 
> I was afraid of a performance regression since the old implementation first
> checked a similar thing.  If you think it should be okay, I'm fine to remove
> isActiveScriptWrappable() check.
> 
> 
> > Also can we add a DCHECK about the fact?
> 
> I've already added static_assert() in interface_base.cpp.  It verifies that,
if
> the interface does not have [ActiveScriptWrappable], hasPendingActivity() must
> not be overridden, vice versa.
> 
> 
> > BTW, if we don't need to check isActiveScriptWrappable(), maybe can we
> entirely
> > remove [ActiveScriptWrappable]?
> 
> That's true, and it means there will be no way for the code generator to
> recognize the interface as 'active', i.e. no way to check the followings.
> 
> 1. [ASW] must accompany with [DependentLifetime].
> 2. [ASW] C++ class must inherit from ActiveScriptWrappable class.
> 3. non-[ASW] C++ class must not inherit from ActiveScriptWrappable class.
> 4. [ASW] C++ class must override hasPendingActivity().
> 5. non-[ASW] C++ class must not override hasPendingActivity().
> 
> See that this CL detected ImageCapture.idl and Sensor.idl were not specifying
> [DependentLifetime], and a previous CL detected 3 IDLs were missing
> [ActiveScriptWrappable].  It's better to keep it as is.

Maybe would it be possible to implement a verification like this?

- Make ScriptWrappable::hasPendingActivity() set
ScriptWrappable::m_defaultHasPendingActivityUsed to true (on Debug builds).
- After calling ScriptWrappable::hasPendingActivity(), check that the object has
[DependentLifetime] if ScriptWrappable::m_defaultHasPendingActivityUsed is
false.

[commit-bot: I haz the power, 2016-08-23 11:42:19]

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

[commit-bot: I haz the power, 2016-08-23 11:42:20]

Dry run: This issue passed the CQ dry run.

[Yuki, 2016-08-23 11:52:54]

On 2016/08/23 11:06:46, haraken wrote:
> On 2016/08/23 09:49:52, Yuki wrote:
> > > BTW, if we don't need to check isActiveScriptWrappable(), maybe can we
> > entirely
> > > remove [ActiveScriptWrappable]?
> > 
> > That's true, and it means there will be no way for the code generator to
> > recognize the interface as 'active', i.e. no way to check the followings.
> > 
> > 1. [ASW] must accompany with [DependentLifetime].
> > 2. [ASW] C++ class must inherit from ActiveScriptWrappable class.
> > 3. non-[ASW] C++ class must not inherit from ActiveScriptWrappable class.
> > 4. [ASW] C++ class must override hasPendingActivity().
> > 5. non-[ASW] C++ class must not override hasPendingActivity().
> > 
> > See that this CL detected ImageCapture.idl and Sensor.idl were not
specifying
> > [DependentLifetime], and a previous CL detected 3 IDLs were missing
> > [ActiveScriptWrappable].  It's better to keep it as is.
> 
> Maybe would it be possible to implement a verification like this?
> 
> - Make ScriptWrappable::hasPendingActivity() set
> ScriptWrappable::m_defaultHasPendingActivityUsed to true (on Debug builds).
> - After calling ScriptWrappable::hasPendingActivity(), check that the object
has
> [DependentLifetime] if ScriptWrappable::m_defaultHasPendingActivityUsed is
> false.

All of your ideas are runtime checks.  What happens if Foo::hasPendingActiviti()
didn't call ScriptWrappable::hasPendingActivity()?  Not all HTML/JavaScript runs
through your checks.  Especially if it's debug build only, what happens if we
didn't have tests that run through the checks?

I'd highly prefer compile-time checks to runtime checks.  Compile-time checks
are developer-friendly, we can notice a simple mistake just compiling without
running the full set of tests.

[haraken, 2016-08-23 11:58:33]

On 2016/08/23 11:52:54, Yuki wrote:
> On 2016/08/23 11:06:46, haraken wrote:
> > On 2016/08/23 09:49:52, Yuki wrote:
> > > > BTW, if we don't need to check isActiveScriptWrappable(), maybe can we
> > > entirely
> > > > remove [ActiveScriptWrappable]?
> > > 
> > > That's true, and it means there will be no way for the code generator to
> > > recognize the interface as 'active', i.e. no way to check the followings.
> > > 
> > > 1. [ASW] must accompany with [DependentLifetime].
> > > 2. [ASW] C++ class must inherit from ActiveScriptWrappable class.
> > > 3. non-[ASW] C++ class must not inherit from ActiveScriptWrappable class.
> > > 4. [ASW] C++ class must override hasPendingActivity().
> > > 5. non-[ASW] C++ class must not override hasPendingActivity().
> > > 
> > > See that this CL detected ImageCapture.idl and Sensor.idl were not
> specifying
> > > [DependentLifetime], and a previous CL detected 3 IDLs were missing
> > > [ActiveScriptWrappable].  It's better to keep it as is.
> > 
> > Maybe would it be possible to implement a verification like this?
> > 
> > - Make ScriptWrappable::hasPendingActivity() set
> > ScriptWrappable::m_defaultHasPendingActivityUsed to true (on Debug builds).
> > - After calling ScriptWrappable::hasPendingActivity(), check that the object
> has
> > [DependentLifetime] if ScriptWrappable::m_defaultHasPendingActivityUsed is
> > false.
> 
> All of your ideas are runtime checks.  What happens if
Foo::hasPendingActiviti()
> didn't call ScriptWrappable::hasPendingActivity()?

If we enable the runtime check, Foo::hasPendingActivity() should *not* call
ScriptWrappable::hasPendingActivity(), right? Also it's useless to call
ScriptWrappable::hasPendingActivity() because it always returns false.

>  Not all HTML/JavaScript runs
> through your checks.  Especially if it's debug build only, what happens if we
> didn't have tests that run through the checks?
> 
> I'd highly prefer compile-time checks to runtime checks.  Compile-time checks
> are developer-friendly, we can notice a simple mistake just compiling without
> running the full set of tests.

If you want to improve the coverage, you can add the runtime check to
ScriptWrappable's constructor (or somewhere).

I'm not sure if it's a good idea to have [ActiveScriptWrappable] only for
compile-time checks.

[Yuki, 2016-08-23 12:49:18]

Discussed offline, and seems okay to go with compile-time checks for now.  Maybe
we want to implement this kind of checks as a Clang plugin.

Could you take another look?

[haraken, 2016-08-23 12:51:18]

Discussed offline. I'm okay with keeping both [ActiveScriptWrappable] and
[DependentLifetime] for now. Once we enable traceWrapper(), we can remove
[DependentLifetime] completely.

LGTM.

BTW, would it be possible to remove the ActiveScriptWrappable class entirely?
The only purpose of the ActiveScriptWrappable class is to call
perIsolateData->addActiveScriptWrappable(this) in its constructor. Assuming that
the IDL compiler knows what IDL interface has [ActiveScriptWrappable], the IDL
copmiler can generate perIsolateData->addActiveScriptWrappable(this) in the
wrap() methods of those IDL interfaces. Then we can entirely remove the
ActiveScriptWrappable class. (At that point, it would make sense to rename
[ActiveScriptWrappable] to [HasPendingActivity].) Of course, you don't need to
do that in this CL though.

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

https://codereview.chromium.org/2265873003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:175: if
(value->IsIndependent())

Now we can move this check to above line 164 (which will speed up the code) and
remove line 167 - 170, because [DependentLifetime] is already verified at
compile time.

[Yuki, 2016-08-23 13:13:33]

> BTW, would it be possible to remove the ActiveScriptWrappable class entirely?
> The only purpose of the ActiveScriptWrappable class is to call
> perIsolateData->addActiveScriptWrappable(this) in its constructor. Assuming
that
> the IDL compiler knows what IDL interface has [ActiveScriptWrappable], the IDL
> copmiler can generate perIsolateData->addActiveScriptWrappable(this) in the
> wrap() methods of those IDL interfaces. Then we can entirely remove the
> ActiveScriptWrappable class. (At that point, it would make sense to rename
> [ActiveScriptWrappable] to [HasPendingActivity].) Of course, you don't need to
> do that in this CL though.

Possible, but it's very hard.  Actually I gave it a shot, and gave up.

V8PerIsolateData holds a set of ActiveScriptWrappables as a
HeapHashSet<WeakMember<ASW>>.  In order to let Oilplan collect these objects, I
think they need to be Oilpan's weak pointers.

Then, when we remove ActiveScriptWrappable, we have to make ScriptWrappable
GarbageCollectedMixin, and this is a really tough job.  There are a lot of
places to convert raw SW pointer to Member<SW>.  Oh, oh.

Maybe, someday someone will do it?

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

https://codereview.chromium.org/2265873003/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8GCController.cpp:175: if
(value->IsIndependent())
On 2016/08/23 12:51:18, haraken wrote:
> 
> Now we can move this check to above line 164 (which will speed up the code)
and
> remove line 167 - 170, because [DependentLifetime] is already verified at
> compile time.
> 
> 

Done.

[Yuki, 2016-08-23 13:13:56]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2016-08-23 13:13:57]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2265873003/#ps100001
(title: "Addressed review comments.")

[commit-bot: I haz the power, 2016-08-23 13:14:15]

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, 2016-08-23 14:48:42]

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

[commit-bot: I haz the power, 2016-08-23 14:48:43]

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...)

[haraken, 2016-08-23 15:19:59]

On 2016/08/23 13:13:33, Yuki wrote:
> > BTW, would it be possible to remove the ActiveScriptWrappable class
entirely?
> > The only purpose of the ActiveScriptWrappable class is to call
> > perIsolateData->addActiveScriptWrappable(this) in its constructor. Assuming
> that
> > the IDL compiler knows what IDL interface has [ActiveScriptWrappable], the
IDL
> > copmiler can generate perIsolateData->addActiveScriptWrappable(this) in the
> > wrap() methods of those IDL interfaces. Then we can entirely remove the
> > ActiveScriptWrappable class. (At that point, it would make sense to rename
> > [ActiveScriptWrappable] to [HasPendingActivity].) Of course, you don't need
to
> > do that in this CL though.
> 
> Possible, but it's very hard.  Actually I gave it a shot, and gave up.
> 
> V8PerIsolateData holds a set of ActiveScriptWrappables as a
> HeapHashSet<WeakMember<ASW>>.  In order to let Oilplan collect these objects,
I
> think they need to be Oilpan's weak pointers.
> 
> Then, when we remove ActiveScriptWrappable, we have to make ScriptWrappable
> GarbageCollectedMixin, and this is a really tough job.  There are a lot of
> places to convert raw SW pointer to Member<SW>.  Oh, oh.
> 
> Maybe, someday someone will do it?

Yeah, agreed. Making ScriptWrappable a GCMixin sounds nice to me (otherwise raw
ScriptWrappable* pointers have a risk of causing use-after-frees), but it will
require a lot of work.

[Yuki, 2016-08-24 02:08:04]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-08-24 02:08:42]

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, 2016-08-24 02:55:36]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-08-24 02:58:36]

Description was changed from

==========
binding: Moves hasPendingActivity from ActiveScriptWrappable to ScriptWrappable.

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

to

==========
binding: Moves hasPendingActivity from ActiveScriptWrappable to ScriptWrappable.

BUG=

Committed: https://crrev.com/4e4184a6c895d3fc49dbd00800c6af7ec6b5093e
Cr-Commit-Position: refs/heads/master@{#413958}
==========

[commit-bot: I haz the power, 2016-08-24 02:58:38]

Patchset 6 (id:??) landed as
https://crrev.com/4e4184a6c895d3fc49dbd00800c6af7ec6b5093e
Cr-Commit-Position: refs/heads/master@{#413958}