use moves inside WrapRunnable* functions

RESOLVED FIXED in Firefox 42

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

(1 attachment)

Assignee

Description

4 years ago
Using moves inside WrapRunnable* when creating the actual runnable has two benefits:

1. We avoid some unnecessary copying of things.  (Any necessary copies are already done by the call to WrapRunnable*, assuming I understand C++ semantics.)
2. We correctly handle objects that have move constructors, but don't have copy constructors.

#2 is especially important to me in the context of bug 1162026, since TemporaryRef<T> has a copy constructor, but converting TemporaryRef<T> usages to already_AddRefed<T> causes issues, since already_AddRefed<T> doesn't have a copy constructor.
Assignee

Comment 1

4 years ago
Calls to WrapRunnable* copy their arguments already; we don't need to
copy them a second time when constructing the actual runnable.  In
addition to making things more efficient, this change also permits calls
to WrapRunnable to correctly handle objects that can only be moved, and
not copied.
Attachment #8623806 - Flags: review?(ekr)
backlog: --- → parking-lot
Assignee

Comment 2

4 years ago
Comment on attachment 8623806 [details] [diff] [review]
make WrapRunnable* more efficient by utilizing moves in wrapper functions

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

jesup suggested Byron might be a lower-latency reviewer.
Attachment #8623806 - Flags: review?(ekr) → review?(docfaraday)
Comment on attachment 8623806 [details] [diff] [review]
make WrapRunnable* more efficient by utilizing moves in wrapper functions

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

LGTM
Attachment #8623806 - Flags: review?(docfaraday) → review+
https://hg.mozilla.org/mozilla-central/rev/468e862d122a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.