remove TemporaryRef<T>

RESOLVED FIXED in Firefox 42

Status

()

Core
MFBT
RESOLVED FIXED
3 years ago
3 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)

(Assignee)

Description

3 years ago
Once bug 1116905 and bug 1160485 land, we're almost all set to do this.
(Assignee)

Comment 1

3 years ago
Created attachment 8601602 [details] [diff] [review]
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.
Attachment #8601602 - Flags: review?(ehsan)
(Assignee)

Comment 2

3 years ago
Created attachment 8601603 [details] [diff] [review]
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.
Attachment #8601603 - Flags: review?(ehsan)
(Assignee)

Comment 3

3 years ago
Created attachment 8601605 [details] [diff] [review]
part 3a - manually convert TemporaryRef<T> to already_AddRefed<T> in RefPtr.h

sed-converting things would have caused quite some havoc. ;)
Attachment #8601605 - Flags: review?(ehsan)
(Assignee)

Comment 4

3 years ago
Created attachment 8601606 [details] [diff] [review]
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".
Attachment #8601606 - Flags: review?(ehsan)
(Assignee)

Comment 5

3 years ago
Created attachment 8601607 [details] [diff] [review]
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.
Attachment #8601607 - Flags: review?(ehsan)
(Assignee)

Comment 6

3 years ago
Created attachment 8601609 [details] [diff] [review]
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.
Attachment #8601609 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Depends on: 1116905, 1160485

Updated

3 years ago
Attachment #8601602 - Flags: review?(ehsan) → review+

Comment 7

3 years ago
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 8

3 years ago
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 9

3 years ago
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+

Updated

3 years ago
Attachment #8601607 - Flags: review?(ehsan) → review+

Comment 10

3 years ago
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+
(Assignee)

Updated

3 years ago
Depends on: 1162026
(Assignee)

Comment 11

3 years ago
Landed part 1 to make people's lives easier:

https://hg.mozilla.org/integration/mozilla-inbound/rev/b45c68b9dceb
Keywords: leave-open
(Assignee)

Updated

3 years ago
Depends on: 1175621
(Assignee)

Comment 13

3 years ago
Created attachment 8623826 [details] [diff] [review]
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...
Attachment #8623826 - Flags: review+
(Assignee)

Comment 14

3 years ago
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)
(Assignee)

Updated

3 years ago
Attachment #8601606 - Attachment is obsolete: true
(Assignee)

Comment 17

3 years ago
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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

3 years ago
Depends on: 1180105

Updated

3 years ago
Duplicate of this bug: 1115033
You need to log in before you can comment on or make changes to this bug.