Closed Bug 1096328 Opened 5 years ago Closed 5 years ago

Remove nativeOwnership from Bindings.conf

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: peterv, Assigned: peterv)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(12 files)

8.46 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.36 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.95 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
12.55 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.41 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
3.70 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
9.14 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.40 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.24 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
5.80 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
10.03 KB, patch
vlad
: review+
Details | Diff | Splinter Review
56.78 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
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.
Attachment #8519939 - Flags: review?(bzbarsky)
This involved moving the refcounting macros to some base classes (but they are never used as concrete classes).
Attachment #8519941 - Flags: review?(bzbarsky)
Attachment #8519942 - Flags: review?(bzbarsky)
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.
Attachment #8519945 - Flags: review?(bzbarsky)
I wonder if we should actually spit out multiple example classes :-/.
Attachment #8519948 - Flags: review?(bzbarsky)
Comment on attachment 8519939 [details] [diff] [review]
Reorganize checks for members in BindingUtils.h.

r=me
Attachment #8519939 - Flags: review?(bzbarsky) → review+
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
Attachment #8519941 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519942 [details] [diff] [review]
Templatize deferred finalization.

r=me.  This C++ template stuff can get downright scary sometimes.
Attachment #8519942 - Flags: review?(bzbarsky) → review+
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.
Attachment #8519945 - Flags: review?(bzbarsky) → review+
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
Attachment #8519946 - Flags: review?(bzbarsky) → review+
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.
Attachment #8519947 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519948 [details] [diff] [review]
Make example code spit out a refcounted class always.

r=me
Attachment #8519948 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519949 [details] [diff] [review]
Change a python check for a refcounted type to a static_assert.

r=me
Attachment #8519949 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519950 [details] [diff] [review]
Don't condition wrapperCache on having a refcounted type.

r=me
Attachment #8519950 - Flags: review?(bzbarsky) → review+
Comment on attachment 8519951 [details] [diff] [review]
Remove the now unused nativeOwnership property.

r=me.  Please update the docs!
Attachment #8519951 - Flags: review?(bzbarsky) → review+
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.
Attachment #8546105 - Flags: review?(bzbarsky)
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.
Attachment #8546105 - Flags: review?(bzbarsky) → review+
Peter, we need to document the new WrapObject setup, and probably update the example codegen.  People on IRC are being confused...
Flags: needinfo?(peterv)
Component: DOM → DOM: Core & HTML

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #28)

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

I think this was mostly done. I'll file a followup about the example codegen for non-refcounted objects.

Flags: needinfo?(peterv)
You need to log in before you can comment on or make changes to this bug.