dom-ui settings: Add setting for setting the minimum font size....

dom-ui settings: Add setting for setting the minimum font size.

BUG=7417
TEST=Exercise the minimum font size setting in dom-ui settings window.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=62450

[csilv, 2010-10-08 23:47:00]

This is the highly requested minimum font size feature.  Right now for DOM-UI
only, but can be extended to platforms in separate CL's.

jhawkins - full review
dhg - impact on CrOS, core_options_handler, preferences.js
arv - fyi

[James Hawkins, 2010-10-09 00:02:44]

http://codereview.chromium.org/3584019/diff/5001/6002
File chrome/browser/dom_ui/options/core_options_handler.cc (right):

http://codereview.chromium.org/3584019/diff/5001/6002#newcode183
chrome/browser/dom_ui/options/core_options_handler.cc:183: DCHECK(dom_ui_);
Can we move all of these DCHECKs to Initialize() or Attach()?

http://codereview.chromium.org/3584019/diff/5001/6002#newcode184
chrome/browser/dom_ui/options/core_options_handler.cc:184: PrefService*
pref_service = dom_ui_->GetProfile()->GetPrefs();
Do we need to use GetOriginalProfile() here?

http://codereview.chromium.org/3584019/diff/5001/6002#newcode316
chrome/browser/dom_ui/options/core_options_handler.cc:316: if (args->GetSize() <
1)
Should this be a DCHECK instead? What circumstances would cause GetSize() to
return < 1 or > 2? Documentation on |args| would probably clarify this.

http://codereview.chromium.org/3584019/diff/5001/6003
File chrome/browser/dom_ui/options/core_options_handler.h (right):

http://codereview.chromium.org/3584019/diff/5001/6003#newcode89
chrome/browser/dom_ui/options/core_options_handler.h:89: // value.
Document |args| here.

http://codereview.chromium.org/3584019/diff/5001/6009
File chrome/browser/resources/options/font_settings_ui.js (right):

http://codereview.chromium.org/3584019/diff/5001/6009#newcode10
chrome/browser/resources/options/font_settings_ui.js:10: // Define a constructor
that uses an select element as its underlying element.
s/an/a/

[arv (Not doing code reviews), 2010-10-09 00:13:54]

LGTM

http://codereview.chromium.org/3584019/diff/14001/15009
File chrome/browser/resources/options/font_settings_ui.js (right):

http://codereview.chromium.org/3584019/diff/14001/15009#newcode71
chrome/browser/resources/options/font_settings_ui.js:71:
options.forEach(function (values) {
function(values) {

[csilv, 2010-10-09 01:12:50]

http://codereview.chromium.org/3584019/diff/5001/6002
File chrome/browser/dom_ui/options/core_options_handler.cc (right):

http://codereview.chromium.org/3584019/diff/5001/6002#newcode183
chrome/browser/dom_ui/options/core_options_handler.cc:183: DCHECK(dom_ui_);
On 2010/10/09 00:02:44, James Hawkins wrote:
> Can we move all of these DCHECKs to Initialize() or Attach()?

Yes.  There is one already in Attach, which makes these others redundant and
non-useful.  Removed here and elsewhere.

http://codereview.chromium.org/3584019/diff/5001/6002#newcode184
chrome/browser/dom_ui/options/core_options_handler.cc:184: PrefService*
pref_service = dom_ui_->GetProfile()->GetPrefs();
On 2010/10/09 00:02:44, James Hawkins wrote:
> Do we need to use GetOriginalProfile() here?

Not needed here.  GetPrefs() will return the original prefs automatically.

http://codereview.chromium.org/3584019/diff/5001/6002#newcode316
chrome/browser/dom_ui/options/core_options_handler.cc:316: if (args->GetSize() <
1)
On 2010/10/09 00:02:44, James Hawkins wrote:
> Should this be a DCHECK instead? What circumstances would cause GetSize() to
> return < 1 or > 2? Documentation on |args| would probably clarify this.

The '> 2' check should have been '> 1', fixed.  The '1' element is optional so
that check is valid.

The < 1 should never occur except in the case of developer error, so a DHECK
would be appropriate.  Changed here and elsewhere.

http://codereview.chromium.org/3584019/diff/5001/6003
File chrome/browser/dom_ui/options/core_options_handler.h (right):

http://codereview.chromium.org/3584019/diff/5001/6003#newcode89
chrome/browser/dom_ui/options/core_options_handler.h:89: // value.
On 2010/10/09 00:02:44, James Hawkins wrote:
> Document |args| here.

Documented |args| here and elsewhere.

http://codereview.chromium.org/3584019/diff/5001/6009
File chrome/browser/resources/options/font_settings_ui.js (right):

http://codereview.chromium.org/3584019/diff/5001/6009#newcode10
chrome/browser/resources/options/font_settings_ui.js:10: // Define a constructor
that uses an select element as its underlying element.
On 2010/10/09 00:02:44, James Hawkins wrote:
> s/an/a/

Done.

http://codereview.chromium.org/3584019/diff/14001/15009#newcode71
chrome/browser/resources/options/font_settings_ui.js:71:
options.forEach(function (values) {
On 2010/10/09 00:13:54, arv wrote:
> function(values) {

Done.

[James Hawkins, 2010-10-09 01:18:16]

LGTM

http://codereview.chromium.org/3584019/diff/25001/26002
File chrome/browser/dom_ui/options/core_options_handler.cc (right):

http://codereview.chromium.org/3584019/diff/25001/26002#newcode209
chrome/browser/dom_ui/options/core_options_handler.cc:209:
DCHECK(args->GetSize() >= 2);
DCHECK_GE, here and elsewhere.

http://codereview.chromium.org/3584019/diff/25001/26002#newcode290
chrome/browser/dom_ui/options/core_options_handler.cc:290:
DCHECK(args->GetSize() > 1);
DCHECK_GT, here and elsewhere.

[csilv, 2010-10-12 22:38:55]

http://codereview.chromium.org/3584019/diff/25001/26002
File chrome/browser/dom_ui/options/core_options_handler.cc (right):

http://codereview.chromium.org/3584019/diff/25001/26002#newcode209
chrome/browser/dom_ui/options/core_options_handler.cc:209:
DCHECK(args->GetSize() >= 2);
On 2010/10/09 01:18:16, James Hawkins wrote:
> DCHECK_GE, here and elsewhere.

Done.

http://codereview.chromium.org/3584019/diff/25001/26002#newcode290
chrome/browser/dom_ui/options/core_options_handler.cc:290:
DCHECK(args->GetSize() > 1);
On 2010/10/09 01:18:16, James Hawkins wrote:
> DCHECK_GT, here and elsewhere.

Done.