784375 - Add a pref to control maximum font inflation ratio

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Scott Johnson (:jwir3)]

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.

Comment 1

[Scott Johnson (:jwir3)]

Attachment: b784375

Comment 2

[Scott Johnson (:jwir3)]

Try-server run:
https://hg.mozilla.org/try/rev/fb01d9e62a67

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 4

[Scott Johnson (:jwir3)]

Attachment: b784375 (v2)

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.

Comment 5

[Scott Johnson (:jwir3)]

(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?

Comment 6

[Scott Johnson (:jwir3)]

David:

Quick review ping, if you have time. :)

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 8

[Scott Johnson (:jwir3)]

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

Comment 9

[Scott Johnson (:jwir3)]

Backed out again, due to oranges:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4b7708e04f4

Comment 10

[Scott Johnson (:jwir3)]

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=65d2573fa8da

Comment 11

[Scott Johnson (:jwir3)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8a59732f220e