Closed
Bug 1456261
Opened 7 years ago
Closed 7 years ago
Support cycle collection of WebIDL dictionaries
Categories
(Core :: DOM: Bindings (WebIDL), enhancement)
Core
DOM: Bindings (WebIDL)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
This came up in bug 1446961.
![]() |
Assignee | |
Comment 1•7 years ago
|
||
MozReview-Commit-ID: 6epU3m0x4B3
Attachment #8970344 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Comment 2•7 years ago
|
||
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-
![]() |
Assignee | |
Comment 3•7 years ago
|
||
> 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.
![]() |
Assignee | |
Comment 4•7 years ago
|
||
Actually, I thought of a better approach than the failIfUnknown thing.
![]() |
Assignee | |
Comment 5•7 years ago
|
||
This should be much saner and clearer. Thank you for pushing back on the other thing!
Attachment #8970393 -
Flags: review?(bugs)
![]() |
Assignee | |
Updated•7 years ago
|
Attachment #8970344 -
Attachment is obsolete: true
Updated•7 years ago
|
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
Comment 7•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/0f0c781e4886
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
![]() |
Assignee | |
Updated•6 years ago
|
Component: DOM → DOM: Bindings (WebIDL)
You need to log in
before you can comment on or make changes to this bug.
Description
•