1232822 - Moz2Dify several font-related functions

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) - Moz2Dify SetupCairoFont()

I wonder if using the parameter name |aRefDT| instead of |aDrawTarget| would be
useful. It could be used to indicate DrawTargets that are only used for
SetupCairoFont() calls.

Comment 2

[Nicholas Nethercote [inactive]]

Attachment: (part 2) - Moz2Dify SetupGlyphExtents()

The gfxContextMatrixAutoSaveRestore in SetupGlyphExtents() is now obviously not
needed, and the rest is trivial.

Comment 3

[Nicholas Nethercote [inactive]]

Attachment: (part 3) - Moz2Dify gfxFont::CalcXScale() and gfxFont::PostShapingFixup()

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.

Comment 4

[Nicholas Nethercote [inactive]]

Attachment: (part 4) - Remove unused argument from SetPotentialLineBreaks()

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: (part 5) - Moz2Dify GetRoundOffsetsToPixels()

Make GetRoundOffsetsToPixel() take a DrawTarget instead of a gfxContext. This
requires moving it into gfxFontShaper. The only change of note within the
function is the use of aDrawTarget->GetTransform().HasNonTranslation() instead
of CurrentMatrix().HasNonTranslation().

Comment 6

[Nicholas Nethercote [inactive]]

Attachment: (part 6) - Move RefCairo() from gfxContext to gfxFont

Because gfxContext will one day disappear, and gfxFont seems like a good place
to move it to.

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8698786 [details] [diff] [review]
(part 2) - Moz2Dify SetupGlyphExtents()

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

Hmm, I wonder what that gfxContextMatrixAutoSaveRestore in SetupGlyphExtents was for, and whether we might need to do anything similar in the DT world? Or do we not set equivalent matrices on drawtargets?

If you know what's going on here and can reassure me, great; otherwise I'd like to try and understand/remember a bit better how it all relates (it's a while since I looked at this code).

Comment 8

[Nicholas Nethercote [inactive]]

> Hmm, I wonder what that gfxContextMatrixAutoSaveRestore in SetupGlyphExtents
> was for, and whether we might need to do anything similar in the DT world?
> Or do we not set equivalent matrices on drawtargets?

The short version: the gfxContextMatrixAutoSaveRestore does cause the
DrawTarget to be temporarily modified, but during the time that it is modified
the DrawTarget is only used as a means to access the refCairo stored in its 
user data. Its transform is not used during that time.

The long version: SetupGlyphExtent() calls gfxContext::SetMatrix(), which
modifies the DrawTarget like this:

> mDT->SetTransform(GetDTTransform());

i.e. it modifies the DrawTarget's transform, and also might modify a 
corresponding transform in the graphics backend (e.g. cairo_set_matrix() in the
Cairo backend; CGContextSetCTM() + CGContextConcatCTM() in the CG backend).

So the question is if any of the remaining uses of the DrawTarget touch 
the DrawTarget's transform or the backend transform/matrix. I'm pretty sure the
answer is no. Here are the remaining operations that touch the DrawTarget.
    
gfxContext::SetMatrix() calls gfxFontEntry::GetSVGGlyphExtents(). The only
thing it does with the DrawTarget is this:

> cairo_get_font_matrix(gfxFont::RefCairo(aDrawTarget), &fontMatrix);
      
That function doesn't use the DrawTarget's transform or anything in the 
backend; it just reads data from the refCairo object, which, although stored in
the DrawTarget, is separate from it.

gfxContext::SetMatrix() also does this:

> cairo_glyph_extents(gfxFont::RefCairo(aDrawTarget), &glyph, 1, &extents);

It's a similar story -- it's reading from the refCairo and so doesn't touch the
DrawTarget.

Finally, at the end of SetupGlyphExten() the gfxContextMatrixAutoSaveRestore
restores the transforms in the DrawTarget and the graphics backend to what they
originally were.

I have a partial try run that looks good. I'll do a fuller one now.

Comment 9

[Nicholas Nethercote [inactive]]

> I have a partial try run that looks good. I'll do a fuller one now.

Looks good: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a0191cf672eb

Comment 10

[Jonathan Kew [:jfkthame]]

Comment on attachment 8698786 [details] [diff] [review]
(part 2) - Moz2Dify SetupGlyphExtents()

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

OK, seems like it should be fine.

Comment 11

[Jonathan Kew [:jfkthame]]

Comment on attachment 8698789 [details] [diff] [review]
(part 5) - Moz2Dify GetRoundOffsetsToPixels()

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

::: gfx/thebes/gfxGraphiteShaper.cpp
@@ +269,5 @@
>      }
>  
> +    bool roundX, roundY;
> +    gfxFontShaper::GetRoundOffsetsToPixels(aContext->GetDrawTarget(),
> +                                           &roundX, &roundY);

It shouldn't be necessary to include "gfxFontShaper::" here, should it? (Given that we're in a subclass of gfxFontShaper already.)

::: gfx/thebes/gfxHarfBuzzShaper.cpp
@@ +1624,2 @@
>      } else {
> +        gfxFontShaper::GetRoundOffsetsToPixels(drawTarget, &roundI, &roundB);

ditto

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ad2588ed167f967de511e3858626fbb227f8ba3
Bug 1232822 (part 1) - Moz2Dify SetupCairoFont(). r=jfkthame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a7f7be13a78bde48b11b4ae6f79ec2157a8b32e
Bug 1232822 (part 2) - Moz2Dify SetupGlyphExtents(). r=jfkthame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82f3102e4840d8368b4c2fd0bfa782d3646fe0e4
Bug 1232822 (part 3) - Moz2Dify gfxFont::CalcXScale() and gfxFont::PostShapingFixup(). r=jfkthame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/2474921deb87a3e31d1ed9dac11b8d2eee8790e5
Bug 1232822 (part 4) - Remove unused argument from SetPotentialLineBreaks(). r=jfkthame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7bca841b053dd36e56365c94e9c1bd8f5d6a15c
Bug 1232822 (part 5) - Moz2Dify GetRoundOffsetsToPixels(). r=jfkthame.

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ad3f98c4c65b94b8576eeecd3a6685c13777358
Bug 1232822 (part 6) - Move RefCairo() from gfxContext to gfxFont. r=jfkthame.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ad2588ed167
https://hg.mozilla.org/mozilla-central/rev/6a7f7be13a78
https://hg.mozilla.org/mozilla-central/rev/82f3102e4840
https://hg.mozilla.org/mozilla-central/rev/2474921deb87
https://hg.mozilla.org/mozilla-central/rev/d7bca841b053
https://hg.mozilla.org/mozilla-central/rev/7ad3f98c4c65