Enable font hinting in "app" content processes

RESOLVED FIXED in mozilla17

Status

()

Core
Graphics
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

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

Firefox Tracking Flags

(blocking-kilimanjaro:+, blocking-basecamp:-)

Details

(Whiteboard: [LOE:S])

Attachments

(1 attachment, 2 obsolete attachments)

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.)
I'm not sure which flags to use here but we need this for b2g v1.
Blocks: 745143
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
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?
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

Updated

5 years ago
blocking-basecamp: ? → +
blocking-kilimanjaro: ? → +
Maybe Jonathan can take this?
Assignee: nobody → jfkthame
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

5 years ago
How are we doing here?
Priority: -- → P2

Comment 7

5 years ago
Renom if you think we can't ship a v1 without this. That having said, this sounds trivial. Would love to have it.
blocking-basecamp: + → ---
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
blocking-basecamp: --- → ?
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.
Created attachment 650375 [details] [diff] [review]
Enable font hinting for "app" processes

gfxAndroidPlatform change --> jfkthame, isForApp plumbing --> jlebar.
Assignee: jfkthame → jones.chris.g
Attachment #650375 - Flags: review?(justin.lebar+bug)
Attachment #650375 - Flags: review?(jfkthame)
This will also enable hinting for browser tabs.  Is that what you want?  (IOW, we're hinting everywhere in b2g.)
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.
Attachment #650375 - Flags: review?(justin.lebar+bug) → review-
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.
> 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()?
OK, I see.  Thanks.
Does our copy of Freetype even enable hinting?
Created attachment 651826 [details] [diff] [review]
Enable font hinting for "app" processes, v2
Attachment #650375 - Attachment is obsolete: true
Attachment #650375 - Flags: review?(jfkthame)
Attachment #651826 - Flags: review?(justin.lebar+bug)
Attachment #651826 - Flags: review?(jfkthame)
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.  :)
Attachment #651826 - Flags: review?(justin.lebar+bug) → review+
(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.
> 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."
Yes, it would most likely break some assumptions in TabParent and ContentParent about !app.
blocking-basecamp: ? → -
Review ping.
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.
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.
Whiteboard: [LOE:S]
> 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.
Created attachment 654519 [details] [diff] [review]
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
Attachment #651826 - Attachment is obsolete: true
Attachment #651826 - Flags: review?(jfkthame)
Attachment #654519 - Flags: review?(jfkthame)
Attachment #654519 - Flags: review?(jfkthame) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/06f6eede6b77
Backed out for mochitest-other crashes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b228bcfa794

Please use try! :-)
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
https://hg.mozilla.org/mozilla-central/rev/6229a3b222e4
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
You need to log in before you can comment on or make changes to this bug.