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

Status: RESOLVED FIXED

(Core :: Graphics: Text, defect)

Description

[Nicholas Nethercote [inactive]]

This is a precursor to passing a DrawTarget to *many* font functions instead of a gfxContext.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: (part 1) - Move the reference |cairo_t*| from gfxContext to DrawTarget (as user data)

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (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.

Comment 3

[Nicholas Nethercote [inactive]]

Try looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b81c146eb771

Comment 4

[Jonathan Kew [:jfkthame]]

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.

Comment 5

[Jonathan Kew [:jfkthame]]

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.

Comment 6

[Nicholas Nethercote [inactive]]

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.

Comment 7

[Nicholas Nethercote [inactive]]

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

Comment 8

[Nicholas Nethercote [inactive]]

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

Comment 9

[Nicholas Nethercote [inactive]]

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.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/da650be00ba7
https://hg.mozilla.org/mozilla-central/rev/ef8a998219bd