Enable font inflation for "browser tab" content

RESOLVED FIXED in Firefox 18

Status

()

Core
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: cjones, Assigned: dbaron)

Tracking

(Depends on: 1 bug)

unspecified
mozilla20
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-basecamp:-, firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Blocks: 793462
No longer blocks: 799585
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.
blocking-basecamp: ? → -
I think this should be a very small and not-risky patch.  I very much want to see this in.

Comment 3

6 years ago
We aren't blocking here, but I would happily approve a low risk patch.
Created attachment 675801 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers

Well that took about 10 minutes.

Thanks dbaron! :)
Assignee: nobody → jones.chris.g
Attachment #675801 - Flags: review?(dbaron)
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?
(Assignee)

Comment 6

6 years ago
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.
(Assignee)

Comment 7

6 years ago
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.
Attachment #675801 - Flags: review?(dbaron) → review-
You're absolutely right.  I haven't responded because I haven't time to pick this back up.
(Assignee)

Comment 9

6 years ago
OK, either I'll take this or Scott will.
Assignee: jones.chris.g → dbaron
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 :).
Created attachment 681434 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers, v2
Attachment #675801 - Attachment is obsolete: true
Attachment #681434 - Flags: review?(dbaron)
(Assignee)

Comment 12

6 years ago
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.
(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.
(Assignee)

Comment 14

6 years ago
(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.
(Assignee)

Comment 16

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

Comment 17

6 years ago
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.
Attachment #681434 - Flags: review?(dbaron) → review+
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.
Attachment #681434 - Flags: approval-mozilla-beta?
(Assignee)

Updated

6 years ago
Attachment #681434 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c0b7c15bca4d
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment on attachment 681434 [details] [diff] [review]
Enable font inflation for async pan/zoomed browsers, v2

b2g-only, low risk, approving.
Attachment #681434 - Flags: approval-mozilla-beta?
Attachment #681434 - Flags: approval-mozilla-beta+
Attachment #681434 - Flags: approval-mozilla-aurora?
Attachment #681434 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 814450
Depends on: 814778
(Assignee)

Updated

6 years ago
Depends on: 815977
You need to log in before you can comment on or make changes to this bug.