1161627 - remove TemporaryRef<T>

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Nathan Froyd [:froydnj]]

Once bug 1116905 and bug 1160485 land, we're almost all set to do this.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - add move constructor and assignment operator for already_AddRefed&& to RefPtr

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.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - machine-convert TemporaryRef<T> to already_AddRefed<T>

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.

Comment 3

[Nathan Froyd [:froydnj]]

Attachment: part 3a - manually convert TemporaryRef<T> to already_AddRefed<T> in RefPtr.h

sed-converting things would have caused quite some havoc. ;)

Comment 4

[Nathan Froyd [:froydnj]]

Attachment: part 3b - manually convert TemporaryRef<T> to already_AddRefed<T> in dom/

This patch probably shouldn't be labeled "manually convert" but rather "fixup
compile errors that resulted from part 2".

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: part 3c - manually convert TemporaryRef<T> to already_AddRefed<T> in TestRefPtr.cpp

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.

Comment 6

[Nathan Froyd [:froydnj]]

Attachment: part 4 - remove TemporaryRef<T> from RefPtr.h

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.

Comment 7

[(no longer active)]

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

Comment 8

[(no longer active)]

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!

Comment 9

[(no longer active)]

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.

Comment 10

[(no longer active)]

Comment on attachment 8601609 [details] [diff] [review]
part 4 - remove TemporaryRef<T> from RefPtr.h

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

\o/

Comment 11

[Nathan Froyd [:froydnj]]

Landed part 1 to make people's lives easier:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b45c68b9dceb

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b45c68b9dceb

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: part 3b - manually convert TemporaryRef<T> to already_AddRefed<T> in dom/

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...

Comment 14

[Nathan Froyd [:froydnj]]

ni? padenot for comment 13, since he seems to have reviewed most of the HRTFPanner changes.

Comment 15

[Paul Adenot (:padenot)]

Yeah no that's cool, you can change it.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c664375c0d5b
https://hg.mozilla.org/integration/mozilla-inbound/rev/d418eb96a90d

Comment 17

[Nathan Froyd [:froydnj]]

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

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c664375c0d5b
https://hg.mozilla.org/mozilla-central/rev/d418eb96a90d