Boris Zbarsky [:bzbarsky] Assignee
	Description • 12 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 12 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 12 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 7 • 12 years ago

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.

	Peter Van der Beken [:peterv]
	Comment 10 • 12 years ago

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;
  }

	Peter Van der Beken [:peterv]
	Comment 11 • 12 years ago

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();|?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 12 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 13 • 12 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 14 • 12 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 12 years ago

Fixed that and pushed again:

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

	Ed Morley [:emorley]
	Comment 16 • 12 years ago

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