Closed Bug 719627 Opened 8 years ago Closed 8 years ago

Remove ScaledFontCairo in favour of merging its functionality into ScaledFontBase

Categories

(Core :: Graphics, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla12

People

(Reporter: joe, Assigned: joe)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
Attachment #590014 - Flags: review?(jmuizelaar)
Attachment #590014 - Flags: feedback?(bas.schouten)
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?
Attachment #590014 - Flags: review?(jmuizelaar) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c5a333c0f6
Assignee: nobody → joe
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/64c5a333c0f6
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Attachment #590014 - Flags: feedback?(bas)
You need to log in before you can comment on or make changes to this bug.