Closed
Bug 803908
Opened 13 years ago
Closed 13 years ago
Enable font inflation for "browser tab" content
Categories
(Core :: General, defect)
Core
General
Tracking
()
People
(Reporter: cjones, Assigned: dbaron)
References
Details
Attachments
(1 file, 1 obsolete file)
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.
Reporter | ||
Updated•13 years ago
|
Comment 1•13 years ago
|
||
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: ? → -
Reporter | ||
Comment 2•13 years ago
|
||
I think this should be a very small and not-risky patch. I very much want to see this in.
Comment 3•13 years ago
|
||
We aren't blocking here, but I would happily approve a low risk patch.
Reporter | ||
Comment 4•13 years ago
|
||
Well that took about 10 minutes.
Thanks dbaron! :)
Assignee: nobody → jones.chris.g
Attachment #675801 -
Flags: review?(dbaron)
Comment 5•13 years ago
|
||
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•13 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•13 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-
Reporter | ||
Comment 8•13 years ago
|
||
You're absolutely right. I haven't responded because I haven't time to pick this back up.
Assignee | ||
Comment 9•13 years ago
|
||
OK, either I'll take this or Scott will.
Assignee: jones.chris.g → dbaron
Reporter | ||
Comment 10•13 years ago
|
||
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 :).
Reporter | ||
Comment 11•13 years ago
|
||
Attachment #675801 -
Attachment is obsolete: true
Attachment #681434 -
Flags: review?(dbaron)
Assignee | ||
Comment 12•13 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.
Reporter | ||
Comment 13•13 years ago
|
||
(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•13 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 15•13 years ago
|
||
Test build on Android (linking it here so I can get to it):
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/dbaron@mozilla.com-5abf257e88a2/try-android/fennec-19.0a1.en-US.android-arm.apk
Assignee | ||
Comment 16•13 years ago
|
||
Works fine on Fennec; I'll land on inbound later today (with the reftest.list adjustments).
Assignee | ||
Comment 17•13 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+
Assignee | ||
Comment 18•13 years ago
|
||
Reporter | ||
Comment 19•13 years ago
|
||
Thanks!
Reporter | ||
Comment 20•13 years ago
|
||
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•13 years ago
|
Attachment #681434 -
Flags: approval-mozilla-aurora?
Comment 21•13 years ago
|
||
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
Comment 22•13 years ago
|
||
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+
Assignee | ||
Comment 23•13 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/ea1cdde162bf
https://hg.mozilla.org/releases/mozilla-beta/rev/7f76d7828b3d
status-firefox18:
--- → fixed
status-firefox19:
--- → fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•