Last Comment Bug 719627 - Remove ScaledFontCairo in favour of merging its functionality into ScaledFontBase
: Remove ScaledFontCairo in favour of merging its functionality into ScaledFont...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Joe Drew (not getting mail)
:
Mentors:
Depends on:
Blocks: 694351
  Show dependency treegraph
 
Reported: 2012-01-19 15:27 PST by Joe Drew (not getting mail)
Modified: 2013-11-20 20:31 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Merge ScaledFontCairo into ScaledFontBase, and support it on OS X (16.52 KB, patch)
2012-01-19 15:27 PST, Joe Drew (not getting mail)
jmuizelaar: review+
Details | Diff | Review

Description Joe Drew (not getting mail) 2012-01-19 15:27:06 PST
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.
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-01-23 12:58:35 PST
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) 2012-01-27 12:07:27 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/64c5a333c0f6
Comment 3 Joe Drew (not getting mail) 2012-01-28 19:13:46 PST
https://hg.mozilla.org/mozilla-central/rev/64c5a333c0f6

Note You need to log in before you can comment on or make changes to this bug.