803908 - Enable font inflation for "browser tab" content

Status: RESOLVED FIXED

Description

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

Because of the autozoom behavior introduced by switching to native viewport support (hatehatehate), text on most pages is now unreadable at their default zoom.  dbaron implemented font inflation to help with this problem.

We want to enable it *only* for "browser tab" content.  More specifically, if and only if

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/TabChild.h#293

returns true for the page being rendered.

I think this is a borderline blocker because of the massive impact on browser usability.

Comment 1

[Andrew Overholt [:overholt]]

We discussed this during triage and it seems like scope creep.  Of course, poor readability of desktop sites is not good and maybe it's simple to fix this?  For now we decided to not block on this but obviously feel free to re-nom if you disagree with not blocking on this.

Comment 2

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

I think this should be a very small and not-risky patch.  I very much want to see this in.

Comment 3

[Andreas Gal :gal]

We aren't blocking here, but I would happily approve a low risk patch.

Comment 4

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

Created attachment 675801 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers

Well that took about 10 minutes.

Thanks dbaron! :)

Comment 5

[Scott Johnson (:jwir3)]

We might want to consider adding a test for this functionality... but, I don't know if this is reasonable for Firefox OS...

Is this something we could test in our reftest suite in layout/reftests/font-inflation?

Comment 6

[David Baron :dbaron: ⌚UTC-7]

Will the thing you're doing to ignore the font inflation pref work for Fennec as well as b2g?  It seems reasonable, roughly, except that it would make it a good bit harder to test font inflation in a desktop build (which is currently possible), and would, I suspect, break all the font inflation reftests (unless I'm understanding this wrong).

Maybe we want a boolean pref where the two boolean values say (without regard for which is true and which is false, which depends on the name):
 (a) font inflation minTwips/emPerLine preferences are honored unconditionally
 (b) font inflation minTwips/emPerLine preferences are honored only for async pan/zoomed browsers
Having (b) the default is probably even reasonable (assuming that it works in Fennec in addition to B2G), but the tests in layout/reftests/font-inflation would need to choose (a), as would anybody testing font inflation in a desktop build.

Comment 7

[David Baron :dbaron: ⌚UTC-7]

Comment on attachment 675801 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers

Minusing to prompt at least a response to comment 6; feel free to re-request if you think I'm wrong, though.

Comment 8

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

You're absolutely right.  I haven't responded because I haven't time to pick this back up.

Comment 9

[David Baron :dbaron: ⌚UTC-7]

OK, either I'll take this or Scott will.

Comment 10

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

Aieee, I was offline all day today so was able to update the patch.  Will post in a bit.

If you're not happy with the updated patch, please feel free to steal :).

Comment 11

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

Created attachment 681434 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers, v2

Comment 12

[David Baron :dbaron: ⌚UTC-7]

This all looks good.  However:
 * still need to adjust the reftest manifest for the font inflation reftests
 * might still need to do something for Fennec Android (need to test)
 * I'm a bit worried about the performance cost of GetTabChildFrom where it is.

I'll try to pick this up, but possibly not until this week's Layout/Graphics/Meeting meeting is over.

Comment 13

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

(In reply to David Baron [:dbaron] from comment #12)
> This all looks good.  However:
>  * still need to adjust the reftest manifest for the font inflation reftests

Right, I was planning to do this when the harness can run tests in the case where IsAsyncPanZoomEnabled() can return true.  Should be soon or now.

>  * might still need to do something for Fennec Android (need to test)

I don't believe so.

>  * I'm a bit worried about the performance cost of GetTabChildFrom where it
> is.

As you probably saw, GetTabChildFrom() is a few QIs and a GI.  I don't know how hot FontInflationEnabled() is, so I'm not sure if that counts as expensive.

Comment 14

[David Baron :dbaron: ⌚UTC-7]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #13)
> (In reply to David Baron [:dbaron] from comment #12)
> > This all looks good.  However:
> >  * still need to adjust the reftest manifest for the font inflation reftests
> 
> Right, I was planning to do this when the harness can run tests in the case
> where IsAsyncPanZoomEnabled() can return true.  Should be soon or now.

It's still needed for running the reftests on desktop, which we do, since otherwise this patch would break those tests.

In any case, I've done that locally.

> >  * might still need to do something for Fennec Android (need to test)
> 
> I don't believe so.

OK, I'll double-check this.

> >  * I'm a bit worried about the performance cost of GetTabChildFrom where it
> > is.
> 
> As you probably saw, GetTabChildFrom() is a few QIs and a GI.  I don't know
> how hot FontInflationEnabled() is, so I'm not sure if that counts as
> expensive.

It's pretty hot (I could imagine it being an 0.1% regression; 1% is probably a stretch), and part of the reason I'm concerned is that given the structure of the code, it would only show up as a regression on the mobile platforms where we don't have performance regression testing infrastructure.  But I don't really see an alternative.  I might try to do a profile locally with the pref setup that we'd have on Fennec.

Comment 15

[David Baron :dbaron: ⌚UTC-7]

Test build on Android (linking it here so I can get to it):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5abf257e88a2/try-android/fennec-19.0a1.en-US.android-arm.apk

Comment 16

[David Baron :dbaron: ⌚UTC-7]

Works fine on Fennec; I'll land on inbound later today (with the reftest.list adjustments).

Comment 17

[David Baron :dbaron: ⌚UTC-7]

Comment on attachment 681434 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers, v2

r=dbaron with the pref adjustments to layout/reftest/font-inflation/reftest.list

I'll land this shortly on inbound.

Comment 18

[David Baron :dbaron: ⌚UTC-7]

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

Comment 19

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

Thanks!

Comment 20

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

Comment on attachment 681434 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers, v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
  Support for <meta viewport>
User impact if declined:
  Significantly degraded browsing experience in b2g browser.  Additionally, a different experience than on fennec-android, which uses font inflation.
Testing completed (on m-c, etc.):
  This is basically just enabling code that already runs in fennec-android.
Risk to taking this patch (and alternatives if risky):
  Basically no risk to non-b2g products.  For b2g, relatively low risk to stability/correctness.  Unknown risk to performance.  The alternative to this patch is disabling <meta viewport> handling, which would be disastrous for compatibility.
String or UUID changes made by this patch:
  None.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 681434 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers, v2

b2g-only, low risk, approving.

Comment 23

[David Baron :dbaron: ⌚UTC-7]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ea1cdde162bf
https://hg.mozilla.org/releases/mozilla-beta/rev/7f76d7828b3d