mfbt Function lacks functionality needed for interoperating with Skia

RESOLVED FIXED in Firefox 47

Status

()

Core
MFBT
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: lsalzman, Assigned: lsalzman)

Tracking

unspecified
mozilla47
Points:
---

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
When trying to shim mfbt Function in place of std::function inside Skia, I ran into the following deficiencies:

1) Skia copies std::function by value in several places, and we need a default copy constructor
2) Skia converts it to bool and we have no bool conversion operator
3) Skia initializes it with nullptr and we had no constructor for that
(Assignee)

Updated

3 years ago
Component: Graphics → MFBT
(Assignee)

Comment 1

3 years ago
Created attachment 8716021 [details] [diff] [review]
make mfbt Function reference-counted so that it can be cheaply copied for compatibility with Skia

This patch addresses all the listed issues and makes Skia compile cleanly.
Attachment #8716021 - Flags: review?(nfroyd)
Comment on attachment 8716021 [details] [diff] [review]
make mfbt Function reference-counted so that it can be cheaply copied for compatibility with Skia

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

r=me  It's not great that we're making Function have higher overhead; OTOH, Function should be going away soonish and it's not widely used in the first place.

::: mfbt/Function.h
@@ +45,3 @@
>  {
>  public:
> +  MOZ_DECLARE_REFCOUNTED_VIRTUAL_TYPENAME(FunctionImplBase)

Does this need to be virtual?  I guess it doesn't matter that much for debug builds...
Attachment #8716021 - Flags: review?(nfroyd) → review+

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/915eba505316
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Updated

2 years ago
See Also: → bug 1263342
You need to log in before you can comment on or make changes to this bug.