Closed Bug 1232576 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(2 files)

This is a precursor to passing a DrawTarget to *many* font functions instead of a gfxContext.
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
Having GetRefCairo() as a static function in gfxContext seems as good a place
as any.
Attachment #8698328 - Flags: review?(jfkthame)
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+
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.
I'll also rename GetRefCairo() as RefCairo(), because it never returns nullptr.
> 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.
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.
Blocks: 1232822
https://hg.mozilla.org/mozilla-central/rev/da650be00ba7
https://hg.mozilla.org/mozilla-central/rev/ef8a998219bd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.