Closed Bug 784375 Opened 7 years ago Closed 7 years ago

Add a pref to control maximum font inflation ratio

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: jwir3, Assigned: jwir3)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch b784375 (obsolete) — Splinter Review
Attachment #659863 - Flags: review?(dbaron)
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-
Attached patch b784375 (v2)Splinter 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.
Attachment #659863 - Attachment is obsolete: true
Attachment #660570 - Flags: review?(dbaron)
(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+
Target Milestone: mozilla19 → ---
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
https://hg.mozilla.org/mozilla-central/rev/8a59732f220e
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.