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

RESOLVED FIXED in mozilla13

Status

()

Core
Graphics
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Trunk
mozilla13
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 595356 [details] [diff] [review]
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.
Assignee: nobody → jones.chris.g
Attachment #595356 - Flags: review?(jdaggett)
Also, I'm not sure who to tag for review here.  Please kick it to whomever is most appropriate.

Updated

5 years ago
Attachment #595356 - Flags: review?(jdaggett) → review?(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?
(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 :).
(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.
(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
Created attachment 595479 [details] [diff] [review]
Re-enable hinting on 'mobile' when non-reflowing-zoom isn't going to be used
Attachment #595356 - Attachment is obsolete: true
Attachment #595356 - Flags: review?(jfkthame)
Attachment #595479 - Flags: review?(jfkthame)
Attachment #595479 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/d13825a03073
Whiteboard: [inbound]
https://hg.mozilla.org/mozilla-central/rev/d13825a03073
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla13
You need to log in before you can comment on or make changes to this bug.