Closed Bug 1456261 Opened 6 years ago Closed 6 years ago

Support cycle collection of WebIDL dictionaries

Categories

(Core :: DOM: Bindings (WebIDL), enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

Attachments

(1 file, 1 obsolete file)

This came up in bug 1446961.
MozReview-Commit-ID: 6epU3m0x4B3
Attachment #8970344 - Flags: review?(bugs)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment on attachment 8970344 [details] [diff] [review]
Add cycle collection bits for WebIDL dictionaries

>+# Failure is when we have a type for which we can't actually tell
>+# whether it needs cycle collection or not.  In that case, if
>+# failureIsFatal we will error out; otherwise we will return false so
>+# we don't generate possibly-not-needed cycle collection machinery.
>+def idlTypeNeedsCycleCollection(type, failIfUnknown=True):
The comment talks about failureIsFatal, but the argument name I see is failIfUnknown.


>+template<typename T>
>+inline void
>+ImplCycleCollectionUnlink(OwningNonNull<T>& aField)
>+{
>+  // We can't be null, so nothing to do here....  Yes, this is a leak hazard.
>+}
I don't understand the comment here. If we are non-null, we definitely should unlink, no?
Why can't we forget() the field or something? Or is the idea that once initialized, we wouldn't  want the
field to ever become null. Well, forget() may do that anyway, right?

r- because of OwningNonNull case.  Please explain.
Attachment #8970344 - Flags: review?(bugs) → review-
> The comment talks about failureIsFatal

Sorry.  I renamed it but failed to completely update the comment.  :(

> Or is the idea that once initialized, we wouldn't  want the field to ever become null.
> Well, forget() may do that anyway, right?

That was the idea, yes.  I had forgotten that forget() exists.  I should just use it here.
Actually, I thought of a better approach than the failIfUnknown thing.
This should be much saner and clearer.  Thank you for pushing back on the other thing!
Attachment #8970393 - Flags: review?(bugs)
Attachment #8970344 - Attachment is obsolete: true
Attachment #8970393 - Flags: review?(bugs) → review+
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f0c781e4886
Add cycle collection bits for WebIDL dictionaries.  r=smaug
https://hg.mozilla.org/mozilla-central/rev/0f0c781e4886
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Component: DOM → DOM: Bindings (WebIDL)
You need to log in before you can comment on or make changes to this bug.