Closed
Bug 1207245
Opened 9 years ago
Closed 9 years ago
get rid of nsRefPtr / RefPtr split
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla44
Tracking | Status | |
---|---|---|
firefox44 | --- | fixed |
People
(Reporter: froydnj, Assigned: froydnj)
References
Details
Attachments
(8 files, 1 obsolete file)
19.60 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.95 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
1.62 MB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
8.72 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
5.70 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
8.76 MB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
2.25 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
2.33 KB,
patch
|
botond
:
review+
|
Details | Diff | Splinter Review |
Seems as good a time as any to do this.
Assignee | ||
Comment 1•9 years ago
|
||
Various bits depend on RefPtr.h to provide RefCounted<T> and RefPtr<T>. It will be easier to manage an automatic conversion from RefPtr<T> to nsRefPtr<T> if we split out the dependency on RefCounted<T> first.
Attachment #8664436 -
Flags: review?(ehsan)
Assignee | ||
Comment 2•9 years ago
|
||
A number of places depend on RefPtr.h providing this function. When we s/RefPtr/nsRefPtr/, such places still need to be able to see this function. Moving it to nsRefPtr.h makes it still visible before we switch (since RefPtr.h includes nsRefPtr.h), and after we switch (since every place that #includes RefPtr.h will now be #including nsRefPtr.h).
Attachment #8664437 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•9 years ago
|
||
# Don't modify select files in mfbt/ because it's not worth trying to # tease out the dependencies currently. # # Don't modify anything in media/gmp-clearkey/0.1/ because those files # use their own RefPtr, defined in their own RefCounted.h. find . -name '*.cpp' -o -name '.h' -o -name '*.mm' -o -name '*.idl'| \ grep -v 'mfbt/RefPtr.h' | \ grep -v 'mfbt/nsRefPtr.h' | \ grep -v 'mfbt/RefCounted.h' | \ grep -v 'media/gmp-clearkey/0.1/' | \ xargs perl -p -i -e ' s/mozilla::RefPtr/nsRefPtr/g; # handle declarations in headers s/\bRefPtr</nsRefPtr</g; # handle local variables in functions s#mozilla/RefPtr.h#mozilla/nsRefPtr.h#; # handle #includes s#mfbt/RefPtr.h#mfbt/nsRefPtr.h#; # handle strange #includes ' # |using mozilla::RefPtr;| is OK; |using nsRefPtr;| is invalid syntax. find . -name '*.cpp' | xargs sed -i -e '/using nsRefPtr/d' # RefPtr.h used |byRef| for dealing with COM-style outparams. # nsRefPtr.h uses |getter_AddRefs|. # Fixup that mismatch. find . -name '*.cpp' | \ xargs perl -p -i -e 's/byRef/getter_AddRefs/g'
Attachment #8664438 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•9 years ago
|
||
part 4 - remove RefPtr.h It has been superseded by the availability of nsRefPtr.h.
Attachment #8664439 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•9 years ago
|
||
Having a template parameter conflict with a global name is terribly inconvenient, so let's try to avoid that by renaming the 'RefPtr' template parameter to something else.
Attachment #8664440 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•9 years ago
|
||
The bulk of this commit was generated with a script, executed at the top level of a typical source code checkout. The only non-machine-generated part was modifying MFBT's moz.build to reflect the new naming. find . -name '*.cpp' -o -name '*.cc' -o -name '*.h' -o -name '*.mm' -o -name '*.idl'| \ xargs perl -p -i -e ' s/nsRefPtr\.h/RefPtr\.h/g; # handle includes s/nsRefPtr ?</RefPtr</g; # handle declarations and variables ' # Handle nsRefPtr.h itself, a couple places that define constructors # from nsRefPtr, and code generators specially. We do this here, rather # than indiscriminantly s/nsRefPtr/RefPtr/, because that would rename # things like nsRefPtrHashtable. perl -p -i -e 's/nsRefPtr/RefPtr/g' \ mfbt/nsRefPtr.h \ xpcom/glue/nsCOMPtr.h \ xpcom/base/OwningNonNull.h \ ipc/ipdl/ipdl/lower.py \ ipc/ipdl/ipdl/builtin.py \ dom/bindings/Codegen.py \ python/lldbutils/lldbutils/utils.py # In our indiscriminate substitution above, we renamed # nsRefPtrGetterAddRefs, the class behind getter_AddRefs. Fix that up. find . -name '*.cpp' -o -name '*.h' -o -name '*.idl' | \ xargs perl -p -i -e 's/nsRefPtrGetterAddRefs/RefPtrGetterAddRefs/g' git mv mfbt/nsRefPtr.h mfbt/RefPtr.h
Attachment #8664442 -
Flags: review?(ehsan)
Assignee | ||
Comment 7•9 years ago
|
||
It's worth noting that the "new" RefPtr<T> is RefPtr *at global scope*; it does not live in the mozilla namespace. I am slightly bummed out by that, but I don't have the tools or fortitude to fix all the instances of RefPtr<T> to be properly qualified (or not, depending). Suggestions or clever ideas on how to do that welcome. I'm also going to assume that I have rs+ for fixing up any problems that a wider set of platforms and/or rebases turn up. Given that the bulk of the patches are machine-generated, I don't think this will be too much of a problem.
Assignee | ||
Updated•9 years ago
|
Attachment #8664439 -
Attachment description: .# Attachment to Bug 1207245 - get rid of nsRefPtr / RefPtr split → part 4 - remove RefPtr.h
Comment 8•9 years ago
|
||
Comment on attachment 8664436 [details] [diff] [review] part 1 - move RefCounted<T> to its own file Review of attachment 8664436 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/RefCounted.h @@ +3,5 @@ > +/* This Source Code Form is subject to the terms of the Mozilla Public > + * License, v. 2.0. If a copy of the MPL was not distributed with this > + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > + > +/* CRTP refcounting templates. Do not use unless you are an Expert. */ I really wish you just converted the above to "Do not use." :-)
Attachment #8664436 -
Flags: review?(ehsan) → review+
Updated•9 years ago
|
Attachment #8664437 -
Flags: review?(ehsan) → review+
Comment 9•9 years ago
|
||
Comment on attachment 8664438 [details] [diff] [review] part 3 - switch all uses of mozilla::RefPtr<T> to nsRefPtr<T> Review of attachment 8664438 [details] [diff] [review]: ----------------------------------------------------------------- rs=me
Attachment #8664438 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 10•9 years ago
|
||
RefPtr.h's byRef permits callees to see the incoming value of the outparam; XPCOM's getter_AddRefs zeros outparams prior to the call, so information doesn't leak through inadvertently. Given this difference, we need to eliminate tests that depended on this (arguably dangerous) behavior. The numerous assertion fixups are required because we're removing construction and destruction of objects along the way.
Attachment #8665065 -
Flags: review?(ehsan)
Comment 11•9 years ago
|
||
Comment on attachment 8665065 [details] [diff] [review] part 0 - fix why-did-we-allow-that tests in TestRefPtr.cpp Review of attachment 8665065 [details] [diff] [review]: ----------------------------------------------------------------- Crap. Looks like we're using byRef() all over the place. :( We should start a new project to get rid of that.
Attachment #8665065 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Ehsan Akhgari (don't ask for review please) from comment #11) > Crap. Looks like we're using byRef() all over the place. :( We should > start a new project to get rid of that. byRef() gets completely removed by part 3 in this patch; its uses are replaced by getter_AddRefs. Investigation on try suggests that nobody is depending on this behavior, which is a *good thing*.
Updated•9 years ago
|
Attachment #8664439 -
Flags: review?(ehsan) → review+
Updated•9 years ago
|
Attachment #8664440 -
Flags: review?(ehsan) → review+
Comment 13•9 years ago
|
||
Comment on attachment 8664442 [details] [diff] [review] part 6 - rename nsRefPtr<T> to RefPtr<T> Review of attachment 8664442 [details] [diff] [review]: ----------------------------------------------------------------- Thanks a lot for doing this, Nathan! You rock!
Attachment #8664442 -
Flags: review?(ehsan) → review+
Comment 14•9 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #12) > (In reply to Ehsan Akhgari (don't ask for review please) from comment #11) > > Crap. Looks like we're using byRef() all over the place. :( We should > > start a new project to get rid of that. > > byRef() gets completely removed by part 3 in this patch; its uses are > replaced by getter_AddRefs. Investigation on try suggests that nobody is > depending on this behavior, which is a *good thing*. Awesome!
Comment 15•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/86cd8c6a23d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ee8cbe5e73 https://hg.mozilla.org/integration/mozilla-inbound/rev/86fb8a462b04
Assignee | ||
Comment 16•9 years ago
|
||
Landed the first three parts: https://hg.mozilla.org/integration/mozilla-inbound/rev/86cd8c6a23d1 https://hg.mozilla.org/integration/mozilla-inbound/rev/a4ee8cbe5e73 https://hg.mozilla.org/integration/mozilla-inbound/rev/86fb8a462b04 I think I may delay landing the others so as not to mess with people doing end-of-the-quarter crash landings.
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → nfroyd
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/86cd8c6a23d1 https://hg.mozilla.org/mozilla-central/rev/a4ee8cbe5e73 https://hg.mozilla.org/mozilla-central/rev/86fb8a462b04
Comment 18•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/bfb9436756a0 https://hg.mozilla.org/mozilla-central/rev/1f51d1614b9a https://hg.mozilla.org/mozilla-central/rev/ddcc05491282 https://hg.mozilla.org/mozilla-central/rev/93828e13ad63 https://hg.mozilla.org/mozilla-central/rev/bcc1bcac2320 https://hg.mozilla.org/mozilla-central/rev/91d4539e00ce
I had to back this out because I was hitting a lot of merge conflicts trying to get inbound merged to m-c. Feel free to rebase and reland this whenever you get to it. https://hg.mozilla.org/mozilla-central/rev/1831ba1be747
Hrm, bugherder didn't list them all, but I backed them all out: https://hg.mozilla.org/mozilla-central/rev/1831ba1be747 https://hg.mozilla.org/mozilla-central/rev/6f47f75d3136 https://hg.mozilla.org/mozilla-central/rev/9f660e994268 https://hg.mozilla.org/mozilla-central/rev/5108c4505520 https://hg.mozilla.org/mozilla-central/rev/11db5387d083 https://hg.mozilla.org/mozilla-central/rev/41dea9df27ed https://hg.mozilla.org/mozilla-central/rev/482930b9667a https://hg.mozilla.org/mozilla-central/rev/711d402ed650
Flags: needinfo?(nfroyd)
Comment 23•9 years ago
|
||
Nathan, you really should do the merges yourself. I don't think sheriffs can ever do it for you successfully.
Comment 24•9 years ago
|
||
Comment on attachment 8664440 [details] [diff] [review] part 5 - rename template parameters for nsRefPtrHashtable Review of attachment 8664440 [details] [diff] [review]: ----------------------------------------------------------------- ::: xpcom/glue/nsRefPtrHashtable.h @@ +15,5 @@ > * templated hashtable class maps keys to reference pointers. > * See nsBaseHashtable for complete declaration. > * @param KeyClass a wrapper-class for the hashtable key, see nsHashKeys.h > * for a complete specification. > * @param RefPtr the reference-type being wrapped You should change this RefPtr in the comment to PtrType, too.
Assignee | ||
Comment 25•9 years ago
|
||
(In reply to Wes Kocher (:KWierso) from comment #22) > Hrm, bugherder didn't list them all, but I backed them all out: Thanks for taking care of this. I'm going to try landing the standalone patches separately, and then have another go at landing the automated ones later this week or early next.
Flags: needinfo?(nfroyd)
Assignee | ||
Comment 27•9 years ago
|
||
(In reply to Pulsebot from comment #26) > https://hg.mozilla.org/integration/mozilla-inbound/rev/0c0654ecff2a This was additional "mozilla/RefCounted.h" includes as a followup for part 2, which I felt could land without additional review.
Assignee | ||
Comment 28•9 years ago
|
||
Might as well have Botond review this properly before trying to land everything else. The problematic class is AtomicRefCountedWithFinalize, from gfx/layers/; I don't know all the details for why that class is written the way it is, but it is simpler to make nsRefPtr work the same way as mozilla::RefPtr in this case.
Attachment #8672722 -
Flags: review?(botond)
Assignee | ||
Comment 29•9 years ago
|
||
The problematic class is AtomicRefCountedWithFinalize, from gfx/layers/; I don't know all the details for why that class is written the way it is, but it is simpler to make nsRefPtr work the same way as mozilla::RefPtr in this case.
Attachment #8672781 -
Flags: review?(botond)
Assignee | ||
Updated•9 years ago
|
Attachment #8672722 -
Attachment is obsolete: true
Attachment #8672722 -
Flags: review?(botond)
Updated•9 years ago
|
Attachment #8672781 -
Flags: review?(botond) → review+
Comment 35•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca86c21a96b4 https://hg.mozilla.org/integration/mozilla-inbound/rev/12d944fcb09d https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c7dfe727cd
Assignee | ||
Comment 36•9 years ago
|
||
(In reply to Pulsebot from comment #35) > https://hg.mozilla.org/integration/mozilla-inbound/rev/ca86c21a96b4 > https://hg.mozilla.org/integration/mozilla-inbound/rev/12d944fcb09d > https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c7dfe727cd These look like they're gonna stick, so removing leave-open. \o/
Keywords: leave-open
Comment 37•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ca86c21a96b4 https://hg.mozilla.org/mozilla-central/rev/12d944fcb09d https://hg.mozilla.org/mozilla-central/rev/e8c7dfe727cd
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox44:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
Blocks: 1320785
You need to log in
before you can comment on or make changes to this bug.
Description
•