Closed Bug 838686 Opened 7 years ago Closed 7 years ago

Switch NodeFilter to WebIDL codegen

Categories

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

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(2 files, 4 obsolete files)

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 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
Attached patch Part 1 with smart ptrs (obsolete) — Splinter Review
Attachment #711275 - Flags: review?(peterv)
Attachment #711265 - Attachment is obsolete: true
Attachment #711265 - Flags: review?(peterv)
Attachment #711266 - Attachment is obsolete: true
Attachment #711266 - Flags: review?(peterv)
Depends on: 839088
Depends on: 776536
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.
Attachment #711276 - Flags: review?(peterv)
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.
Blocks: 776536
No longer depends on: 776536
Attachment #711276 - Attachment is obsolete: true
Whiteboard: [need review]
Attachment #711275 - Attachment is obsolete: true
Attachment #711275 - Flags: review?(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;
  }
Attachment #712306 - Flags: review?(peterv) → review+
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();|?
Attachment #712079 - Flags: review?(peterv) → review+
> 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.
Blocks: 862627
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.