Closed
Bug 1251480
Opened 8 years ago
Closed 8 years ago
Remove XMLHttpRequest::StateData's CustomAutoRooter
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla47
Tracking | Status | |
---|---|---|
firefox47 | --- | fixed |
People
(Reporter: sfink, Assigned: sfink)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 1 obsolete file)
3.59 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
5.69 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
We shouldn't need this CustomAutoRooter anymore.
Assignee | ||
Comment 1•8 years ago
|
||
This was my first attempt. It's not straightforward because the object is held in an nsAutoPtr<T>, and we want to use a Rooted<T*>. This is the straightforward way to go about it, relying on the nested destructors to fire in the right order.
Assignee | ||
Comment 2•8 years ago
|
||
Specialize for UniquePtr. One part of this is weird -- MapTypeToRootKind<UniquePtr<T>> delegates to MapTypeToRootKind<T>. Another alternative would be to just hardcode it to Traceable, on the assumption that UniquePtr<JSObject> is pretty much useless (without a *really* funky deleter, I suppose.) I don't entirely understand what the regular pointer specialization is doing, but perhaps it might make sense?
Attachment #8723880 -
Flags: review?(terrence)
Assignee | ||
Updated•8 years ago
|
Attachment #8723875 -
Attachment is obsolete: true
Assignee | ||
Comment 3•8 years ago
|
||
And here's the actual user of Rooted<UniquePtr<T>>. Giving review to bz mostly as an FYI that this is now possible. Assuming terrence is ok with the earlier patch.
Attachment #8723881 -
Flags: review?(bzbarsky)
Comment 4•8 years ago
|
||
Comment on attachment 8723880 [details] [diff] [review] Implement Rooted<UniquePtr<T>> Review of attachment 8723880 [details] [diff] [review]: ----------------------------------------------------------------- Nice! ::: js/public/GCPolicyAPI.h @@ +122,5 @@ > + tp->get()->trace(trc); > + } > + static bool needsSweep(mozilla::UniquePtr<T>* tp) { > + return tp->get()->needSweep(); > + } So I guess this is hard-coded to assume that T would use a StructGCPolicy. Could we instead write these all as: return GCPolicy<T>::foo(t->get()); And add a comment at the top that explains somehow that the GCPolicy for UniquePtr<T> is to look through the UniquePtr and use the policy for T. ::: js/public/TraceKind.h @@ +139,5 @@ > JS::MapTraceKindToRootKind<JS::MapTypeToTraceKind<T>::kind>::kind; > }; > +template <typename T> > +struct MapTypeToRootKind<mozilla::UniquePtr<T>> { > + static const JS::RootKind kind = JS::MapTypeToRootKind<T>::kind; Ah, just like this.
Attachment #8723880 -
Flags: review?(terrence) → review+
Comment 5•8 years ago
|
||
Comment on attachment 8723881 [details] [diff] [review] Use Rooted<UniquePtr<StateData>> in place of StateData::CustomAutoRooter r=me
Attachment #8723881 -
Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0d2cd5f762b https://hg.mozilla.org/integration/mozilla-inbound/rev/80869f1474a4
Comment 7•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d0d2cd5f762b https://hg.mozilla.org/mozilla-central/rev/80869f1474a4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in
before you can comment on or make changes to this bug.
Description
•