DOM: NodeIterator.filter and TreeWalker.filter should return values which were specified to createN…

DOM: NodeIterator.filter and TreeWalker.filter should return values which were specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

The core part of this CL is CPP_VALUE_TO_V8_VALUE in v8_types.py. We return a v8
value stored in V8NodeFilterCondition.

Because we don't need to wrap values by NodeFilter, this CL has simplification like:

- Cleanup of indirection ownership;
 Old: NodeIteratorBase -> NodeFilter -> V8NodeFilterCondition as NodeFilterCondition
 New: NodeIteratorBase -> V8NodeFilterCondition

- Removal of a V8PrivateProperty
 It can be replaced with TraceWrapper.


This CL fixes 1,106 failures in external/wpt/dom/.

BUG=591919, 715418
Review-Url: https://codereview.chromium.org/2840163002
Cr-Commit-Position: refs/heads/master@{#467599}
Committed: https://chromium.googlesource.com/chromium/src/+/772fbff236bdf680788846586a34e9f4d074b2c5

[tkent, 2017-04-26 05:18:53]

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

[commit-bot: I haz the power, 2017-04-26 05:19:17]

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-26 05:27:22]

Description was changed from

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

This CL fixes 1,106 failures in external/wpt/dom/.

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

to

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM
specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

This CL fixes 1,106 failures in external/wpt/dom/.

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

[tkent, 2017-04-26 05:33:15]

Description was changed from

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM
specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

This CL fixes 1,106 failures in external/wpt/dom/.

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

to

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM
specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

The core part of this CL is CPP_VALUE_TO_V8_VALUE in v8_types.py. We return a v8
value stored in V8NodeFilterCondition.

Because we don't need to wrap values by NodeFilter, this CL has simplification
like:

- Cleanup of indirection ownership;
 Old: NodeIteratorBase -> NodeFilter -> V8NodeFilterCondition as
NodeFilterCondition
 New: NodeIteratorBase -> V8NodeFilterCondition

- Removal of a V8PrivateProperty
 It can be replaced with TraceWrapper.


This CL fixes 1,106 failures in external/wpt/dom/.

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

[tkent, 2017-04-26 05:45:53]

Description was changed from

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM
specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

The core part of this CL is CPP_VALUE_TO_V8_VALUE in v8_types.py. We return a v8
value stored in V8NodeFilterCondition.

Because we don't need to wrap values by NodeFilter, this CL has simplification
like:

- Cleanup of indirection ownership;
 Old: NodeIteratorBase -> NodeFilter -> V8NodeFilterCondition as
NodeFilterCondition
 New: NodeIteratorBase -> V8NodeFilterCondition

- Removal of a V8PrivateProperty
 It can be replaced with TraceWrapper.


This CL fixes 1,106 failures in external/wpt/dom/.

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

to

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM
specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

The core part of this CL is CPP_VALUE_TO_V8_VALUE in v8_types.py. We return a v8
value stored in V8NodeFilterCondition.

Because we don't need to wrap values by NodeFilter, this CL has simplification
like:

- Cleanup of indirection ownership;
 Old: NodeIteratorBase -> NodeFilter -> V8NodeFilterCondition as
NodeFilterCondition
 New: NodeIteratorBase -> V8NodeFilterCondition

- Removal of a V8PrivateProperty
 It can be replaced with TraceWrapper.


This CL fixes 1,106 failures in external/wpt/dom/.

BUG=591919, 715418
==========

[tkent, 2017-04-26 06:35:35]

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

[tkent, 2017-04-26 06:35:35]

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

This is the first step to support 'NodeFilter as a legacy callback interface'.
We stop creating NodeFilter instances by this CL.

[peria, 2017-04-26 06:50:11]

Cool!
LGTM from my side, but please wait bashi@'s look.

[commit-bot: I haz the power, 2017-04-26 07:16:44]

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

[commit-bot: I haz the power, 2017-04-26 07:16:45]

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

[Yuki, 2017-04-26 08:36:47]

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

[Yuki, 2017-04-26 08:36:49]

LGTM.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp
(right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp:58:
visitor->TraceWrappers(filter_.Cast<v8::Value>());
nit: No need of casting.
  visitor->TraceWrappers(filter_);
should work.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_types.py:953: 'NodeFilter': (
Just a comment,

If this is a common pattern for callback interfaces, we might want to do the
following:

class NodeFilter : public CallbackInterface { ... };

v8::Local<v8::Value> ToV8(CallbackInterface* callback_interface) {
  ...
}

For non-callback IDL interfaces, we have ScriptWrappable and
ToV8(ScriptWrappable*).  ToV8() also supports built-in types such as int, bool,
String, etc.  In general, ToV8() is responsible to convert a value of Blink's
type to the corresponding value of V8's type.  It's not good to introduce this
kind of special cases.

Even if this is not a common pattern for callback interfaces, still it could be
better to define ToV8(V8NodeFilterCondition*).

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/NodeFilter.h (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/NodeFilter.h:36: // V8 value in Blink.
nit: Probably we may want a TODO comment to change
"interface NodeFilter" to "callback interface NodeFilter" in NodeFilter.idl.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/NodeFilter.h:70: unsigned acceptNode(Node*,
ExceptionState&) const { return kFilterReject; }
Given that an instance of NodeFilter is never created, how can we reach here? 
NOTREACHED()?

Given that this is unreachable, taking an ExceptionState is weird.  You can drop
[RaisesException] from NodeFilter.idl (optional, you can do this in a follow-up
CL).

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/NodeFilter.h:75: explicit NodeFilter();
nit: s/explicit //
nit: "= delete" is nice to have.

[haraken, 2017-04-26 09:03:34]

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

[haraken, 2017-04-26 09:03:35]

LGTM

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp
(right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp:52:
filter_.Clear();

Nit: I'd prefer doing:

  if (!filter.IsEmpty() && filter->IsObject())
    filter_.Set(...)

rather than clearing after unconditionally setting.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h:71:
TraceWrapperV8Reference<v8::Value> filter_;

v8::Value => v8::Object ? (or probably v8::Function)

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_types.py:953: 'NodeFilter': (
On 2017/04/26 08:36:48, Yuki wrote:
> Just a comment,
> 
> If this is a common pattern for callback interfaces, we might want to do the
> following:
> 
> class NodeFilter : public CallbackInterface { ... };
> 
> v8::Local<v8::Value> ToV8(CallbackInterface* callback_interface) {
>   ...
> }
> 
> For non-callback IDL interfaces, we have ScriptWrappable and
> ToV8(ScriptWrappable*).  ToV8() also supports built-in types such as int,
bool,
> String, etc.  In general, ToV8() is responsible to convert a value of Blink's
> type to the corresponding value of V8's type.  It's not good to introduce this
> kind of special cases.
> 
> Even if this is not a common pattern for callback interfaces, still it could
be
> better to define ToV8(V8NodeFilterCondition*).

+1

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/NodeFilter.h (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/NodeFilter.h:36: // V8 value in Blink.
On 2017/04/26 08:36:48, Yuki wrote:
> nit: Probably we may want a TODO comment to change
> "interface NodeFilter" to "callback interface NodeFilter" in NodeFilter.idl.

BTW why do we need a special handling for NodeFilter (i.e.,
V8NodeFilterCondition) different from other callback interfaces?

[bashi, 2017-04-26 23:18:20]

lgtm on my side.

[tkent, 2017-04-26 23:25:09]

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

[tkent, 2017-04-26 23:25:33]

tkent@chromium.org changed reviewers:
- haraken@chromium.org

[tkent, 2017-04-26 23:25:34]

Thank you for many LGTMs and feedbacks :)

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp
(right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp:52:
filter_.Clear();
On 2017/04/26 at 09:03:35, haraken wrote:
> Nit: I'd prefer doing:
> 
>   if (!filter.IsEmpty() && filter->IsObject())
>     filter_.Set(...)
> 
> rather than clearing after unconditionally setting.

Done.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp:58:
visitor->TraceWrappers(filter_.Cast<v8::Value>());
On 2017/04/26 at 08:36:48, Yuki wrote:
> nit: No need of casting.
>   visitor->TraceWrappers(filter_);
> should work.

Yeah, it worked.  However Patch Set 2 changed filter_ to
TraceWrapperV8Reference<v8::Object>, and now this Cast<> is necessary.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h:71:
TraceWrapperV8Reference<v8::Value> filter_;
On 2017/04/26 at 09:03:35, haraken wrote:
> v8::Value => v8::Object ? (or probably v8::Function)

Done.  It should be v8::Object, not v8::Function. It's either an object with
"acceptNode" method, or a function.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_types.py:953: 'NodeFilter': (
On 2017/04/26 at 08:36:48, Yuki wrote:
> Just a comment,
> 
> If this is a common pattern for callback interfaces, we might want to do the
following:
> 
> class NodeFilter : public CallbackInterface { ... };
> 
> v8::Local<v8::Value> ToV8(CallbackInterface* callback_interface) {
>   ...
> }
> 
> For non-callback IDL interfaces, we have ScriptWrappable and
ToV8(ScriptWrappable*).  ToV8() also supports built-in types such as int, bool,
String, etc.  In general, ToV8() is responsible to convert a value of Blink's
type to the corresponding value of V8's type.  It's not good to introduce this
kind of special cases.
> 
> Even if this is not a common pattern for callback interfaces, still it could
be better to define ToV8(V8NodeFilterCondition*).

Done. Added ToV8(V8NodeFilterCondition*). Patch Set 2 added it to
V8NodeFilterCondition.h. Should it be added to ToV8ForCore.h?

It might be possible to share NodeFilter as V8NodeFilterCondition and
EventHandler as V8AbstractEventListener.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/dom/NodeFilter.h (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/NodeFilter.h:36: // V8 value in Blink.
On 2017/04/26 at 08:36:48, Yuki wrote:
> nit: Probably we may want a TODO comment to change
> "interface NodeFilter" to "callback interface NodeFilter" in NodeFilter.idl.

We already have it.

> BTW why do we need a special handling for NodeFilter (i.e.,
V8NodeFilterCondition) different from other callback interfaces?

NodeFilter is special because it's a 'legacy callback interface'.
https://heycam.github.io/webidl/#legacy-callback-interface-object

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/NodeFilter.h:70: unsigned acceptNode(Node*,
ExceptionState&) const { return kFilterReject; }
On 2017/04/26 at 08:36:48, Yuki wrote:
> Given that an instance of NodeFilter is never created, how can we reach here? 
NOTREACHED()?
> 
> Given that this is unreachable, taking an ExceptionState is weird.  You can
drop [RaisesException] from NodeFilter.idl (optional, you can do this in a
follow-up CL).

Right, added NOTREACHED().
The C++ function itself will be removed by a following CL.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/dom/NodeFilter.h:75: explicit NodeFilter();
On 2017/04/26 at 08:36:48, Yuki wrote:
> nit: s/explicit //
> nit: "= delete" is nice to have.

Done.

[commit-bot: I haz the power, 2017-04-26 23:26:16]

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-26 23:31:36]

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

[commit-bot: I haz the power, 2017-04-26 23:33:06]

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 00:02:38]

Patchset #2 (id:20001) has been deleted

[commit-bot: I haz the power, 2017-04-27 01:59:51]

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

[commit-bot: I haz the power, 2017-04-27 01:59:51]

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

[tkent, 2017-04-27 02:06:38]

> > BTW why do we need a special handling for NodeFilter (i.e.,
V8NodeFilterCondition) different from other callback interfaces?
> 
> NodeFilter is special because it's a 'legacy callback interface'.
> https://heycam.github.io/webidl/#legacy-callback-interface-object

Also, NodeFilter can't adopt to the current generated code for callback
interface due to crbug.com/630986.  The current NodeFilter implementation
supports object.acceptNode() case, but the generated code doesn't.

[haraken, 2017-04-27 02:07:36]

On 2017/04/27 02:06:38, tkent wrote:
> > > BTW why do we need a special handling for NodeFilter (i.e.,
> V8NodeFilterCondition) different from other callback interfaces?
> > 
> > NodeFilter is special because it's a 'legacy callback interface'.
> > https://heycam.github.io/webidl/#legacy-callback-interface-object
> 
> Also, NodeFilter can't adopt to the current generated code for callback
> interface due to crbug.com/630986.  The current NodeFilter implementation
> supports object.acceptNode() case, but the generated code doesn't.

Makes sense.

[Yuki, 2017-04-27 02:58:45]

LGTM.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp
(right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp:58:
visitor->TraceWrappers(filter_.Cast<v8::Value>());
On 2017/04/26 23:25:34, tkent wrote:
> On 2017/04/26 at 08:36:48, Yuki wrote:
> > nit: No need of casting.
> >   visitor->TraceWrappers(filter_);
> > should work.
> 
> Yeah, it worked.  However Patch Set 2 changed filter_ to
> TraceWrapperV8Reference<v8::Object>, and now this Cast<> is necessary.

That's true, and this CL itself LGTM, but it's quite strange I think.

We should add
  template <typename T>
  TraceWrappers(const TraceWrapperV8Reference<T>&) const;
in WrapperVisitor.

You can do this in a separate CL, or add a TODO(mlippautz) comment.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_types.py:953: 'NodeFilter': (
On 2017/04/26 23:25:34, tkent wrote:
> Done. Added ToV8(V8NodeFilterCondition*). Patch Set 2 added it to
> V8NodeFilterCondition.h. Should it be added to ToV8ForCore.h?

Thanks for doing this.  Yes, please add ToV8 into ToV8ForCore.h.
All ToV8 should be defined in ToV8.h, ToV8ForCore.h or ToV8ForModules.h.

Also, could you follow the existing convention?
v8::Local<v8::Value> ToV8(T* value, v8::Local<v8::Object> creation_context,
v8::Isolate* isolate);
I knew that you don't need |creation_context|, but it makes more chance for the
code generator to treat them uniformly.

https://codereview.chromium.org/2840163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h (right):

https://codereview.chromium.org/2840163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h:61: unsigned
AcceptNode(Node*, ExceptionState&) const override;
Our new convention is to use |fooBarBaz| style for IDL attributes / operations. 
Probably, we should rename this to |acceptNode|, although this class doesn't
inherit from ScriptWrappable.

[tkent, 2017-04-27 03:30:41]

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

[commit-bot: I haz the power, 2017-04-27 03:31:23]

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-27 05:57:01]

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

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

Dry run: This issue passed the CQ dry run.

[tkent, 2017-04-27 05:58:59]

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

[tkent, 2017-04-27 05:59:00]

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp
(right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp:58:
visitor->TraceWrappers(filter_.Cast<v8::Value>());
On 2017/04/27 at 02:58:45, Yuki wrote:
> On 2017/04/26 23:25:34, tkent wrote:
> > On 2017/04/26 at 08:36:48, Yuki wrote:
> > > nit: No need of casting.
> > >   visitor->TraceWrappers(filter_);
> > > should work.
> > 
> > Yeah, it worked.  However Patch Set 2 changed filter_ to
> > TraceWrapperV8Reference<v8::Object>, and now this Cast<> is necessary.
> 
> That's true, and this CL itself LGTM, but it's quite strange I think.
> 
> We should add
>   template <typename T>
>   TraceWrappers(const TraceWrapperV8Reference<T>&) const;
> in WrapperVisitor.
> 
> You can do this in a separate CL, or add a TODO(mlippautz) comment.

Done.  Added TODO() to WrapperVisitor.h.

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
File third_party/WebKit/Source/bindings/scripts/v8_types.py (right):

https://codereview.chromium.org/2840163002/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/scripts/v8_types.py:953: 'NodeFilter': (
On 2017/04/27 at 02:58:45, Yuki wrote:
> On 2017/04/26 23:25:34, tkent wrote:
> > Done. Added ToV8(V8NodeFilterCondition*). Patch Set 2 added it to
> > V8NodeFilterCondition.h. Should it be added to ToV8ForCore.h?
> 
> Thanks for doing this.  Yes, please add ToV8 into ToV8ForCore.h.
> All ToV8 should be defined in ToV8.h, ToV8ForCore.h or ToV8ForModules.h.
> 
> Also, could you follow the existing convention?
> v8::Local<v8::Value> ToV8(T* value, v8::Local<v8::Object> creation_context,
v8::Isolate* isolate);
> I knew that you don't need |creation_context|, but it makes more chance for
the code generator to treat them uniformly.

Done.

https://codereview.chromium.org/2840163002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h (right):

https://codereview.chromium.org/2840163002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h:61: unsigned
AcceptNode(Node*, ExceptionState&) const override;
On 2017/04/27 at 02:58:45, Yuki wrote:
> Our new convention is to use |fooBarBaz| style for IDL attributes /
operations.  Probably, we should rename this to |acceptNode|, although this
class doesn't inherit from ScriptWrappable.

Done.

[tkent, 2017-04-27 05:59:10]

The CQ bit was checked by tkent@chromium.org

[tkent, 2017-04-27 05:59:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, peria@chromium.org, bashi@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2840163002/#ps60001
(title: "Move ToV8() to ToV8ForCore.h, etc.")

[commit-bot: I haz the power, 2017-04-27 05:59:23]

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-27 06:04:21]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1493272750583100,
"parent_rev": "4dc75a6309ee25e843244e51eb42623230193ba0", "commit_rev":
"772fbff236bdf680788846586a34e9f4d074b2c5"}

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

Description was changed from

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM
specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

The core part of this CL is CPP_VALUE_TO_V8_VALUE in v8_types.py. We return a v8
value stored in V8NodeFilterCondition.

Because we don't need to wrap values by NodeFilter, this CL has simplification
like:

- Cleanup of indirection ownership;
 Old: NodeIteratorBase -> NodeFilter -> V8NodeFilterCondition as
NodeFilterCondition
 New: NodeIteratorBase -> V8NodeFilterCondition

- Removal of a V8PrivateProperty
 It can be replaced with TraceWrapper.


This CL fixes 1,106 failures in external/wpt/dom/.

BUG=591919, 715418
==========

to

==========
DOM: NodeIterator.filter and TreeWalker.filter should return values which were
specified to createNodeIterator/createTreeWalker.

We used to return values wrapped by NodeIterator interface. The DOM
specification
doesn't define such behavior, and Edge, Firefox, and Safari do not.

The core part of this CL is CPP_VALUE_TO_V8_VALUE in v8_types.py. We return a v8
value stored in V8NodeFilterCondition.

Because we don't need to wrap values by NodeFilter, this CL has simplification
like:

- Cleanup of indirection ownership;
 Old: NodeIteratorBase -> NodeFilter -> V8NodeFilterCondition as
NodeFilterCondition
 New: NodeIteratorBase -> V8NodeFilterCondition

- Removal of a V8PrivateProperty
 It can be replaced with TraceWrapper.


This CL fixes 1,106 failures in external/wpt/dom/.

BUG=591919, 715418

Review-Url: https://codereview.chromium.org/2840163002
Cr-Commit-Position: refs/heads/master@{#467599}
Committed:
https://chromium.googlesource.com/chromium/src/+/772fbff236bdf680788846586a34...
==========

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

Committed patchset #3 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/772fbff236bdf680788846586a34...