725240 - Re-enable font hinting for "mobile" when we don't care about non-reflowing-zoom

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

There are several GFX_OPTIMIZE_MOBILE "turds" in our FT2 code that summarily disable hinting for all mobile platforms.  We only care about that when non-reflowing-zoom might be used.  Currently that's only in content processes in xul-fennec and the gecko thread in new-fennec.  Turning off hinting makes font rendering look like crap everywhere else.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Re-enable hinting on 'mobile' when non-reflowing-zoom isn't going to be used

Hi John, just so you know, this bug is pretty high priority for b2g.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Also, I'm not sure who to tag for review here.  Please kick it to whomever is most appropriate.

Comment 3

[Jonathan Kew [:jfkthame]]

Comment on attachment 595356 [details] [diff] [review]
Re-enable hinting on 'mobile' when non-reflowing-zoom isn't going to be used

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

::: gfx/thebes/Makefile.in
@@ +275,5 @@
>  	gfxFT2FontList.cpp \
>  	gfxPDFSurface.cpp \
>  	nsUnicodeRange.cpp \
>  	$(NULL)
> +DEFINES +=	-DMOZ_ANDROID_WIDGETRY

Please clarify what this #define is for - why do we need something distinct from MOZ_WIDGET_TOOLKIT with its various existing values?

::: gfx/thebes/gfxFT2FontList.cpp
@@ +279,5 @@
>      FT2FontEntry *fe = new FT2FontEntry(fontName);
>      fe->mItalic = aFace->style_flags & FT_STYLE_FLAG_ITALIC;
>      fe->mFTFace = aFace;
> +    int flags = gfxFT2Font::EnableHinting() ?
> +                0  :

While you're here, let's change the 0 to FT_LOAD_DEFAULT. And lose the extra space before the colon.

@@ +329,5 @@
>          FT_Face face;
>          FT_New_Face(gfxToolkitPlatform::GetPlatform()->GetFTLibrary(), mFilename.get(), mFTFontIndex, &face);
>          mFTFace = face;
> +        int flags = gfxFT2Font::EnableHinting() ?
> +                    0  :

Ditto.

::: gfx/thebes/gfxFT2Fonts.cpp
@@ +467,5 @@
>      return true;
>  }
>  
> +/*static*/ bool
> +gfxFT2Font::EnableHinting()

I realize gfxFT2Font already features a bunch of platform-specific #ifdef gunk, but I'd like us to be reducing that rather than adding to it. It might be neater to make this a virtual function EnableFontHinting() on gfxPlatform, with a default implementation just returning true, and then override this appropriately in gfxAndroidPlatform. WDYT?

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jonathan Kew (:jfkthame) from comment #3)
> Comment on attachment 595356 [details] [diff] [review]
> Re-enable hinting on 'mobile' when non-reflowing-zoom isn't going to be used
> 
> Review of attachment 595356 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/thebes/Makefile.in
> @@ +275,5 @@
> >  	gfxFT2FontList.cpp \
> >  	gfxPDFSurface.cpp \
> >  	nsUnicodeRange.cpp \
> >  	$(NULL)
> > +DEFINES +=	-DMOZ_ANDROID_WIDGETRY
> 
> Please clarify what this #define is for - why do we need something distinct
> from MOZ_WIDGET_TOOLKIT with its various existing values?
> 

MOZ_WIDGET_TOOLKIT is a gmake macro.  It's not reflected into cpp.

> ::: gfx/thebes/gfxFT2FontList.cpp
> @@ +279,5 @@
> >      FT2FontEntry *fe = new FT2FontEntry(fontName);
> >      fe->mItalic = aFace->style_flags & FT_STYLE_FLAG_ITALIC;
> >      fe->mFTFace = aFace;
> > +    int flags = gfxFT2Font::EnableHinting() ?
> > +                0  :
> 
> While you're here, let's change the 0 to FT_LOAD_DEFAULT. And lose the extra
> space before the colon.
> 

np

> ::: gfx/thebes/gfxFT2Fonts.cpp
> @@ +467,5 @@
> >      return true;
> >  }
> >  
> > +/*static*/ bool
> > +gfxFT2Font::EnableHinting()
> 
> I realize gfxFT2Font already features a bunch of platform-specific #ifdef
> gunk, but I'd like us to be reducing that rather than adding to it. It might
> be neater to make this a virtual function EnableFontHinting() on
> gfxPlatform, with a default implementation just returning true, and then
> override this appropriately in gfxAndroidPlatform. WDYT?

I'm not particularly against this.  There are two issues though
 - we currently use gfxAndroidPlatform for gonk (b2g), because gfx on android and gonk is pretty much the same.  We might want to diverge in the future, but for now we'll end up with two files that are identical except for the EnableFontHinting() setting.  (Or we just push the ifdef'ery into gfxAndroidPlatform.)
 - the EnableFontHinting() bit isn't going to be honored anywhere outside of the FT2 backend.  It might be a little confusing to try to flip that off outside of FT2 and nothing happen.

I'm happy to do whatever you tell me to :).

Comment 5

[Jonathan Kew [:jfkthame]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> MOZ_WIDGET_TOOLKIT is a gmake macro.  It's not reflected into cpp.

Duh. Of course. And I suppose the existing ANDROID define isn't sufficient because you're distinguishing between when the code is used on Android vs Gonk.

I'm not thrilled with the name MOZ_ANDROID_WIDGETRY, though - it doesn't seem at all clear what this is actually about. Any chance you can come up with something more meaningful?

> > I realize gfxFT2Font already features a bunch of platform-specific #ifdef
> > gunk, but I'd like us to be reducing that rather than adding to it. It might
> > be neater to make this a virtual function EnableFontHinting() on
> > gfxPlatform, with a default implementation just returning true, and then
> > override this appropriately in gfxAndroidPlatform. WDYT?
> 
> I'm not particularly against this.  There are two issues though
>  - we currently use gfxAndroidPlatform for gonk (b2g), because gfx on
> android and gonk is pretty much the same.  We might want to diverge in the
> future, but for now we'll end up with two files that are identical except
> for the EnableFontHinting() setting.  (Or we just push the ifdef'ery into
> gfxAndroidPlatform.)

I'd prefer to have such #ifdef-ery in gfxAndroidPlatform rather than gfxFT2Fonts, I think - and if gonk's needs at the gfxPlatform level begin to diverge in more than just this one thing, you could then have a gfxGonkPlatform, either completely separate from gfxAndroidPlatform or as a subclass that overrides a few things.

>  - the EnableFontHinting() bit isn't going to be honored anywhere outside of
> the FT2 backend.  It might be a little confusing to try to flip that off
> outside of FT2 and nothing happen.

True, it wouldn't have any effect on other backends, at least for now.

My inclination is still to do this on gfxPlatform, it feels like the more appropriate place. Maybe include a comment noting that the setting is currently only implemented in the FT2 backend.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to Jonathan Kew (:jfkthame) from comment #5)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> > MOZ_WIDGET_TOOLKIT is a gmake macro.  It's not reflected into cpp.
> 
> Duh. Of course. And I suppose the existing ANDROID define isn't sufficient
> because you're distinguishing between when the code is used on Android vs
> Gonk.
> 

Correct.  "ANDROID" in cpp basically means "using bionic".

> I'm not thrilled with the name MOZ_ANDROID_WIDGETRY, though - it doesn't
> seem at all clear what this is actually about. Any chance you can come up
> with something more meaningful?
> 

I can try, I guess.  All the macro is supposed to mean is "using the android widget code", as opposed to b2g.

> > > I realize gfxFT2Font already features a bunch of platform-specific #ifdef
> > > gunk, but I'd like us to be reducing that rather than adding to it. It might
> > > be neater to make this a virtual function EnableFontHinting() on
> > > gfxPlatform, with a default implementation just returning true, and then
> > > override this appropriately in gfxAndroidPlatform. WDYT?
> > 
> > I'm not particularly against this.  There are two issues though
> >  - we currently use gfxAndroidPlatform for gonk (b2g), because gfx on
> > android and gonk is pretty much the same.  We might want to diverge in the
> > future, but for now we'll end up with two files that are identical except
> > for the EnableFontHinting() setting.  (Or we just push the ifdef'ery into
> > gfxAndroidPlatform.)
> 
> I'd prefer to have such #ifdef-ery in gfxAndroidPlatform rather than
> gfxFT2Fonts, I think - and if gonk's needs at the gfxPlatform level begin to
> diverge in more than just this one thing, you could then have a
> gfxGonkPlatform, either completely separate from gfxAndroidPlatform or as a
> subclass that overrides a few things.
> 

wfm

> >  - the EnableFontHinting() bit isn't going to be honored anywhere outside of
> > the FT2 backend.  It might be a little confusing to try to flip that off
> > outside of FT2 and nothing happen.
> 
> True, it wouldn't have any effect on other backends, at least for now.
> 
> My inclination is still to do this on gfxPlatform, it feels like the more
> appropriate place. Maybe include a comment noting that the setting is
> currently only implemented in the FT2 backend.

will do

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Re-enable hinting on 'mobile' when non-reflowing-zoom isn't going to be used

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d13825a03073

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d13825a03073