762221 - Enable font hinting in "app" content processes

Status: RESOLVED FIXED

(Core :: Graphics, defect, P2)

Description

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

Currently we disable hinting for all content processes, assuming that their content is usually going to be non-reflow-zoomed.  That's true of content processes used for browser tabs.

Content processes used for "web apps" will basically never be non-reflow-zoomed.  We need to enable font hinting for those content processes.

(This gets a little complicated if content process load both browser tabs and apps, but this is highly unlikely to ever happen.)

Comment 1

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

I'm not sure which flags to use here but we need this for b2g v1.

Comment 2

[Jason Smith [:jsmith]]

Could you clarify the use cases here in the context of a web developer building web apps for this bug? Trying to understand the value-added benefit of this. Also, outside of B2G, does this provide value-added benefit to the desktop web runtime implementation?

Comment 3

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

Can you help me by asking questions wrt comment 0?  In general,
 no hinting => fonts look bad on low res devices, but the content can be zoomed without reflowing
 hinting => fonts look better, but content has to be reflowed when zooming

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe Jonathan can take this?

Comment 5

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

Sure, but this is pretty trivial --- we want to send down the "is for app" bit when we create content processes [1], and then check that bit here [2].

[1] http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.h#87
[2] http://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxAndroidPlatform.cpp#176

Comment 6

[Andreas Gal :gal]

How are we doing here?

Comment 7

[Andreas Gal :gal]

Renom if you think we can't ship a v1 without this. That having said, this sounds trivial. Would love to have it.

Comment 8

[Jason Smith [:jsmith]]

Per IRC conversations with a few other folks, I think the best course of action if there is disagreement on whether this blocks or not is to do the following:

- Move the blocking-basecamp flag to ? for re-evaluation
- Indicate a rationale for why you disagree

Comment 9

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

If we ship v1 without this, all apps (except browser UI and status bar) will have text that looks like crap compared to competitors'.  I don't think we can do that.

Since this is a few lines' patch, we're in the territory of spending more time arguing than would be needed to actually write the patch.

Comment 10

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

Attachment: Enable font hinting for "app" processes

gfxAndroidPlatform change --> jfkthame, isForApp plumbing --> jlebar.

Comment 11

[Justin Lebar (not reading bugmail)]

This will also enable hinting for browser tabs.  Is that what you want?  (IOW, we're hinting everywhere in b2g.)

Comment 12

[Justin Lebar (not reading bugmail)]

Comment on attachment 650375 [details] [diff] [review]
Enable font hinting for "app" processes

Reading more in the code, I see that you /don't/ want to hint browser tabs.

I think you need is-for-browser in addition (or even, instead of) is-for-app.  At least, afaict from ContentParent::CreateBrowser.

Comment 13

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

We want to use this in the same conditions as TabParent::UseAsyncPanZoom(), relevantly

          !cp->IsForApp() && IsForMozBrowser() &&

Because that works, I think this patch will work.  But I'm also barely keeping above a morass of confusion here.

Comment 14

[Justin Lebar (not reading bugmail)]

> We want to use this in the same conditions as TabParent::UseAsyncPanZoom(), relevantly
>
>          !cp->IsForApp() && IsForMozBrowser() &&

I agree with that.  But why does that imply that the following condition is correct?

> +    return (XRE_GetProcessType() != GeckoProcessType_Content ||
> +            ContentChild::GetSingleton()->IsForApp());

Aren't you missing the IsForMozBrowser()?

Comment 15

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

OK, I see.  Thanks.

Comment 16

[Joe Drew (not getting mail)]

Does our copy of Freetype even enable hinting?

Comment 17

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

Attachment: Enable font hinting for "app" processes, v2

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 651826 [details] [diff] [review]
Enable font hinting for "app" processes, v2

>diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
> /*static*/ TabParent*
> ContentParent::CreateBrowser(mozIApplication* aApp, bool aIsBrowserElement)
> {
>+    // We currently don't set the <app> ancestor for <browser> content
>+    // correctly.
>+    MOZ_ASSERT(!aApp || !aIsBrowserElement);

Ah, you're writing code with the assumption that this assertion does not always
hold, and relying on the fact that it happens to work even when this assertion
does always hold.

That's totally fine, but it's not what I had in mind when I suggested an
assertion.  I don't think we need this assertion how you've structured things,
since the assertion isn't protecting anything.

>diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
>--- a/dom/ipc/ContentParent.h
>+++ b/dom/ipc/ContentParent.h
>-    static ContentParent* GetNewOrUsed();
>+    static ContentParent* GetNewOrUsed(bool aForBrowserElement = false);

I feel compelled to point out my personal vendetta against this method signature:

http://jlebar.com/2011/12/16/Boolean_parameters_to_API_functions_considered_harmful..html

But anyway, you don't need to change it.  :)

Comment 19

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

(In reply to Justin Lebar [:jlebar] from comment #18)
> Comment on attachment 651826 [details] [diff] [review]
> Enable font hinting for "app" processes, v2
> 
> >diff --git a/dom/ipc/ContentParent.cpp b/dom/ipc/ContentParent.cpp
> > /*static*/ TabParent*
> > ContentParent::CreateBrowser(mozIApplication* aApp, bool aIsBrowserElement)
> > {
> >+    // We currently don't set the <app> ancestor for <browser> content
> >+    // correctly.
> >+    MOZ_ASSERT(!aApp || !aIsBrowserElement);
> 
> Ah, you're writing code with the assumption that this assertion does not
> always
> hold, and relying on the fact that it happens to work even when this
> assertion
> does always hold.
> 

Sort of ... if we started setting the app with isbrowser=true, then that would be something unexpected that I would want to know about.

> That's totally fine, but it's not what I had in mind when I suggested an
> assertion.  I don't think we need this assertion how you've structured
> things,
> since the assertion isn't protecting anything.
> 

It's just asserting that what we expect to be happening currently is happening.

> >diff --git a/dom/ipc/ContentParent.h b/dom/ipc/ContentParent.h
> >--- a/dom/ipc/ContentParent.h
> >+++ b/dom/ipc/ContentParent.h
> >-    static ContentParent* GetNewOrUsed();
> >+    static ContentParent* GetNewOrUsed(bool aForBrowserElement = false);
> 
> I feel compelled to point out my personal vendetta against this method
> signature:
> 
> http://jlebar.com/2011/12/16/
> Boolean_parameters_to_API_functions_considered_harmful..html
> 
> But anyway, you don't need to change it.  :)

That's fair, but we pass the boolean arg everywhere else.  I'm happy too to defer this to The Great App/Browser Refactoring.

Comment 20

[Justin Lebar (not reading bugmail)]

> Sort of ... if we started setting the app with isbrowser=true, then that would be 
> something unexpected that I would want to know about.

But would that change require any code changes?  If so, we should have the assertion.  If not, I don't think we should have an assertion just for "ping cjones if you remove this."

Comment 21

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

Yes, it would most likely break some assumptions in TabParent and ContentParent about !app.

Comment 22

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

Review ping.

Comment 23

[Jonathan Kew [:jfkthame]]

Comment on attachment 651826 [details] [diff] [review]
Enable font hinting for "app" processes, v2

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

::: dom/ipc/ContentChild.cpp
@@ +927,5 @@
>          NS_WARNING("Setting content child's ID twice?");
>      }
>      mID = id;
> +    mIsForApp = aIsForApp;
> +    mIsForBrowser = mIsForBrowser;

Typo!   s/= m/= a/

::: dom/ipc/PContent.ipdl
@@ +182,5 @@
> +     *
> +     * |id| is a unique ID among all subprocesses.  When |isForApp &&
> +     * isForBrowser|, we're loading <browser> for an app.  When
> +     * |isForBrowser|, we're loading <browser>.  When |!isForApp &&
> +     * !isForBrowser|, we're probably loading <xul:browser remote>.

Is this comment accurate? AFAICS, it's saying that for an app, both isForApp AND isForBrowser will be true; but the test in gfxAndroidPlatform looks like it's expecting isForApp to be true and isForBrowser to be false.

::: gfx/thebes/gfxAndroidPlatform.cpp
@@ +190,5 @@
>      // content that can always be be non-reflow-zoomed.  So turn off
>      // hinting.
>      // 
>      // XXX when gecko-android-java is used as an "app runtime", we'll
>      // want to re-enable hinting.

With the changes you're making, this comment needs updating, doesn't it?

@@ +197,5 @@
> +    // Otherwise, enable hinting unless we're in a content process
> +    // that might be used for non-reflowing zoom.
> +    return (XRE_GetProcessType() != GeckoProcessType_Content ||
> +            (ContentChild::GetSingleton()->IsForApp() &&
> +             !ContentChild::GetSingleton()->IsForBrowser()));

I'm confused - see question in PContent.ipdl. If the conditional here is correct, please make sure the comment about the flags is accurate. Or vice versa.

Comment 24

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

Thanks for the review! :)

(In reply to Jonathan Kew (:jfkthame) from comment #23)
> Comment on attachment 651826 [details] [diff] [review]
> Enable font hinting for "app" processes, v2
> 
> Review of attachment 651826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/ipc/ContentChild.cpp
> @@ +927,5 @@
> >          NS_WARNING("Setting content child's ID twice?");
> >      }
> >      mID = id;
> > +    mIsForApp = aIsForApp;
> > +    mIsForBrowser = mIsForBrowser;
> 
> Typo!   s/= m/= a/
> 

Yep, I fixed this locally.

> ::: dom/ipc/PContent.ipdl
> @@ +182,5 @@
> > +     *
> > +     * |id| is a unique ID among all subprocesses.  When |isForApp &&
> > +     * isForBrowser|, we're loading <browser> for an app.  When
> > +     * |isForBrowser|, we're loading <browser>.  When |!isForApp &&
> > +     * !isForBrowser|, we're probably loading <xul:browser remote>.
> 
> Is this comment accurate? AFAICS, it's saying that for an app, both isForApp
> AND isForBrowser will be true; but the test in gfxAndroidPlatform looks like
> it's expecting isForApp to be true and isForBrowser to be false.
> 

<browser> is the content that we want to non-reflow zoom, so we want to disable hinting in that case.  It's somewhat confusing, but <browser>s can only be loaded by <app>s.

Basically, the only case in which we want to enable hinting is for |isForApp && !isBrowser|.  This is the only type of content gecko we won't attempt to non-reflow zoom.

> ::: gfx/thebes/gfxAndroidPlatform.cpp
> @@ +190,5 @@
> >      // content that can always be be non-reflow-zoomed.  So turn off
> >      // hinting.
> >      // 
> >      // XXX when gecko-android-java is used as an "app runtime", we'll
> >      // want to re-enable hinting.
> 
> With the changes you're making, this comment needs updating, doesn't it?
> 

My patch doesn't affect this.

> @@ +197,5 @@
> > +    // Otherwise, enable hinting unless we're in a content process
> > +    // that might be used for non-reflowing zoom.
> > +    return (XRE_GetProcessType() != GeckoProcessType_Content ||
> > +            (ContentChild::GetSingleton()->IsForApp() &&
> > +             !ContentChild::GetSingleton()->IsForBrowser()));
> 
> I'm confused - see question in PContent.ipdl. If the conditional here is
> correct, please make sure the comment about the flags is accurate. Or vice
> versa.

Will post a new patch tomorrow.

Comment 25

[Justin Lebar (not reading bugmail)]

> It's somewhat confusing, but <browser>s can only be
> loaded by <app>s.

In B2G, but not in general!

Desktop Firefox contains a <browser> outside <app> (for some social API code).  And our test harnesses use browser outside app, afaik.

Comment 26

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

Attachment: Enable font hinting for "app" processes, v2.1

Rebased, included m = a fix, expanded comment about !browser || !app assertion in ContentParent::CreateBrowser().

carrying over r=jlebar

Comment 27

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/06f6eede6b77

Comment 28

[Ed Morley [:emorley]]

Backed out for mochitest-other crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b228bcfa794

Please use try! :-)

Comment 29

[Ed Morley [:emorley]]

Hmmm even though it crashed in multiple parts of mothitest-other with the same signature (https://tbpl.mozilla.org/php/getParsedLog.php?id=14626094&tree=Mozilla-Inbound), a retrigger came back green. Have relanded for now & will see how it goes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6229a3b222e4

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6229a3b222e4