838686 - Switch NodeFilter to WebIDL codegen

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

I'm going to add infrastructure here to have a class representing a "WebIDL callback or XPCOM interface", which we need anyway for bug 835643.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a helper class that can store a WebIDL callback or an XPCOM interface.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Use NodeFilterHolder in treewalker and nodeiterator and start using WebIDL codegen for NodeFilter.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 711265 [details] [diff] [review]
part 1.  Add a helper class that can store a WebIDL callback or an XPCOM interface.

Review of attachment 711265 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/CallbackObject.cpp
@@ +178,5 @@
> +  if (NS_FAILED(rv) || !wrappedJS) {
> +    return nullptr;
> +  }
> +
> +  nsISupports* retval;

Smart pointer, please

::: dom/bindings/CallbackObject.h
@@ +300,5 @@
> +  {
> +    if (HasWebIDLCallback()) {
> +      WebIDLCallbackT* callback = GetWebIDLCallback();
> +      NS_IF_ADDREF(callback);
> +      return callback;

And here, please

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 with smart ptrs

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 without some junk that was moved to part 1

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 711276 [details] [diff] [review]
Part 2 without some junk that was moved to part 1

This is failing tests (particularly js/xpconnect/tests/mochitest/test_bug446584.html) because we need to propagate through the exception.  I'm going to add some infrastructure for that.  Filed bug 839088 on that.  We'll also need to fix bug 776536 in the process.

Comment 7

[Boris Zbarsky [:bzbarsky]]

So maybe the right thing to do is as follows:

1) Modify part 2 here so that it grabs an nsIDOMNodeFilter from the incoming NodeFilterHolder so that we avoid regressing that test.

2) Fix bug 839088.

3) Fix bug 776536 and stop coercing to nsIDOMNodeFilter in the process.

I'll work on doing that, I guess.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 modified to keep calling the XPCOM callback for now for NodeFilter

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 with UnwrapObject (without the Checked bit) because UnwrapObjectChecked doesn't seem to work right.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 712306 [details] [diff] [review]
Part 1 with UnwrapObject (without the Checked bit) because UnwrapObjectChecked doesn't seem to work right.

Review of attachment 712306 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/CallbackObject.h
@@ +80,5 @@
>      xpc_UnmarkGrayObject(mCallback);
>      return mCallback;
>    }
>  
> +  JSObject* CallbackNoUnmarkGray() const

In nsWrapperCache we named this *PreserveColor, maybe we should be consistent. You should probably add a comment similar to the one above nsWrapperCache::GetWrapperPreserveColor.

@@ +257,5 @@
> +    if (!aOtherCallback || !HasWebIDLCallback() || !GetWebIDLCallback()) {
> +      // Other is null and we're not, or we're not a WebIDL callback,
> +      // or we're null and other is not.
> +      return false;
> +    }

I think I'd prefer:

  if (!aOtherCallback) {
    // If other is null then we must be null to be equal.
    return !GetISupports();
  }

  if (!HasWebIDLCallback() || !GetWebIDLCallback()) {
    // If other is non-null then we can't be equal if we have a non-WebIDL or
    // a null callback.
    return false;
  }

Comment 11

[Peter Van der Beken [:peterv]]

Comment on attachment 712079 [details] [diff] [review]
Part 2 modified to keep calling the XPCOM callback for now for NodeFilter

Review of attachment 712079 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/libeditor/text/nsTextEditRules.cpp
@@ +448,5 @@
>      nsCOMPtr<nsINode> node = do_QueryInterface(selNode);
>      // if node is null, return it to indicate there's no text
>      NS_ENSURE_TRUE(node, nullptr);
>      // This should be the root node, walk the tree looking for text nodes
> +    mozilla::dom::NodeFilterHolder filter(static_cast<nsIDOMNodeFilter*>(nullptr));

Can't this be |mozilla::dom::NodeFilterHolder filter();|?

Comment 12

[Boris Zbarsky [:bzbarsky]]

> In nsWrapperCache we named this *PreserveColor, maybe we should be consistent. You should
> probably add a comment similar to the one above nsWrapperCache::GetWrapperPreserveColor.

OK, done.

> I think I'd prefer:

Agreed that this is clearer.  Fixed accordingly.

> Can't this be |mozilla::dom::NodeFilterHolder filter();|?

Yes, it can.  Good catch.  Fixed.

Comment 13

[Boris Zbarsky [:bzbarsky]]

   https://hg.mozilla.org/integration/mozilla-inbound/rev/a4763990a983
   https://hg.mozilla.org/integration/mozilla-inbound/rev/1c851a5bbc9f

Comment 14

[Boris Zbarsky [:bzbarsky]]

And backed out because gcc doesn't like it:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/7e07e7ffe99e
   https://hg.mozilla.org/integration/mozilla-inbound/rev/215273993b1f

Comment 15

[Boris Zbarsky [:bzbarsky]]

Fixed that and pushed again:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/ac3b7681470c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/a3aaa9067e14

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/ac3b7681470c
https://hg.mozilla.org/mozilla-central/rev/a3aaa9067e14