Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Remove ScaledFontCairo in favour of merging its functionality into ScaledFontBase

RESOLVED FIXED in mozilla12

Status

()

Core
Graphics
RESOLVED FIXED
6 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

6 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

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

Comment 3

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