Move the reference |cairo_t*| from gfxContext to DrawTarget (as user data).

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

unspecified
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox46 fixed)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
This is a precursor to passing a DrawTarget to *many* font functions instead of a gfxContext.
(Assignee)

Comment 1

3 years ago
Created attachment 8698325 [details] [diff] [review]
(part 1) - Move the reference |cairo_t*| from gfxContext to DrawTarget (as user data)
Attachment #8698325 - Flags: review?(jfkthame)
(Assignee)

Updated

3 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8698328 [details] [diff] [review]
(part 2) - Rename gfxContext::GetCairo() as GetRefCairo() and make it static

Having GetRefCairo() as a static function in gfxContext seems as good a place
as any.
Attachment #8698328 - Flags: review?(jfkthame)
(Assignee)

Updated

3 years ago
Blocks: 1231550
Comment on attachment 8698325 [details] [diff] [review]
(part 1) - Move the reference |cairo_t*| from gfxContext to DrawTarget (as user data)

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

::: gfx/thebes/gfxContext.cpp
@@ +131,5 @@
>  
> +// DrawTargets that don't use a Cairo backend can be given a 1x1 "reference"
> +// |cairo_t*|, stored in the DrawTarget's user data, for doing font-related
> +// operations.
> +static UserDataKey sRefCairo;

You could move this static inside GetCairo(), as that's the only place it's used.
Attachment #8698325 - Flags: review?(jfkthame) → review+
Comment on attachment 8698328 [details] [diff] [review]
(part 2) - Rename gfxContext::GetCairo() as GetRefCairo() and make it static

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

Looks fine to me (assuming it works, I didn't actually test it!)

Aren't we working towards eventual removal of gfxContext? In which case GetRefCairo() will need to find a new home (maybe as a static method of gfxFont, as it's primarily used by font subclasses?) But if you want to leave it here for now, and potentially move it when gfxContext finally reaches EOL, that's OK too.
Attachment #8698328 - Flags: review?(jfkthame) → review+
(Assignee)

Comment 6

3 years ago
Thank you for the fast reviews.

> You could move this static inside GetCairo(), as that's the only place it's
> used.

Good idea.


> Looks fine to me (assuming it works, I didn't actually test it!)

That's par for the course for reviews, no? :)
 

> Aren't we working towards eventual removal of gfxContext? In which case
> GetRefCairo() will need to find a new home (maybe as a static method of
> gfxFont, as it's primarily used by font subclasses?)

Ok, I'll move it into gfxFont.
(Assignee)

Comment 7

3 years ago
I'll also rename GetRefCairo() as RefCairo(), because it never returns nullptr.
(Assignee)

Comment 8

3 years ago
> Ok, I'll move it into gfxFont.

Actually, that'll need to wait until I move GetRoundOffsetsToPixels() into gfxFont, but I have a patch for that which I'll attach to a new bug shortly.
(Assignee)

Comment 9

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/da650be00ba7a367ef1b9fbeb710edf54dde4306
Bug 1232576 (part 1) - Move the reference |cairo_t*| from gfxContext to DrawTarget (as user data). r=jfkthame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef8a998219bd1afa187b51cc84b231f73903f285
Bug 1232576 (part 2) - Rename gfxContext::GetCairo() as GetRefCairo() and make it static. r=jfkthame.
(Assignee)

Updated

3 years ago
Blocks: 1232822

Comment 10

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/da650be00ba7
https://hg.mozilla.org/mozilla-central/rev/ef8a998219bd
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.