Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The NodeFilter callback interface doesn't make sense #186

Closed
foolip opened this issue Mar 8, 2016 · 6 comments
Closed

The NodeFilter callback interface doesn't make sense #186

foolip opened this issue Mar 8, 2016 · 6 comments

Comments

@foolip
Copy link
Member

foolip commented Mar 8, 2016

https://dom.spec.whatwg.org/#interface-nodefilter

Quoting a Blink commit message I wrote:

NodeFilter is a callback interface in the spec, but a plain interface in
Blink. It's the only callback interface that also has attributes, so
that there must be a NodeFilter attribute on the global object. In order
to make NodeFilter a callback interface per spec, the bindings generator
would need new code to generate that object.

If it's possible to make the createNodeIterator() and createTreeWalker()
filter arguments callback functions instead of callback interfaces, it
looks like this could all be simplified significantly. NodeFilter would
then remain as a plain interface with only the const attributes.

There is also a minor incompatiblity with Gecko related to NodeFilter.
Blink always wraps the function or object inside a new object which is
instanceof NodeFilter, and NodeIterator.prototype.filter returns this
object. Gecko, on the other hand, returns the same object thas was
passed in to createNodeIterator(), and instanceof NodeFilter throws a
TypeError.

As it turns out, the usage was for too high to consider such changes. Quoting a Chromium issue comment:

While chromstatus.com is not updating, @RByers 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.

So, now the trouble is that this doesn't make much sense and I wonder if it could be tweaked somehow. Is what Gecko does how this is intended to work?

@annevk
Copy link
Member

annevk commented Mar 8, 2016

I'm not aware of any issues with the specification. I'm aware that this is the most special of all callback interfaces though.

@foolip
Copy link
Member Author

foolip commented Mar 8, 2016

Sigh. I've done some more testing in Gecko and Edge and looked closer at what the WebIDL spec says. I was honestly hoping to find a big mess of some sort, but actually it looks looks pretty well defined. That instanceof NodeFilter throws is explained by the fact that the NodeFilter interface object doesn't have a prototype, I think.

It doesn't really seem like this allows for any meaningful simplification. I had the idea of splitting acceptNode out from NodeFilter so that there would no longer be any callback interfaces that also have interface objects, but the interface object for what would remain of NodeFilter would still be just as special.

Will comment on Chromium issue that implementing this as current specified seems like the best path forward.

@foolip foolip closed this as completed Mar 8, 2016
@foolip
Copy link
Member Author

foolip commented Mar 8, 2016

WebIDL says "The internal [[Prototype]] property of an interface object for a callback interface must be the Object.prototype object."

I think that means that Object.getPrototypeOf(NodeFilter) === Object.prototype should hold true, but it doesn't in Edge or Gecko.

@foolip
Copy link
Member Author

foolip commented Mar 8, 2016

Aha, it looks like Object.getPrototypeOf(NodeFilter) === Function.prototype holds true. I don't think that's what WebIDL is saying, so I'll file an issue...

@ArkadiuszMichalski
Copy link
Contributor

Hmm, not similar whatwg/webidl#83 ?

@foolip
Copy link
Member Author

foolip commented Mar 8, 2016

Yes, very similar, and quite possibly the fix is the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants