719627 - Remove ScaledFontCairo in favour of merging its functionality into ScaledFontBase

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Joe Drew (not getting mail)]

Attachment: Merge ScaledFontCairo into ScaledFontBase, and support it on OS X

It's easiest to pass cairo_scaled_font_t from the gfxFont down into mozilla::gfx::Factory, so I've created a way to do that, supported only on backends/platforms that use ScaledFontBase.

I'm very interested in people's feedback on this.

Comment 1

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 590014 [details] [diff] [review]
Merge ScaledFontCairo into ScaledFontBase, and support it on OS X

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

::: gfx/2d/2D.h
@@ +780,5 @@
> +   * must be used when using the Cairo backend.
> +   */
> +  static TemporaryRef<ScaledFont>
> +    CreateScaledFont(const NativeFont &aNativeFont, Float aSize, cairo_scaled_font_t* aScaledFont);
> +

I'm not thrilled with this name. It doesn't reflect the fact that the scaled_font needs to be the same as native font. Personally, I'd prefer this to pull the native font out of the cairo_scaled_font so you might want to add your justification for not doing so.

::: gfx/2d/Factory.cpp
@@ +166,2 @@
>      }
> +#endif

This needs to go. You'd prefer to do it in another bug.

::: gfx/2d/ScaledFontBase.cpp
@@ +37,5 @@
>  
>  #include "ScaledFontBase.h"
> +
> +#include "gfxFont.h"
> +

You can't include 'gfxFont.h'

@@ +83,5 @@
> +#endif
> +#ifdef USE_CAIRO
> +  mScaledFont = aFont->GetCairoScaledFont();
> +  cairo_scaled_font_reference(mScaledFont);
> +#endif

This is more of the stuff we shouldn't have. i.e. We should have a ScaledFontFreeType that we use for linux here, instead of the skia lookup craziness.

@@ +163,5 @@
> +#ifdef USE_CAIRO
> +void
> +ScaledFontBase::SetCairoScaledFont(cairo_scaled_font_t* font)
> +{
> +  cairo_scaled_font_destroy(mScaledFont);

I don't know if we want the ability to change this. I'd rather it be set once and that's all you get. You can add an assert!

::: gfx/2d/ScaledFontBase.h
@@ +58,5 @@
>    ScaledFontBase(Float aSize);
>    virtual ~ScaledFontBase();
>  
>    virtual TemporaryRef<Path> GetPathForGlyphs(const GlyphBuffer &aBuffer, const DrawTarget *aTarget);
> +  ScaledFontBase(gfxFont* aFont, Float aSize);

This is more of the gfxFont stuff sneaking out, please leave it skia only.

@@ +74,5 @@
>    friend class DrawTargetSkia;
>  #ifdef USE_SKIA
>    SkTypeface* mTypeface;
>  #endif
> +#ifdef USE_SKIA

USE_CAIRO?

Comment 2

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/64c5a333c0f6

Comment 3

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/64c5a333c0f6