Peter Van der Beken [:peterv] Assignee
	Description • 10 years ago

      No description provided.

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

This is really just preparation. It reorganizes the HAS_MEMBER macros so you can have multiple of them in one struct (eg NativeHasMember<T>::GetParentObject/WrapObject/...), and for the others it uses ::value instead of ::Value, which is more in line with what mfbt does.

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

This involved moving the refcounting macros to some base classes (but they are never used as concrete classes).

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

By making sure that we null out the private if we create a binding object and binding initialization fails somewhere, we don't need the tookOwnership bool.

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

I wonder if we should actually spit out multiple example classes :-/.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 10 years ago

Comment on attachment 8519939 [details] [diff] [review]
Reorganize checks for members in BindingUtils.h.

r=me

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

Comment on attachment 8519941 [details] [diff] [review]
Add template to detect refcounted classes.

Nice catches on the DOMMatrix/DOMPoint bits.

>+            cgThings.append(CGGeneric(define="static_assert(IsRefcounted<NativeType>::value == IsRefcounted<%s::NativeType>::value,\n"

You don't need the define= part, right?  Just the string.

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 13 • 10 years ago

Comment on attachment 8519942 [details] [diff] [review]
Templatize deferred finalization.

r=me.  This C++ template stuff can get downright scary sometimes.

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 10 years ago

Comment on attachment 8519945 [details] [diff] [review]
Make the WrapObject signature for non-refcounted objects the same as for refcounted objects.

>+class BindingCreator

Maybe ReflectorCreator?  Or BindingJSObjectCreator?

>+  ~BindingCreator()

Newline before this line?

>+      js::SetReservedOrProxyPrivateSlot(mObject, DOM_OBJECT_SLOT,
>+                                        JS::PrivateValue(nullptr));

I think if you set it to JS::UndefinedValue() you won't need to add a null-check in AppendAndTake.

>+  JS::Rooted<JSObject*>&
>+  CreateObject(JSContext* aCx, const JSClass* aClass,

Newline before these two lines?

r=me, but it feels like a comment somewhere, perhaps before the BindingCreator (or whatever) clas definition, that explains how this works, would help a lot.

	Boris Zbarsky [:bzbarsky]
	Comment 15 • 10 years ago

Comment on attachment 8519946 [details] [diff] [review]
Make WrapNewBindingNonWrapperCachedObject work for owned objects.

>+  static_assert(IsRefcounted<T>::value, "Don't pas owned classes in here.");

"pass"

>+  static_assert(!IsRefcounted<T>::value, "Only pas owned classes in here.");

And here.

>+    static_assert(IsRefcounted<T>::value, "Don't pas owned classes in here.");

And here.

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 16 • 10 years ago

Comment on attachment 8519947 [details] [diff] [review]
Autodetect the right smart pointer for owned and refcounted objects.

There's a stray bit of commit comment above the "#" bits there.

>+struct StrongPtrMember

Maybe StrongPtrForMember?

>+                               nsRefPtr<T>, nsAutoPtr<T>>::Type Ptr;

And maybe call this Type instead of Ptr?

So with this patch the return values of [NewObject] things that return owned objects need to be nsAutoPtr, right?  That seems reasonable, as long as we update the documentation accordingly.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 10 years ago

Comment on attachment 8519948 [details] [diff] [review]
Make example code spit out a refcounted class always.

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 18 • 10 years ago

Comment on attachment 8519949 [details] [diff] [review]
Change a python check for a refcounted type to a static_assert.

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 10 years ago

Comment on attachment 8519950 [details] [diff] [review]
Don't condition wrapperCache on having a refcounted type.

r=me

	Boris Zbarsky [:bzbarsky]
	Comment 20 • 10 years ago

Comment on attachment 8519951 [details] [diff] [review]
Remove the now unused nativeOwnership property.

r=me.  Please update the docs!

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

The rooting analysis complains about a hazard because the call to BindingJSObjectCreator::ForgetObject returns a JSObject*, and then the BindingJSObjectCreator destructor destroys a nsRefPtr. ForgetObject nulls out that nsRefPtr though, so it's not really an issue. We could annotate, but it might make more sense to take this patch, to make WrapObject take a MutableHandle instead of returning a raw pointer. I've avoided changing the nsWrapperCache::WrapObject signature for now, there's a lot of implementations and we're considering changing its signature anyway for bug 1117172 so we might want to do it there at the same time.
The one thing I don't like is that now BindingJSObjectCreator keeps a JSObject* pointer to an object that's held alive by the MutableHandle in the Wrap functions, but nothing really enforces that. Not sure if we can do something elegant about that.

	Boris Zbarsky [:bzbarsky]
	Comment 23 • 10 years ago

Comment on attachment 8546105 [details] [diff] [review]
Make rooting analysis happy

How is holding a JSObject* mReflector ok?  How do we know it hasn't been moved by the time we get to ~BindingJSObjectCreator?

@@ -3323,70 +3325,70 @@ class CGWrapGlobalMethod(CGAbstractMetho
...

+            if (!DefineProperties(aCx, aReflector, ${properties}, ${chromeProperties})) {
               return nullptr;

return false?

The rest looks good, but I would _really_ like to understand the story with mReflector before you land this.

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

(In reply to Boris Zbarsky [:bz] from comment #23)
> How is holding a JSObject* mReflector ok?  How do we know it hasn't been
> moved by the time we get to ~BindingJSObjectCreator?

I made it a JS::Rooted<JSObject*>.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c37e10cff765
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f330a5dd346
https://hg.mozilla.org/integration/mozilla-inbound/rev/6005fd357342
https://hg.mozilla.org/integration/mozilla-inbound/rev/55c831086864
https://hg.mozilla.org/integration/mozilla-inbound/rev/899d8cd8c4e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e68ba4846d
https://hg.mozilla.org/integration/mozilla-inbound/rev/47be69b84be5
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ed945e9a8a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e64e751ece
https://hg.mozilla.org/integration/mozilla-inbound/rev/676112a4f092
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a067de94f22
https://hg.mozilla.org/integration/mozilla-inbound/rev/12dd1ad43923

	[:fabrice] Fabrice Desré
	Comment 25 • 10 years ago

https://hg.mozilla.org/projects/cypress/rev/a7f64e53893e

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

Turns out this wasn't the cause of the problem for which it was backed out, so relanded:

https://hg.mozilla.org/integration/mozilla-inbound/rev/fb38a3ad96ce
https://hg.mozilla.org/integration/mozilla-inbound/rev/7941dd7c8866
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecdac08e0897
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4a528db0d9
https://hg.mozilla.org/integration/mozilla-inbound/rev/f4f784f1d227
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3b689b8d233
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae57edae370e
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2bdf38c84f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5557b9aee8f
https://hg.mozilla.org/integration/mozilla-inbound/rev/fae280dc596d
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5be217117fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/3bff2bbe6677

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 27 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/fb38a3ad96ce
https://hg.mozilla.org/mozilla-central/rev/7941dd7c8866
https://hg.mozilla.org/mozilla-central/rev/ecdac08e0897
https://hg.mozilla.org/mozilla-central/rev/8e4a528db0d9
https://hg.mozilla.org/mozilla-central/rev/f4f784f1d227
https://hg.mozilla.org/mozilla-central/rev/a3b689b8d233
https://hg.mozilla.org/mozilla-central/rev/ae57edae370e
https://hg.mozilla.org/mozilla-central/rev/c2bdf38c84f3
https://hg.mozilla.org/mozilla-central/rev/d5557b9aee8f
https://hg.mozilla.org/mozilla-central/rev/fae280dc596d
https://hg.mozilla.org/mozilla-central/rev/b5be217117fa
https://hg.mozilla.org/mozilla-central/rev/3bff2bbe6677

	Boris Zbarsky [:bzbarsky]
	Comment 28 • 10 years ago

Peter, we need to document the new WrapObject setup, and probably update the example codegen.  People on IRC are being confused...