Closed Bug 803908 Opened 8 years ago Closed 8 years ago

Enable font inflation for "browser tab" content

Categories

(Core :: General, defect)

Product:
Core
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla20
Project Flags:
blocking-basecamp -
Tracking Flags:
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: cjones, Assigned: dbaron)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Enable font inflation for async pan/zoomed browsers, v2
8.99 KB, patch
dbaron
: review+
lsblakk
: approval-mozilla-aurora+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review 
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.
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.
We aren't blocking here, but I would happily approve a low risk patch.
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?
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 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.
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 :).
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.
(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.
Works fine on Fennec; I'll land on inbound later today (with the reftest.list adjustments).
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?
Attachment #681434 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/c0b7c15bca4d
Status: NEW → RESOLVED
Closed: 8 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
You need to log in before you can comment on or make changes to this bug.