Remove ScaledFontCairo in favour of merging its functionality into ScaledFontBase

RESOLVED FIXED in mozilla12

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Joe Drew (not getting mail))

Tracking

(Blocks: 1 bug)

Trunk
mozilla12
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 590014 [details] [diff] [review]
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.
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+
(Assignee)

Comment 2

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c5a333c0f6
Assignee: nobody → joe
Target Milestone: --- → mozilla12
(Assignee)

Comment 3

5 years ago
https://hg.mozilla.org/mozilla-central/rev/64c5a333c0f6
Status: NEW → RESOLVED
Last Resolved: 5 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.