Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: froydnj, Assigned: froydnj)

Tracking

unspecified
mozilla42
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Once bug 1116905 and bug 1160485 land, we're almost all set to do this.
This change is prep work for future mass rewriting.  The compiler was not
picking up the already_AddRefed<T>& constructor/assignment, which I think means
that we might be able to delete those.  That's a separate bug.
Attachment #8601602 - Flags: review?(ehsan)
This conversion was done with the script:

  find . -name '*.cpp' -o -name '*.h' -o -name '*.idl' | \
    egrep -v 'cairo-win32-refptr.h|RefPtr.h|TestRefPtr.cpp' | \
    xargs sed -i -e 's/mozilla::TemporaryRef</already_AddRefed</g' \
                 -e 's/TemporaryRef</already_AddRefed</g'

Some manual fixups will be necessary; we'll handle those in a separate patch.
Attachment #8601603 - Flags: review?(ehsan)
sed-converting things would have caused quite some havoc. ;)
Attachment #8601605 - Flags: review?(ehsan)
This patch probably shouldn't be labeled "manually convert" but rather "fixup
compile errors that resulted from part 2".
Attachment #8601606 - Flags: review?(ehsan)
I guess almost all of this could have been done in part 2, but it seemed
valuable to separate it out in case there was anything a little weird.
Attachment #8601607 - Flags: review?(ehsan)
Finally, we're ready to do this.

I have only compiled this on Linux64, and while it works OK there, there's
issues with media/'s wacky WrapRunnable* family of functions that will need
some extra effort to work out.  I'll tackle those in a separate patch or bug.

There may be some followups to address Windows/Mac/Android/B2G compilation
issues.
Attachment #8601609 - Flags: review?(ehsan)
Depends on: 1116905, 1160485
Attachment #8601602 - Flags: review?(ehsan) → review+
Comment on attachment 8601603 [details] [diff] [review]
part 2 - machine-convert TemporaryRef<T> to already_AddRefed<T>

Review of attachment 8601603 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me
Attachment #8601603 - Flags: review?(ehsan) → review+
Comment on attachment 8601605 [details] [diff] [review]
part 3a - manually convert TemporaryRef<T> to already_AddRefed<T> in RefPtr.h

Review of attachment 8601605 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/RefPtr.h
@@ +284,4 @@
>    {
>      T* tmp = mPtr;
>      mPtr = nullptr;
> +    return already_AddRefed<T>(tmp);

We should probably move dont_AddRef into AlreadyAddRefed.h and use it here instead.  Please either do that here, or file a follow-up for it.  Thanks!
Attachment #8601605 - Flags: review?(ehsan) → review+
Comment on attachment 8601606 [details] [diff] [review]
part 3b - manually convert TemporaryRef<T> to already_AddRefed<T> in dom/

Review of attachment 8601606 [details] [diff] [review]:
-----------------------------------------------------------------

Please fold into part 2 when landing.
Attachment #8601606 - Flags: review?(ehsan) → review+
Attachment #8601607 - Flags: review?(ehsan) → review+
Comment on attachment 8601609 [details] [diff] [review]
part 4 - remove TemporaryRef<T> from RefPtr.h

Review of attachment 8601609 [details] [diff] [review]:
-----------------------------------------------------------------

\o/
Attachment #8601609 - Flags: review?(ehsan) → review+
Depends on: 1162026
Landed part 1 to make people's lives easier:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b45c68b9dceb
Keywords: leave-open
Depends on: 1175621
This is the current manual fixup patch.  It differs in one small respect from
the previously reviewed patch, which is the change to PannerNode.cpp:
HRTFPanner's constructor now takes already_AddRefed, so we need an explicit
Move to not invoke the copy constructor.

All well and good, except that HRTFPanner resides in
dom/media/webaudio/blink/.  How much do we care about keeping this code
synchronized with Blink?  I see there have been several changes to HRTFPanner
since its inclusion ~2 years ago...
Attachment #8623826 - Flags: review+
ni? padenot for comment 13, since he seems to have reviewed most of the HRTFPanner changes.
Flags: needinfo?(padenot)
Yeah no that's cool, you can change it.
Flags: needinfo?(padenot)
Attachment #8601606 - Attachment is obsolete: true
I squashed the part 2/3 patches together into a single patch when landing for bisection goodness and renumbered things accordingly.

Try was clean except for W(4) failures, which are expected (we don't run wpt debug on non-try yet):

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4debf610be3a
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/c664375c0d5b
https://hg.mozilla.org/mozilla-central/rev/d418eb96a90d
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Depends on: 1180105
Duplicate of this bug: 1115033
You need to log in before you can comment on or make changes to this bug.