Make NodeFilter a legacy callback interface.

Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Add new legacy_callback_interface.*.tmpl
  Because NodeFilter can't use generated methods for callback interface for now.

* Source/bindings/tests/idls/core/TestLegacyCallbackInterface.idl
  Add it for quick check of legacy callback interface generation.


BUG=591919
Review-Url: https://codereview.chromium.org/2837923003
Cr-Commit-Position: refs/heads/master@{#467963}
Committed: https://chromium.googlesource.com/chromium/src/+/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0

[tkent, 2017-04-27 08:08:44]

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

[commit-bot: I haz the power, 2017-04-27 08:08:57]

Dry run: CQ is trying da patch.

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

[tkent, 2017-04-27 08:09:42]

Patchset #1 (id:1) has been deleted

[tkent, 2017-04-27 08:11:00]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

WIP

BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

BUG=591919
==========

[tkent, 2017-04-27 08:19:25]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

Legacy callback interface needs an interface object, but it has no instances and
no prototype.

Implementation:
  * This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

  * Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::

BUG=591919
==========

[tkent, 2017-04-27 08:20:28]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

Legacy callback interface needs an interface object, but it has no instances and
no prototype.

Implementation:
  * This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

  * Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::

BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

Legacy callback interface needs an interface object, but it has no instances and
no prototype.

Implementation:

  * This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

  * Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

BUG=591919
==========

[tkent, 2017-04-27 08:21:46]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

Legacy callback interface needs an interface object, but it has no instances and
no prototype.

Implementation:

  * This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

  * Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

Legacy callback interface needs an interface object, but it has no instances and
no prototype.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Source/bindings/tests/idls/core/TestObject.idl
  Add 'readonly' because non-readonly NodeFilter attribute doesn't exist in the
  real world.

BUG=591919
==========

[tkent, 2017-04-27 08:22:35]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object

Legacy callback interface needs an interface object, but it has no instances and
no prototype.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Source/bindings/tests/idls/core/TestObject.idl
  Add 'readonly' because non-readonly NodeFilter attribute doesn't exist in the
  real world.

BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Source/bindings/tests/idls/core/TestObject.idl
  Add 'readonly' because non-readonly NodeFilter attribute doesn't exist in the
  real world.

BUG=591919
==========

[tkent, 2017-04-27 08:23:17]

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

[tkent, 2017-04-27 08:23:18]

yukishiino@, bashi@, would you review this please?

[bashi, 2017-04-27 08:49:50]

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/global-constructors.html (left):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/global-constructors.html:84:
shouldBe("window.NodeFilter.prototype.isPrototypeOf(nodeFilter)", false);
Could you elaborate why removing this is fine? Should we add another ShouldBe()
or is this change already covered by wpt?

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/utilities.py:348: match =
re.search(r'callback\s+interface\s+\w+\s*{', file_contents)
Could you add a comment why we search 'const' to detect legacy callback
interface?

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:83:
'methods': [] if is_legacy else [method_context(operation)
nit: We use alphabetical ordering in dicts.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:83:
'methods': [] if is_legacy else [method_context(operation)
Why do we need to set an empty list when |is_legacy| is true?

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:12: {%
if is_legacy %}
It seems that we should have a new template file
(legacy_callback_interface.{cpp,h}.tmpl).

I think you can modify CodeGeneratorV8.generate_interface_code() to select
appropriate template files. You will need to create Jinja2 context before
selecting templates.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:13: //
Suppress warning: global constructors, because struct WrapperTypeInfo is trivial
nit: Wrap to 80 columns?

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:51:
//interfaceTemplate->SetLength({{interface_length}});
Remove?

[tkent, 2017-04-27 09:15:12]

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/dom/global-constructors.html (left):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/dom/global-constructors.html:84:
shouldBe("window.NodeFilter.prototype.isPrototypeOf(nodeFilter)", false);
On 2017/04/27 at 08:49:49, bashi wrote:
> Could you elaborate why removing this is fine? Should we add another
ShouldBe() or is this change already covered by wpt?

Now window.NodeFilter.prototype is undefined, and isPrototypeOf() check is
impossible.
Undefined window.NodeFilter.prototype is tested well in WPT.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:83:
'methods': [] if is_legacy else [method_context(operation)
On 2017/04/27 at 08:49:49, bashi wrote:
> Why do we need to set an empty list when |is_legacy| is true?

Because method_context() crashes due to unsupported 'unsigned short' return
value in NodeFilter.idl.

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:12: {%
if is_legacy %}
On 2017/04/27 at 08:49:49, bashi wrote:
> It seems that we should have a new template file
(legacy_callback_interface.{cpp,h}.tmpl).
> 
> I think you can modify CodeGeneratorV8.generate_interface_code() to select
appropriate template files. You will need to create Jinja2 context before
selecting templates.

IMO, we should not have a separated template. Ideally we should fix
crbug.com/630986, and NodeFilter uses code generated with the normal callback
interface template.

[commit-bot: I haz the power, 2017-04-27 09:42:16]

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

[commit-bot: I haz the power, 2017-04-27 09:42:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Yuki, 2017-04-27 12:18:28]

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

[Yuki, 2017-04-27 12:18:29]

LGTM.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:133: return
interface_object;
nit: Ideally, we shouldn't skip InstallConditionalFeatures below.
I knew that it's okay so far.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/utilities.py:348: match =
re.search(r'callback\s+interface\s+\w+\s*{', file_contents)
On 2017/04/27 08:49:49, bashi wrote:
> Could you add a comment why we search 'const' to detect legacy callback
> interface?

|file_contents| doesn't include any comment, right?

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:37:
TypeErrorConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
Due to the snapshot project, I think that this needs to be
CORE/MODULES_EXPORT'ed.  All callback functions registered to V8 need to be
exported.

peria@?

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:43: //
Legacy callback interface doesn't need prototype.
nit: s/doesn't need prototype/must not have a prototype object/

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:51:
//interfaceTemplate->SetLength({{interface_length}});
On 2017/04/27 08:49:49, bashi wrote:
> Remove?

Should be explicitly set to zero because the spec defines so?

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl:19: class
{{v8_class}} {
nit: final?

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl:22:
static v8::Local<v8::FunctionTemplate> DomTemplate(v8::Isolate*, const
DOMWrapperWorld&);
nit: {{exported}}
due to the snapshot project.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl:23:
static const WrapperTypeInfo wrapperTypeInfo;
nit: {{exported}}
due to the snapshot project.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:83: readonly
attribute NodeFilter nodeFilterAttribute;
If you'd like to test Document.createTreeWalker, you need to make this IDL
operation, not IDL attribute.

If you'd like to test window.NodeFilter, you need ... bashi@?

Anyway, adding |readonly| doesn't make sense to me.

[peria, 2017-04-27 13:14:49]

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:37:
TypeErrorConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
On 2017/04/27 12:18:29, Yuki wrote:
> Due to the snapshot project, I think that this needs to be
> CORE/MODULES_EXPORT'ed.  All callback functions registered to V8 need to be
> exported.
> 
> peria@?

Yes, it should.
The project is still under construction, but I'm happy if this callback function
is an EXPORTed static method of V8Class.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl:22:
static v8::Local<v8::FunctionTemplate> DomTemplate(v8::Isolate*, const
DOMWrapperWorld&);
On 2017/04/27 12:18:29, Yuki wrote:
> nit: {{exported}}
> due to the snapshot project.

It is OK to keep callback interfaces' DomTemplate() not exported.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl:23:
static const WrapperTypeInfo wrapperTypeInfo;
On 2017/04/27 12:18:29, Yuki wrote:
> nit: {{exported}}
> due to the snapshot project.

Yes, please put it.

[bashi, 2017-04-27 23:12:33]

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:83:
'methods': [] if is_legacy else [method_context(operation)
On 2017/04/27 09:15:12, tkent wrote:
> On 2017/04/27 at 08:49:49, bashi wrote:
> > Why do we need to set an empty list when |is_legacy| is true?
> 
> Because method_context() crashes due to unsupported 'unsigned short' return
> value in NodeFilter.idl.

Could you put the error log here? I'd prefer fixing method_context() rather than
adding a workaround here.

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:12: {%
if is_legacy %}
On 2017/04/27 09:15:12, tkent wrote:
> On 2017/04/27 at 08:49:49, bashi wrote:
> > It seems that we should have a new template file
> (legacy_callback_interface.{cpp,h}.tmpl).
> > 
> > I think you can modify CodeGeneratorV8.generate_interface_code() to select
> appropriate template files. You will need to create Jinja2 context before
> selecting templates.
> 
> IMO, we should not have a separated template. Ideally we should fix
> crbug.com/630986, and NodeFilter uses code generated with the normal callback
> interface template.

How much code you are expecting to have overlaps between legacy callback
interface and non legacy callback interface? Seeing this CL I would suspect
there might be only a few things to share between them and there are large
amount of differences (e.g. non legacy callback doesn't need WrapperTypeInfo).

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:83: readonly
attribute NodeFilter nodeFilterAttribute;
On 2017/04/27 12:18:29, Yuki wrote:
> If you'd like to test Document.createTreeWalker, you need to make this IDL
> operation, not IDL attribute.
> 
> If you'd like to test window.NodeFilter, you need ... bashi@?
> 
> Anyway, adding |readonly| doesn't make sense to me.

If we want to check how we generate bindings for legacy callback interface, we
may want to add a callback interface which contains some const fields in this
directory.

[tkent, 2017-04-28 00:09:43]

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:83:
'methods': [] if is_legacy else [method_context(operation)
On 2017/04/27 at 23:12:32, bashi wrote:
> On 2017/04/27 09:15:12, tkent wrote:
> > On 2017/04/27 at 08:49:49, bashi wrote:
> > > Why do we need to set an empty list when |is_legacy| is true?
> > 
> > Because method_context() crashes due to unsupported 'unsigned short' return
> > value in NodeFilter.idl.
> 
> Could you put the error log here? I'd prefer fixing method_context() rather
than adding a workaround here.

Obviously, it's an intentional exception.

def method_context(operation):
    ....
    if idl_type_str not in ['boolean', 'void']:
        raise Exception('We only support callbacks that return boolean or void
values.')

If we add 'unsigned short' here, the method generation part of
callback_interface.cpp.tmpl generates non-buildable cpp files though NodeFilter
doesn't use this code path.

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:12: {%
if is_legacy %}
On 2017/04/27 at 23:12:32, bashi wrote:
> On 2017/04/27 09:15:12, tkent wrote:
> > On 2017/04/27 at 08:49:49, bashi wrote:
> > > It seems that we should have a new template file
> > (legacy_callback_interface.{cpp,h}.tmpl).
> > > 
> > > I think you can modify CodeGeneratorV8.generate_interface_code() to select
> > appropriate template files. You will need to create Jinja2 context before
> > selecting templates.
> > 
> > IMO, we should not have a separated template. Ideally we should fix
> > crbug.com/630986, and NodeFilter uses code generated with the normal
callback
> > interface template.
> 
> How much code you are expecting to have overlaps between legacy callback
interface and non legacy callback interface? Seeing this CL I would suspect
there might be only a few things to share between them and there are large
amount of differences (e.g. non legacy callback doesn't need WrapperTypeInfo).

Well, suppose that:
A: Code amount to support non-legacy callback interface
B: Code amount to support legacy callback interface object; WrapperTypeInfo,
DomTemplate, etc.

A in callback_interface.h.tmpl: 20 lines
B in callback_interface.h.tmpl: 7 lines

A in callback_interface.cpp.tmpl: 62 lines
B in callabck_interface.cpp.tmpl: 54 lines

At this moment NodeFilter needs only B, but it should use A+B in the future. I
guess A in *.cpp.tmpl will be increased to fix crbug.com/630986.

[bashi, 2017-04-28 00:59:09]

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:83:
'methods': [] if is_legacy else [method_context(operation)
On 2017/04/28 00:09:43, tkent wrote:
> On 2017/04/27 at 23:12:32, bashi wrote:
> > On 2017/04/27 09:15:12, tkent wrote:
> > > On 2017/04/27 at 08:49:49, bashi wrote:
> > > > Why do we need to set an empty list when |is_legacy| is true?
> > > 
> > > Because method_context() crashes due to unsupported 'unsigned short'
return
> > > value in NodeFilter.idl.
> > 
> > Could you put the error log here? I'd prefer fixing method_context() rather
> than adding a workaround here.
> 
> Obviously, it's an intentional exception.
> 
> def method_context(operation):
>     ....
>     if idl_type_str not in ['boolean', 'void']:
>         raise Exception('We only support callbacks that return boolean or void
> values.')
> 
> If we add 'unsigned short' here, the method generation part of
> callback_interface.cpp.tmpl generates non-buildable cpp files though
NodeFilter
> doesn't use this code path.

I see. Thank you for the explanation. Given that we don't need to use 'methods'
and need extra info like constants for legacy callback interface (IIUC
NodeFilter is the only instance of legacy callback interface), it would make
sense to have legacy_callback_interface_context() instead of modifying
callback_interface_context().

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:12: {%
if is_legacy %}
On 2017/04/28 00:09:43, tkent wrote:
> On 2017/04/27 at 23:12:32, bashi wrote:
> > On 2017/04/27 09:15:12, tkent wrote:
> > > On 2017/04/27 at 08:49:49, bashi wrote:
> > > > It seems that we should have a new template file
> > > (legacy_callback_interface.{cpp,h}.tmpl).
> > > > 
> > > > I think you can modify CodeGeneratorV8.generate_interface_code() to
select
> > > appropriate template files. You will need to create Jinja2 context before
> > > selecting templates.
> > > 
> > > IMO, we should not have a separated template. Ideally we should fix
> > > crbug.com/630986, and NodeFilter uses code generated with the normal
> callback
> > > interface template.
> > 
> > How much code you are expecting to have overlaps between legacy callback
> interface and non legacy callback interface? Seeing this CL I would suspect
> there might be only a few things to share between them and there are large
> amount of differences (e.g. non legacy callback doesn't need WrapperTypeInfo).
> 
> Well, suppose that:
> A: Code amount to support non-legacy callback interface
> B: Code amount to support legacy callback interface object; WrapperTypeInfo,
> DomTemplate, etc.
> 
> A in callback_interface.h.tmpl: 20 lines
> B in callback_interface.h.tmpl: 7 lines
> 
> A in callback_interface.cpp.tmpl: 62 lines
> B in callabck_interface.cpp.tmpl: 54 lines
> 
> At this moment NodeFilter needs only B, but it should use A+B in the future. I
> guess A in *.cpp.tmpl will be increased to fix crbug.com/630986.
> 

Still I'd prefer having separate templates. Rationale:
- The number of lines wouldn't be a good metric for code health. Readability
overs writing cost. This CL doesn't share almost all code between legacy and non
legacy callback interface.
- If there are dups we can use Jinja2 macros.
- I'm still not sure how many code we will share between legacy and non legacy
callback interface. If they should use the same template because both hold a v8
value, should we use the same template for callback function as well? Until we
have clear signal I'd prefer keep them separate things.

[tkent, 2017-04-28 06:13:37]

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.cpp:133: return
interface_object;
On 2017/04/27 at 12:18:28, Yuki wrote:
> nit: Ideally, we shouldn't skip InstallConditionalFeatures below.
> I knew that it's okay so far.

Done.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/utilities.py (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/utilities.py:348: match =
re.search(r'callback\s+interface\s+\w+\s*{', file_contents)
On 2017/04/27 at 12:18:29, Yuki wrote:
> On 2017/04/27 08:49:49, bashi wrote:
> > Could you add a comment why we search 'const' to detect legacy callback
> > interface?
> 
> |file_contents| doesn't include any comment, right?

Done. Added a comment.

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

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:12: {%
if is_legacy %}
On 2017/04/28 at 00:59:09, bashi wrote:
> Still I'd prefer having separate templates. Rationale:

ok, done in Patch Set 2.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:13: //
Suppress warning: global constructors, because struct WrapperTypeInfo is trivial
On 2017/04/27 at 08:49:49, bashi wrote:
> nit: Wrap to 80 columns?

Done.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:37:
TypeErrorConstructorCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
On 2017/04/27 at 13:14:49, peria wrote:
> On 2017/04/27 12:18:29, Yuki wrote:
> > Due to the snapshot project, I think that this needs to be
> > CORE/MODULES_EXPORT'ed.  All callback functions registered to V8 need to be
> > exported.
> > 
> > peria@?
> 
> Yes, it should.
> The project is still under construction, but I'm happy if this callback
function
> is an EXPORTed static method of V8Class.

Done.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:43: //
Legacy callback interface doesn't need prototype.
On 2017/04/27 at 12:18:29, Yuki wrote:
> nit: s/doesn't need prototype/must not have a prototype object/

Done.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.cpp.tmpl:51:
//interfaceTemplate->SetLength({{interface_length}});
On 2017/04/27 at 12:18:29, Yuki wrote:
> On 2017/04/27 08:49:49, bashi wrote:
> > Remove?
> 
> Should be explicitly set to zero because the spec defines so?

Done.  Set 0 explicitly.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl
(right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl:19: class
{{v8_class}} {
On 2017/04/27 at 12:18:29, Yuki wrote:
> nit: final?

Done.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/callback_interface.h.tmpl:23:
static const WrapperTypeInfo wrapperTypeInfo;
On 2017/04/27 at 13:14:49, peria wrote:
> On 2017/04/27 12:18:29, Yuki wrote:
> > nit: {{exported}}
> > due to the snapshot project.
> 
> Yes, please put it.

Done.

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl (right):

https://codereview.chromium.org/2837923003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/tests/idls/core/TestObject.idl:83: readonly
attribute NodeFilter nodeFilterAttribute;
On 2017/04/27 at 23:12:32, bashi wrote:
> On 2017/04/27 12:18:29, Yuki wrote:
> > If you'd like to test Document.createTreeWalker, you need to make this IDL
> > operation, not IDL attribute.
> > 
> > If you'd like to test window.NodeFilter, you need ... bashi@?
> > 
> > Anyway, adding |readonly| doesn't make sense to me.
> 
> If we want to check how we generate bindings for legacy callback interface, we
may want to add a callback interface which contains some const fields in this
directory.

The intention was to skip to generate NodeFilter attribute setter because we
don't need to support it. It's not a serious issue, and I reverted this.

Also, I added TestLegacyCallbackInterface.idl and its results.

[tkent, 2017-04-28 06:13:50]

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

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

Dry run: CQ is trying da patch.

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

[bashi, 2017-04-28 06:28:34]

Thank you so much for addressing comments! lgtm on my side.

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator_v8.py:184: if
interface.is_callback and len(interface.constants) > 0:
Could you add a comment why we use len(interface.constants) to detect legacy
callback interface?

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
(right):

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:97: #
TODO(tkent): Fix crbug.com/630986, and add 'methods'.
Feel free to change to TODO(bashi)

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:101:
'constants': [constant_context(constant, callback_interface)
nit: alphabetical order

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/NodeFilter.idl (right):

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/NodeFilter.idl:48: // TODO(tkent): Fix
crbug.com/630986
Also feel free to assign this to me if fixing 630986 is beyond fixing
NodeFilter.

[tkent, 2017-04-28 06:35:13]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Source/bindings/tests/idls/core/TestObject.idl
  Add 'readonly' because non-readonly NodeFilter attribute doesn't exist in the
  real world.

BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.
  Because NodeFilter can't use generated methods for callback interface, this CL
  introduces templates dedicated for legacy callback interface.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Source/bindings/tests/idls/core/TestLegacyCallbackInterface.idl
  Add it for quick check of legacy callback interface generation.


BUG=591919
==========

[bashi, 2017-04-28 06:39:05]

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

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/legacy_callback_interface.cpp.tmpl:1:
{% filter format_blink_cpp_source_code %}
Sorry forgot to mention that we need to add
legacy_callback_interface.{cpp,h}.tmpl to
third_party/WebKit/Source/bindings/templates/templates.gni.

[tkent, 2017-04-28 07:29:45]

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

[commit-bot: I haz the power, 2017-04-28 07:30:08]

Dry run: CQ is trying da patch.

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

[tkent, 2017-04-28 07:38:19]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.
  Because NodeFilter can't use generated methods for callback interface, this CL
  introduces templates dedicated for legacy callback interface.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Source/bindings/tests/idls/core/TestLegacyCallbackInterface.idl
  Add it for quick check of legacy callback interface generation.


BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Add new legacy_callback_interface.*.tmpl
  Because NodeFilter can't use generated methods for callback interface for now.

* Source/bindings/tests/idls/core/TestLegacyCallbackInterface.idl
  Add it for quick check of legacy callback interface generation.


BUG=591919
==========

[tkent, 2017-04-28 09:20:59]

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

[commit-bot: I haz the power, 2017-04-28 09:21:23]

Dry run: CQ is trying da patch.

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

[tkent, 2017-04-28 09:21:25]

Patchset #3 (id:60001) has been deleted

[tkent, 2017-04-28 09:22:02]

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/code_generator_v8.py (right):

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/code_generator_v8.py:184: if
interface.is_callback and len(interface.constants) > 0:
On 2017/04/28 at 06:28:34, bashi wrote:
> Could you add a comment why we use len(interface.constants) to detect legacy
callback interface?

Done.

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
(right):

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py:101:
'constants': [constant_context(constant, callback_interface)
On 2017/04/28 at 06:28:34, bashi wrote:
> nit: alphabetical order

Done.

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

https://codereview.chromium.org/2837923003/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/templates/legacy_callback_interface.cpp.tmpl:1:
{% filter format_blink_cpp_source_code %}
On 2017/04/28 at 06:39:05, bashi wrote:
> Sorry forgot to mention that we need to add
legacy_callback_interface.{cpp,h}.tmpl to
third_party/WebKit/Source/bindings/templates/templates.gni.

Done.

[Yuki, 2017-04-28 10:34:12]

LGTM.

[commit-bot: I haz the power, 2017-04-28 11:19:08]

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

[commit-bot: I haz the power, 2017-04-28 11:19:10]

Dry run: This issue passed the CQ dry run.

[tkent, 2017-04-28 11:59:07]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-04-28 11:59:08]

The patchset sent to the CQ was uploaded after l-g-t-m from bashi@chromium.org
Link to the patchset: https://codereview.chromium.org/2837923003/#ps80001
(title: "templates.gni, etc.")

[commit-bot: I haz the power, 2017-04-28 11:59:26]

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-28 12:03:42]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1493380747674330,
"parent_rev": "c41d3c785c5179a38e54c6c80ea63d56cc7cfd12", "commit_rev":
"bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0"}

[commit-bot: I haz the power, 2017-04-28 12:03:56]

Description was changed from

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Add new legacy_callback_interface.*.tmpl
  Because NodeFilter can't use generated methods for callback interface for now.

* Source/bindings/tests/idls/core/TestLegacyCallbackInterface.idl
  Add it for quick check of legacy callback interface generation.


BUG=591919
==========

to

==========
Make NodeFilter a legacy callback interface.

https://heycam.github.io/webidl/#legacy-callback-interface-object
Legacy callback interface needs an interface object, but it has no instances and
no prototype.

This CL fixes 19 failures in external/wpt/dom/.

Implementation:

* This CL adds WrapperTypeInfo for NodeFilter, but we never make wrappers for
  NodeFilter.

* Add kWrapperTypeNoPrototype
  window.NodeFilter should not have a prototype, but it should be a constructor.
  V8PerContextData::ConstructorForTypeSlowCase() rejected to return a
constructor
  if the type had no prototype. This CL introduces kWrapperTypeNoPrototype to
  avoid this issue.

* Add new legacy_callback_interface.*.tmpl
  Because NodeFilter can't use generated methods for callback interface for now.

* Source/bindings/tests/idls/core/TestLegacyCallbackInterface.idl
  Add it for quick check of legacy callback interface generation.


BUG=591919

Review-Url: https://codereview.chromium.org/2837923003
Cr-Commit-Position: refs/heads/master@{#467963}
Committed:
https://chromium.googlesource.com/chromium/src/+/bc4bd9dfb5ff6b2f62d3985e3596...
==========

[commit-bot: I haz the power, 2017-04-28 12:03:58]

Committed patchset #3 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/bc4bd9dfb5ff6b2f62d3985e3596...

[haraken, 2017-04-28 13:44:31]

LGTM