Last Comment Bug 716286 - Fixup how we handle ScaledFont interaction between skia and mac
: Fixup how we handle ScaledFont interaction between skia and mac
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Jeff Muizelaar [:jrmuizel]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-07 14:50 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-01-30 05:23 PST (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
A quick version of what this could look like (10.10 KB, patch)
2012-01-07 14:53 PST, Jeff Muizelaar [:jrmuizel]
matt.woodrow: feedback+
Details | Diff | Review
A version that should be good enough to review (8.41 KB, patch)
2012-01-08 17:20 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Review
V2 without multiple inheritence (15.13 KB, patch)
2012-01-09 10:57 PST, Jeff Muizelaar [:jrmuizel]
matt.woodrow: review+
Details | Diff | Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-07 14:50:23 PST

    
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-01-07 14:53:20 PST
Created attachment 586735 [details] [diff] [review]
A quick version of what this could look like

This is pretty similar to what's currently there: the difference is that instead of creating the SkiaFont on creation of the ScaledFont it is created on first use and cached inside the ScaledFontMac. We also keep around the cgFont as that's what is used by the CG backend.

I've done this up using inheritance because that seemed to work well enough. For cairo we run into the same thing and we can either intermingle the Skia and cairo implementation in a single ScaledFontBase or use multiple inheritence.
Comment 2 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-01-08 15:20:57 PST
Comment on attachment 586735 [details] [diff] [review]
A quick version of what this could look like

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

Concept looks good to me!
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-01-08 17:20:56 PST
Created attachment 586866 [details] [diff] [review]
A version that should be good enough to review
Comment 4 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-01-08 17:37:35 PST
Comment on attachment 586866 [details] [diff] [review]
A version that should be good enough to review

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

::: gfx/2d/Factory.cpp
@@ -136,5 @@
>  #endif
> -  case NATIVE_FONT_SKIA_FONT_FACE:
> -    {
> -      return new ScaledFontSkia(static_cast<gfxFont*>(aNativeFont.mFont), aSize);
> -    }

Removing this will break fonts on android. We could fix this as a followup though.

::: gfx/2d/ScaledFontMac.h
@@ +47,3 @@
>  
>  namespace mozilla {
>  namespace gfx {

Probably need to make this only inherit from ScaledFromSkia if USE_SKIA is defined.

::: gfx/2d/ScaledFontSkia.cpp
@@ +61,4 @@
>    return SkTypeface::kNormal;
>  }
>  
> +#endif

Again, needed for android.

@@ +76,5 @@
>  
>  TemporaryRef<Path>
>  ScaledFontSkia::GetPathForGlyphs(const GlyphBuffer &aBuffer, const DrawTarget *aTarget)
>  {
> +#ifdef USE_SKIA

This is always defined if this file is being compiled

@@ +79,5 @@
>  {
> +#ifdef USE_SKIA
> +  if (aTarget->GetType() == BACKEND_SKIA) {
> +    SkPaint paint;
> +    paint.setTypeface(mTypeface);

Use GetSkTypeface instead of mTypeface incase it hasn't been initialized yet? Maybe that isn't possible, but it seems clearer.

::: gfx/2d/ScaledFontWin.h
@@ +50,4 @@
>    ScaledFontWin(gfxGDIFont* aFont, Float aSize);
>  
>    virtual FontType GetType() const { return FONT_GDI; }
> +  virtual SkTypeface* GetSkTypeface();

Missing the same two ifdefs as ScaledFontMac
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-01-09 10:51:26 PST
(In reply to Matt Woodrow (:mattwoodrow) from comment #4)
> Comment on attachment 586866 [details] [diff] [review]
> A version that should be good enough to review
> 
> Review of attachment 586866 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/2d/Factory.cpp
> @@ -136,5 @@
> >  #endif
> > -  case NATIVE_FONT_SKIA_FONT_FACE:
> > -    {
> > -      return new ScaledFontSkia(static_cast<gfxFont*>(aNativeFont.mFont), aSize);
> > -    }
> 
> Removing this will break fonts on android. We could fix this as a followup
> though.

Shouldn't we have a better way of getting fonts in Skia then having to look them up again. Also, aren't webfonts broken with the current implementation? Is there a way we can use the FT_Face?
Comment 6 Jeff Muizelaar [:jrmuizel] 2012-01-09 10:57:28 PST
Created attachment 587050 [details] [diff] [review]
V2 without multiple inheritence

The solution using multiple inheritance wasn't going to work out well. So I've reworked the patch so that ScaledFontMac just inherits from ScaledFontBase. We can put the cairo and skia specific bits in there and that should work out better than the multiple inheritance route would.
Comment 7 Matt Woodrow (:mattwoodrow) (PTO until 27 June) 2012-01-09 20:58:03 PST
> Shouldn't we have a better way of getting fonts in Skia then having to look
> them up again. Also, aren't webfonts broken with the current implementation?
> Is there a way we can use the FT_Face?

That would be nice, but I can't see a way. We could probably add this to skia and upstream the patch.
Comment 8 Jeff Muizelaar [:jrmuizel] 2012-01-18 08:01:48 PST
This got folded into bug 692879

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