716286 - Fixup how we handle ScaledFont interaction between skia and mac

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Attachment: A quick version of what this could look like

This is pretty similar to what's currently there: the difference is that instead of creating the SkiaFont on creation of the ScaledFont it is created on first use and cached inside the ScaledFontMac. We also keep around the cgFont as that's what is used by the CG backend.

I've done this up using inheritance because that seemed to work well enough. For cairo we run into the same thing and we can either intermingle the Skia and cairo implementation in a single ScaledFontBase or use multiple inheritence.

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 586735 [details] [diff] [review]
A quick version of what this could look like

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

Concept looks good to me!

Comment 3

[Jeff Muizelaar [:jrmuizel]]

Attachment: A version that should be good enough to review

Comment 4

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 586866 [details] [diff] [review]
A version that should be good enough to review

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

::: gfx/2d/Factory.cpp
@@ -136,5 @@
>  #endif
> -  case NATIVE_FONT_SKIA_FONT_FACE:
> -    {
> -      return new ScaledFontSkia(static_cast<gfxFont*>(aNativeFont.mFont), aSize);
> -    }

Removing this will break fonts on android. We could fix this as a followup though.

::: gfx/2d/ScaledFontMac.h
@@ +47,3 @@
>  
>  namespace mozilla {
>  namespace gfx {

Probably need to make this only inherit from ScaledFromSkia if USE_SKIA is defined.

::: gfx/2d/ScaledFontSkia.cpp
@@ +61,4 @@
>    return SkTypeface::kNormal;
>  }
>  
> +#endif

Again, needed for android.

@@ +76,5 @@
>  
>  TemporaryRef<Path>
>  ScaledFontSkia::GetPathForGlyphs(const GlyphBuffer &aBuffer, const DrawTarget *aTarget)
>  {
> +#ifdef USE_SKIA

This is always defined if this file is being compiled

@@ +79,5 @@
>  {
> +#ifdef USE_SKIA
> +  if (aTarget->GetType() == BACKEND_SKIA) {
> +    SkPaint paint;
> +    paint.setTypeface(mTypeface);

Use GetSkTypeface instead of mTypeface incase it hasn't been initialized yet? Maybe that isn't possible, but it seems clearer.

::: gfx/2d/ScaledFontWin.h
@@ +50,4 @@
>    ScaledFontWin(gfxGDIFont* aFont, Float aSize);
>  
>    virtual FontType GetType() const { return FONT_GDI; }
> +  virtual SkTypeface* GetSkTypeface();

Missing the same two ifdefs as ScaledFontMac

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 586866 [details] [diff] [review]
> A version that should be good enough to review
> 
> Review of attachment 586866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Factory.cpp
> @@ -136,5 @@
> >  #endif
> > -  case NATIVE_FONT_SKIA_FONT_FACE:
> > -    {
> > -      return new ScaledFontSkia(static_cast<gfxFont*>(aNativeFont.mFont), aSize);
> > -    }
> 
> Removing this will break fonts on android. We could fix this as a followup
> though.

Shouldn't we have a better way of getting fonts in Skia then having to look them up again. Also, aren't webfonts broken with the current implementation? Is there a way we can use the FT_Face?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: V2 without multiple inheritence

The solution using multiple inheritance wasn't going to work out well. So I've reworked the patch so that ScaledFontMac just inherits from ScaledFontBase. We can put the cairo and skia specific bits in there and that should work out better than the multiple inheritance route would.

Comment 7

[Matt Woodrow (:mattwoodrow)]

> Shouldn't we have a better way of getting fonts in Skia then having to look
> them up again. Also, aren't webfonts broken with the current implementation?
> Is there a way we can use the FT_Face?

That would be nice, but I can't see a way. We could probably add this to skia and upstream the patch.

Comment 8

[Jeff Muizelaar [:jrmuizel]]

This got folded into bug 692879