New issue
Advanced search Search tips

Issue 591919 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug

Blocking:
issue 651744



Sign in to add a comment

NodeFilter should be a legacy callback interface

Project Member Reported by tkent@chromium.org, Mar 4 2016

Issue description

Comment 1 by phil...@opera.com, Mar 4 2016

The description of https://codereview.chromium.org/1493023004 sums up my thinking about this issue. The use counters haven't reached stable yet, but it already looks like making it a plain callback function is out of the question:
https://www.chromestatus.com/metrics/feature/timeline/popularity/1059
https://www.chromestatus.com/metrics/feature/timeline/popularity/1060

That's a shame, because this doesn't API doesn't make sense, see the third paragraph of the CL description.

I think that one possible fix would be split NodeFilter into a plain NodeFilter interface with only the contants and a NodeFilterCallback with only acceptNode. I'll probably come back to this issue once the use counter data from stable is in.

Comment 2 by phil...@opera.com, Mar 8 2016

Cc: rbyers@chromium.org
While chromstatus.com is not updating, Rick helped me check usage from the stable channel internally:
NodeFilterIsFunction - 0.82%
NodeFilterIsObject - 0.039%

As expected, making this a plain callback function is out of the question, the current behavior needs to be preserved. I'll file a spec bug.

Comment 4 by phil...@opera.com, Mar 8 2016

And I've closed the issue already. At this point it doesn't seem like there's any meaningful simplification to be done here. The special thing here is the NodeFilter interface object. I haven't dug into all of the details, but WebIDL seems to define it detail, with the most interesting bits in http://heycam.github.io/webidl/#es-interfaces

Comment 5 by phil...@opera.com, Mar 8 2016

Filed another issue that needs to be sorted out before implementing:
https://github.com/heycam/webidl/issues/96

Comment 6 by tkent@chromium.org, Oct 17 2016

Blocking: 651744

Comment 7 by foolip@chromium.org, Nov 11 2016

Cc: foolip@chromium.org

Comment 8 by foolip@chromium.org, Nov 11 2016

Cc: -phil...@opera.com

Comment 9 by cvrebert@google.com, Feb 14 2017

IIUC, this is also causing a bunch of failures in
http://w3c-test.org/dom/traversal/NodeIterator.html

Comment 10 by tkent@chromium.org, Mar 28 2017

Summary: NodeFilter should be a legacy callback interface (was: NodeFilter should be a callback interface)
https://heycam.github.io/webidl/#legacy-callback-interface-object

Comment 11 by tkent@chromium.org, Mar 28 2017

Labels: Hotlist-Interop

Comment 12 by tkent@chromium.org, Apr 25 2017

Owner: tkent@chromium.org
Status: Started (was: Available)

Comment 13 by tkent@chromium.org, Apr 26 2017

I'll fix the followings separately.
 - .filter should return values specified to createNodeIterator()/createTreeWalker()

 - NodeFilter should be a legacy callback interface

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 27 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/772fbff236bdf680788846586a34e9f4d074b2c5

commit 772fbff236bdf680788846586a34e9f4d074b2c5
Author: tkent <tkent@chromium.org>
Date: Thu Apr 27 06:04:18 2017

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}

[delete] https://crrev.com/4dc75a6309ee25e843244e51eb42623230193ba0/third_party/WebKit/LayoutTests/external/wpt/dom/nodes/Document-createTreeWalker-expected.txt
[delete] https://crrev.com/4dc75a6309ee25e843244e51eb42623230193ba0/third_party/WebKit/LayoutTests/external/wpt/dom/traversal/NodeIterator-expected.txt
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/LayoutTests/external/wpt/dom/traversal/TreeWalker-expected.txt
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/LayoutTests/fast/dom/constants-expected.txt
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/LayoutTests/fast/dom/constants.html
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/LayoutTests/fast/dom/global-constructors.html
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/LayoutTests/fast/dom/node-filter-use-counters.html
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/bindings/core/v8/ToV8ForCore.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/bindings/core/v8/V8BindingForCore.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/bindings/core/v8/V8NodeFilterCondition.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/bindings/core/v8/V8PrivateProperty.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/bindings/scripts/v8_types.py
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/BUILD.gn
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/Document.h
[delete] https://crrev.com/4dc75a6309ee25e843244e51eb42623230193ba0/third_party/WebKit/Source/core/dom/NodeFilter.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/NodeFilter.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/NodeFilterCondition.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/NodeIterator.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/NodeIterator.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/NodeIteratorBase.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/NodeIteratorBase.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/TreeWalker.cpp
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/core/dom/TreeWalker.h
[modify] https://crrev.com/772fbff236bdf680788846586a34e9f4d074b2c5/third_party/WebKit/Source/platform/heap/WrapperVisitor.h

Project Member

Comment 17 by bugdroid1@chromium.org, Apr 28 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0

commit bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0
Author: tkent <tkent@chromium.org>
Date: Fri Apr 28 12:03:39 2017

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}

[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/LayoutTests/external/wpt/dom/interfaces-expected.txt
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/LayoutTests/fast/dom/global-constructors.html
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/LayoutTests/platform/mac/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/LayoutTests/platform/win/virtual/stable/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/LayoutTests/virtual/service-worker-navigation-preload-disabled/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/LayoutTests/webexposed/global-interface-listing-expected.txt
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/scripts/code_generator_v8.py
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/scripts/generate_global_constructors.py
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/scripts/utilities.py
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/scripts/v8_callback_interface.py
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/scripts/v8_types.py
[add] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/templates/legacy_callback_interface.cpp.tmpl
[add] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/templates/legacy_callback_interface.h.tmpl
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/templates/templates.gni
[add] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/tests/idls/core/TestLegacyCallbackInterface.idl
[add] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/tests/results/core/V8TestLegacyCallbackInterface.cpp
[add] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/tests/results/core/V8TestLegacyCallbackInterface.h
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/core/dom/NodeFilter.h
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/core/dom/NodeFilter.idl
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/platform/bindings/V8PerContextData.cpp
[modify] https://crrev.com/bc4bd9dfb5ff6b2f62d3985e3596df27a9bacec0/third_party/WebKit/Source/platform/bindings/WrapperTypeInfo.h

Comment 18 by tkent@chromium.org, Apr 30 2017

Labels: M-60
Status: Fixed (was: Started)

Sign in to add a comment