Closed Bug 1232822 Opened 4 years ago Closed 4 years ago

Moz2Dify several font-related functions

Categories

(Core :: Graphics: Text, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: njn, Assigned: njn)

References

Details

Attachments

(6 files)

This is a precursor to passing a DrawTarget to *many* font functions instead of a gfxContext.
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.
Attachment #8698785 - Flags: review?(jfkthame)
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
The gfxContextMatrixAutoSaveRestore in SetupGlyphExtents() is now obviously not
needed, and the rest is trivial.
Attachment #8698786 - Flags: review?(jfkthame)
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.
Attachment #8698787 - Flags: review?(jfkthame)
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().
Attachment #8698789 - Flags: review?(jfkthame)
Because gfxContext will one day disappear, and gfxFont seems like a good place
to move it to.
Attachment #8698790 - Flags: review?(jfkthame)
Attachment #8698785 - Flags: review?(jfkthame) → review+
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).
> 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.
> 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 on attachment 8698786 [details] [diff] [review]
(part 2) - Moz2Dify SetupGlyphExtents()

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

OK, seems like it should be fine.
Attachment #8698786 - Flags: review?(jfkthame) → review+
Attachment #8698787 - Flags: review?(jfkthame) → review+
Attachment #8698788 - Flags: review?(jfkthame) → review+
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
Attachment #8698789 - Flags: review?(jfkthame) → review+
Attachment #8698790 - Flags: review?(jfkthame) → review+
You need to log in before you can comment on or make changes to this bug.