Per bug 621823 comment #30, toolkit's Zoom Manager throws an exception when trying to set a zoom level which is outside the range specified by the zoom.{min,max}Percent preferences. There are a couple of cases where the implementation assumes implicitly that 100% is always within that range, specifically for the default case when no global value from the content pref is given, and when the Ctrl+0 keyboard shortcut is used (toolkit bug 539417). While 100% will be excluded as menu item when it's outside the allowed range after bug 621823 now, the remaining cases yet have to be fixed. Setting zoom.minPercent > 100% has been an accessibility use case to define a default global zoom (equally affecting text and figures) as a replacement for larger font settings or DPI changes (which won't scale figures and some formats), but now does no longer work.

This patch adds this._ensureValid(1) where previously 1 was simply passed as argument without any checks. The first hunk takes care of the Ctrl+0 case and solves bug 539417 for SeaMonkey; the second hunk covers the handling of default cases. In both instances, the actual value used is adjusted either to the lower or the higher end to stay within the min-max range, thus avoiding that setZoomForBrowser() throws an exception. Note that globalValue is checked against min/max already in its get() function.

Is is me or is it possible for _ensureValid to return an invalid value?

I didn't plan to touch that function, but apparently it will always return /some/ integer (e.g., "1" if the input is NaN). The only conflict I can see is the case where zoom.minPercent > zoom.maxPercent, where it will return ZoomManager.MIN as it's the earlier test. Such a case is probably rather a configuration error, not really much can be done if the preferences are selected in a conflicting way.

(In reply to comment #3) > I didn't plan to touch that function, but apparently it will always return > /some/ integer (e.g., "1" if the input is NaN). This bug is about using "1" without checking that it's valid. So I'm concerned that this is another case of that bug...

I can make it "undefined" instead for that case, which wouldn't be valid return value in a strict sense either. The other option may be to treat it as aValue = 1 instead and let it go through the min/max checks as if it was the input value.

Same as attachment 546455 [details] [diff] [review] but also subjects a NaN value to the min/max test.

Comment on attachment 546526 [details] [diff] [review] Proposed fix (v2) >+ var testValue = aValue; > >+ if (isNaN(testValue)) >+ testValue = 1; Makes sense, but no need to make a copy of aValue, just set aValue = 1;

I've learned that messing with function arguments (even if passed by value) is a bad style, but as you wish. ;-) Thanks for the quick review!

Could someone give the patch a friendly push on comm-central (along with any other patches in the checkin-needed queue), please? I was planning to nominate this for comm-aurora/beta, assuming that fixing even obscured exceptions qualifies and that it's simple enough, but that requires it to land on trunk first...

Comment on attachment 546542 [details] [diff] [review] Patch for checkin [Checked in: trunk Comment 10 & comm-aurora/beta Comment 11] http://hg.mozilla.org/comm-central/rev/bfd229071b88

Comment on attachment 546542 [details] [diff] [review] Patch for checkin [Checked in: trunk Comment 10 & comm-aurora/beta Comment 11] http://hg.mozilla.org/releases/comm-aurora/rev/5129a00ec84c http://hg.mozilla.org/releases/comm-beta/rev/4c97611e9940

Thanks Ian!

Callek, this landed on Beta in time for 2.3, so why did you change TM to 2.5? I find that confusing.

(In reply to comment #13) > Callek, this landed on Beta in time for 2.3, so why did you change TM to > 2.5? I find that confusing. Target milestone reflects what trunk was when it landed "there", branch-approvals get status-X:fixed.