binding: Adds a check that ActiveScriptWrappable specifies [ActiveScriptWrappable].

binding: Adds a check that ActiveScriptWrappable specifies [ActiveScriptWrappable].

Makes it a compile error that a subclass of ActiveScriptWrappable is
defined without specifying [ActiveScriptWrappable] in its IDL file.

BUG=

Committed: https://crrev.com/f19828c6d2bff36b0e3811c92ebe9e8f7bd5e12c
Cr-Commit-Position: refs/heads/master@{#412217}

[Yuki, 2016-08-16 07:08:19]

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

[commit-bot: I haz the power, 2016-08-16 07:08:36]

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-16 07:11:15]

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

[Yuki, 2016-08-16 07:11:16]

Could you review this CL?

[peria, 2016-08-16 07:24:42]

peria@chromium.org changed reviewers:
+ peria@chromium.org

[peria, 2016-08-16 07:24:43]

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:44: "Be
consistent.");
Don't we need to care the inverted situation?
I mean that an IDL class has [ActiveScriptWrappable] attribute, while the C++
class does not inherit from ActiveScriptWrappable.

[Yuki, 2016-08-16 08:43:28]

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:44: "Be
consistent.");
On 2016/08/16 07:24:43, peria wrote:
> Don't we need to care the inverted situation?
> I mean that an IDL class has [ActiveScriptWrappable] attribute, while the C++
> class does not inherit from ActiveScriptWrappable.

In that case, a compile error is already happening because T* is not convertible
to ActiveScriptWrappable*.

[Yuki, 2016-08-16 08:44:37]

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/templates/interface_base.cpp:44: "Be
consistent.");
On 2016/08/16 08:43:27, Yuki wrote:
> On 2016/08/16 07:24:43, peria wrote:
> > Don't we need to care the inverted situation?
> > I mean that an IDL class has [ActiveScriptWrappable] attribute, while the
C++
> > class does not inherit from ActiveScriptWrappable.
> 
> In that case, a compile error is already happening because T* is not
convertible
> to ActiveScriptWrappable*.

Just FYI, it's the same that T* does not inherit from ScriptWrappable, or
EventTarget.

[commit-bot: I haz the power, 2016-08-16 10:31:38]

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

[commit-bot: I haz the power, 2016-08-16 10:31:39]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-08-16 11:38:02]

LGTM

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl
(right):

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl:6:
ActiveScriptWrappable,

How was CompositorWorker working without having [ActiveScriptWrappable]? Or was
it broken?

[Yuki, 2016-08-16 12:04:39]

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl
(right):

https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl:6:
ActiveScriptWrappable,
On 2016/08/16 11:38:02, haraken wrote:
> 
> How was CompositorWorker working without having [ActiveScriptWrappable]? Or
was
> it broken?

I have no idea.  Probably we were just lucky, or it may have been broken.
a) CompositorWorker itself does not define hasPendingActivity(), so it's
possible that CompositorWorker itself does not care much about pending
activities.
b) CompositorWorker inherits from InProcessWorkerBase, which defines
hasPendingActivity().

I don't have deep knowledge about CompositorWorker, so I can't say a lot.

[Yuki, 2016-08-16 12:04:48]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-08-16 12:05:05]

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-16 12:09:20]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-08-16 12:11:11]

Description was changed from

==========
binding: Adds a check that ActiveScriptWrappable specifies
[ActiveScriptWrappable].

Makes it a compile error that a subclass of ActiveScriptWrappable is
defined without specifying [ActiveScriptWrappable] in its IDL file.

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

to

==========
binding: Adds a check that ActiveScriptWrappable specifies
[ActiveScriptWrappable].

Makes it a compile error that a subclass of ActiveScriptWrappable is
defined without specifying [ActiveScriptWrappable] in its IDL file.

BUG=

Committed: https://crrev.com/f19828c6d2bff36b0e3811c92ebe9e8f7bd5e12c
Cr-Commit-Position: refs/heads/master@{#412217}
==========

[commit-bot: I haz the power, 2016-08-16 12:11:12]

Patchset 1 (id:??) landed as
https://crrev.com/f19828c6d2bff36b0e3811c92ebe9e8f7bd5e12c
Cr-Commit-Position: refs/heads/master@{#412217}

[haraken, 2016-08-16 12:20:03]

On 2016/08/16 12:04:39, Yuki wrote:
>
https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
> File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl
> (right):
> 
>
https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
> third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl:6:
> ActiveScriptWrappable,
> On 2016/08/16 11:38:02, haraken wrote:
> > 
> > How was CompositorWorker working without having [ActiveScriptWrappable]? Or
> was
> > it broken?
> 
> I have no idea.  Probably we were just lucky, or it may have been broken.
> a) CompositorWorker itself does not define hasPendingActivity(), so it's
> possible that CompositorWorker itself does not care much about pending
> activities.
> b) CompositorWorker inherits from InProcessWorkerBase, which defines
> hasPendingActivity().
> 
> I don't have deep knowledge about CompositorWorker, so I can't say a lot.

I guess it was broken but no one had noticed it because CompositorWorker is
behind a flag. Also the bug shows up only when you write JS that relies on an
expando on CompositorWorker's wrapper. It would be rare. The same thing would
have been happening for Request and Sensor.

[Yuki, 2016-08-16 12:31:56]

On 2016/08/16 12:20:03, haraken wrote:
> On 2016/08/16 12:04:39, Yuki wrote:
> >
>
https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
> > File third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl
> > (right):
> > 
> >
>
https://codereview.chromium.org/2250063002/diff/1/third_party/WebKit/Source/m...
> > third_party/WebKit/Source/modules/compositorworker/CompositorWorker.idl:6:
> > ActiveScriptWrappable,
> > On 2016/08/16 11:38:02, haraken wrote:
> > > 
> > > How was CompositorWorker working without having [ActiveScriptWrappable]?
Or
> > was
> > > it broken?
> > 
> > I have no idea.  Probably we were just lucky, or it may have been broken.
> > a) CompositorWorker itself does not define hasPendingActivity(), so it's
> > possible that CompositorWorker itself does not care much about pending
> > activities.
> > b) CompositorWorker inherits from InProcessWorkerBase, which defines
> > hasPendingActivity().
> > 
> > I don't have deep knowledge about CompositorWorker, so I can't say a lot.
> 
> I guess it was broken but no one had noticed it because CompositorWorker is
> behind a flag. Also the bug shows up only when you write JS that relies on an
> expando on CompositorWorker's wrapper. It would be rare. The same thing would
> have been happening for Request and Sensor.

Thanks, got it.