SetupCairoFont() is a leaf function that gets passed a |gfxContext*|. In all implementations it only calls gfxContext::GetCairo(). We can instead pass in a |cairo_t*| obtained with GetCairo() higher up in the call graph (in some cases, quite a lot higher up). This helps with gfxContext-to-DrawTarget conversions, because it reduces the number of functions that are exposed to a gfxContext.

The only arguable downside I can see with this patch is that, because we call GetCairo() earlier in most places, it might cause us to call GetCairo() in cases where previously we didn't. But I don't think it's expensive and so this shouldn't be a problem?

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

SetupCairoFont() is a leaf function that gets passed a |gfxContext*|. In all implementations it only calls gfxContext::GetCairo(). We can instead pass in a |cairo_t*| obtained with GetCairo() higher up in the call graph (in some cases, quite a lot higher up). This helps with gfxContext-to-DrawTarget conversions, because it reduces the number of functions that are exposed to a gfxContext. This patch also removes the |gfxContextMatrixAutoSaveRestore| in gfxFont::SetupGlyphExtents() because it was unnecessary.

SetupCairoFont() is a leaf function that gets passed a |gfxContext*|. In all implementations it only calls gfxContext::GetCairo(). We can instead pass in a |cairo_t*| obtained with GetCairo() higher up in the call graph (in some cases, quite a lot higher up). This helps with gfxContext-to-DrawTarget conversions, because it reduces the number of functions that are exposed to a gfxContext. This patch also removes the |gfxContextMatrixAutoSaveRestore| in gfxFont::SetupGlyphExtents() because it was unnecessary.

This boils down to these two lines being equivalent: Size t = aContext->UserToDevice(Size(1.0, 0.0)); Size t = aDrawTarget->GetTransform() * Size(1.0, 0.0); The rest is just plumbing.

A lot of text-related functions that take a |gfxContext*| only actually use a DrawTarget and a reference |cairo_t*|. This patch introduces ShapeTextParams, a new type that encapsulates those two things. This requires moving GetRoundOffsetsToPixel() out of |gfxContext| and passing it a |ShapeTextParams|.

This is already a huge patch and it doesn't even compile yet. There are plenty more functions that can be converted. I'm on the fence about this whole approach. It's good that it significantly reduces the amount of code exposed to gfxContext, which seems like it can only help converting gfxContext uses to DrawTarget. But I also wonder if it's just rearranging furniture. Opinions one way or the other would be welcome :) One thing worth noting is that ShapeTextParams needs a DrawTarget member only for a single case: gfxGDIFont::GetGlyphWidth(). If it wasn't for that we could instead pass a |cairo_t*| for the reference cairo instead (like in part 1 above).

(In reply to Nicholas Nethercote [:njn] from comment #0) > This helps with gfxContext-to-DrawTarget conversions, because it reduces the > number of functions that are exposed to a gfxContext. You might want to confirm that this is actually actually the case. It sounds to me as though this might hurt the conversion since I suspect we want to convert the things that use cairo_t into using APIs on DrawTarget. I'm not an expert in this area, though.

(In reply to Nicholas Nethercote [:njn] from comment #1) > Created attachment 8697114 [details] [diff] [review] > Pass a cairo_t* instead of a gfxContext* to various font functions > > The only arguable downside I can see with this patch is that, because we call > GetCairo() earlier in most places, it might cause us to call GetCairo() in > cases where previously we didn't. But I don't think it's expensive and so > this > shouldn't be a problem? Hmm. I'd like to think about this a bit... GetCairo() might be somewhat expensive in the case where the context's backend type isn't cairo. And depending on the font backend, we may or may not really need the cairo at all -- I don't think it should be necessary for Core Text, or for DirectWrite using natural widths. I wonder if we could pass a DrawTarget into the font functions instead, and then let them extract the cairo from that only if needed.

> Hmm. I'd like to think about this a bit... GetCairo() might be somewhat > expensive in the case where the context's backend type isn't cairo. Another option would be to create a wrapper type for gfxContext that restricts you so you can only look at its mDT and its mRefCairo. Then the GetCairo() calls could occur at the same time as they currently do but it would also be clear which parts of the code don't need a full gfxContext. A bit of a hack, though. > And depending on the font backend, we may or may not really need the cairo at > all -- I don't think it should be necessary for Core Text, or for > DirectWrite using natural widths. But as long as any backend needs the cairo, and all backends are using the same interface, we're stuck with the problem, correct? > I wonder if we could pass a DrawTarget into the font functions instead, and > then let them extract the cairo from that only if needed. I would love it if we could do that. It would make things a lot simpler to pass a DrawTarget everywhere instead of the new ShapeTextParams, which contains a DrawTarget and a cairo_t. Here's what GetCairo() looks like: > cairo_t * > gfxContext::GetCairo() > { > if (mDT->GetBackendType() == BackendType::CAIRO) { > cairo_t *ctx = > (cairo_t*)mDT->GetNativeSurface(NativeSurfaceType::CAIRO_CONTEXT); > if (ctx) { > return ctx; > } > } > > if (mRefCairo) { > // Set transform! > return mRefCairo; > } > > mRefCairo = cairo_create(gfxPlatform::GetPlatform()->ScreenReferenceSurface()->CairoSurface()); > > return mRefCairo; > } If we hit the first |return| statement then the DrawTarget alone is enough. It's the second and third |return| statements, involving mRefCairo, that complicate things. Is there some way we could get rid of mRefCairo? Or could we store a pointer to it in the DrawTarget's user data? I need a better understanding of this stuff: what operations are still done by Cairo, what operations are done by Moz2D but with a Cairo backend, and what operations are done by Moz2D without any Cairo involvement.

At this point I think parts 2 and 4 are definitely worth taking, and parts 1, 3 and 5 are questionable.

(In reply to Nicholas Nethercote [:njn] from comment #12) > If we hit the first |return| statement then the DrawTarget alone is enough. > It's the second and third |return| statements, involving mRefCairo, that > complicate things. Is there some way we could get rid of mRefCairo? Or could > we store a pointer to it in the DrawTarget's user data? (Huh.... I thought I'd commented on this already, but must have failed to post it, sorry.) Yes, I believe mRefCairo exists only to cache the result of cairo_create(gfxPlatform::GetPlatform()->ScreenReferenceSurface()->CairoSurface()), so ISTM we could have something like a GetCairoFromDrawTarget() helper function that encapsulates the same idea... get the cairo directly from the DT if it's cairo-backed; if not, check for one cached in the DT's userdata; if that's not present, then create it from ScreenReferenceSurface() and cache it there for next time. Then the font backends can just call GetCairoFromDrawTarget() if they need a cairo context, where they currently use gfxContext::GetCairo. And I think we can probably eliminate this entirely from some of the backends, but that can be a separate followup.

> so ISTM we could have something like a > GetCairoFromDrawTarget() helper function that encapsulates the same idea... > get the cairo directly from the DT if it's cairo-backed; if not, check for > one cached in the DT's userdata; if that's not present, then create it from > ScreenReferenceSurface() and cache it there for next time. I created bug 1232576 to do this part.

Apologies for the monster patch. Take your time for the review :)

(Updated gfxWordCacheTest.cpp, which is NPOTB.)

Comment on attachment 8699696 [details] [diff] [review] Use DrawTarget instead of gfxContext and/or nsRenderingContext in many places in font/text code Review of attachment 8699696 [details] [diff] [review]: ----------------------------------------------------------------- Looks fine, afaics. ::: layout/generic/MathMLTextRunFactory.cpp @@ +528,5 @@ > #define TT_DTLS TRUETYPE_TAG('d', 't', 'l', 's') > > void > MathMLTextRunFactory::RebuildTextRun(nsTransformedTextRun* aTextRun, > + mozilla::gfx::DrawTarget* aRefDrawTarget, No need for the "mozilla::" here, there's a "using namespace..." at the top of the file. ::: layout/generic/TextOverflow.cpp @@ +33,2 @@ > public: > + typedef mozilla::gfx::DrawTarget DrawTarget; "mozilla::" shouldn't be needed. ::: layout/generic/nsFrame.h @@ +272,5 @@ > ComputeSizeFlags aFlags) override; > > // Compute tight bounds assuming this frame honours its border, background > // and outline, its children's tight bounds, and nothing else. > + nsRect ComputeSimpleTightBounds(mozilla::gfx::DrawTarget* aDrawTarget) const; Shouldn't need the "mozilla::gfx::" prefix here, I think, as you're adding a typedef in nsIFrame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/290f666471a0177f984f05d5c9933914d2d11443 Bug 1231550 - Use DrawTarget instead of gfxContext and/or nsRenderingContext in many places in font/text code. r=jfkthame.