Closed Bug 784375 Opened 7 years ago Closed 7 years ago
Add a pref to control maximum font inflation ratio
As part of readability 2.0, we're investigating a number of possible solutions to the readability issue. As part of this, we should investigate limiting the maximum font inflation ratio. Currently, the font inflation algorithm maps specified font size, s, to inflated font size, i, and then returns the ratio i/s, which is then multiplied by the specified font size in order to make the adjustment. We should add a preference that allows the specification of a maximum ratio, R, such that a computed ratio i/s > R always returns R as a ratio. This allows us to preserve the ideal font size within the font inflation algorithm, but still limit our aggressiveness in situations where we are blending font inflation with another solution to the readability problem.
Try-server run: https://hg.mozilla.org/try/rev/fb01d9e62a67
Comment on attachment 659863 [details] [diff] [review] b784375 Given the preference API, I think it makes more sense to use an integer preference that's a percentage (i.e., 200 means 2.0), and then handle things the normal way. And how hard would it be to cause a reflow when the preference changes? (I forget exactly what state the code is in regarding handling changes to the other preferences.) Otherwise this looks fine.
Attachment #659863 - Flags: review?(dbaron) → review-
Here's a new patch that makes the changes you requested, with the exception of the reflow change. This will be difficult until the followup patch for b749186 (where we generate a reframe event) lands. The bug for this work is bug 766599, which I'll take a look at today again, to see if I can come up with a workable solution.
(In reply to David Baron [:dbaron] from comment #3) > And how hard would it be to cause a reflow when the preference changes? (I > forget exactly what state the code is in regarding handling changes to the > other preferences.) Yeah, so this seems difficult. I tried just adding another preference to listen for changes to in nsPresContext::PreferenceChanged, setting mPrefChangePendingNeedsReflow after it was seen, but this causes a crash when one of the font inflation prefs change. Interestingly, it doesn't always crash in the same place. Often, it crashes at the line (my line numbers) 879 in nsPresContext::UpdateAfterPreferencesChanged(), which is: > mDeviceContext->FlushFontCache(); due to mDeviceContext being null. But, sometimes it will crash inside of RebuildAllStyleData, at the line: > mShell->FrameConstructor()->RebuildAllStyleData(aExtraHint); When it tries to delete the style tree. Perhaps there's something easy I can do about these crashes that I'm not seeing, but otherwise, I think we'll need to wait for bug 766599 until we can get dynamic pref updating back into font inflation. :( Thoughts?
David: Quick review ping, if you have time. :)
Comment on attachment 660570 [details] [diff] [review] b784375 (v2) >+ float iOverS = 0.0; Maybe call this |inflationRatio| instead of |iOverS|? (Otherwise it seems a little confusing that maxRatio is a constraint on this, rather than the other variable called ratio.) Also, don't bother initializing to 0.0. You initialize it in both branches below, and you'd want compiler warnings or valgrind to catch it if you don't. r=dbaron
Attachment #660570 - Flags: review?(dbaron) → review+
Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/94f06c75c3b5 then backed out because I forgot a reviewer: https://hg.mozilla.org/integration/mozilla-inbound/rev/811a86ca567e And inbounded again: https://hg.mozilla.org/integration/mozilla-inbound/rev/e52a16b96738
Target Milestone: --- → mozilla19
Backed out again, due to oranges: https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b7708e04f4
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=65d2573fa8da
Inbound, with minor fix (removed code that shouldn't be there and was included in testing): https://hg.mozilla.org/integration/mozilla-inbound/rev/8a59732f220e
Target Milestone: --- → mozilla19
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.