dom-ui settings: properly implement the 'metrics reporting' checkbox....

dom-ui settings: properly implement the 'metrics reporting' checkbox.
- the metrics reporting preference is unique, so handle it correctly.
- show a 'restart required' alert when changing the metrics reporting setting.
- enhance alert overlay to allow only an OK button.
- fix a bug in the alert overlay where it was incorrectly loading default strings.

BUG=56536
TEST=Verify that the metrics reporting checkbox works properly on Chrome branded builds.

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

[csilv, 2010-09-24 00:05:25]

+satorux for review

[satorux1, 2010-09-24 03:58:58]

LGTM. Thank you for implementing this so quickly!

The change seems to have fixed issues in alert_overlay.js along the way. Please
mention it in the change description.

http://codereview.chromium.org/3419020/diff/4001/5001
File chrome/browser/dom_ui/advanced_options_handler.cc (right):

http://codereview.chromium.org/3419020/diff/4001/5001#newcode270
chrome/browser/dom_ui/advanced_options_handler.cc:270:
SetupMetricsReportingCheckbox(enabled == is_enabled);
Giving it a name would make the code a bit easier to read?

bool user_changed = (enabled == is_enabled);

http://codereview.chromium.org/3419020/diff/4001/5004
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/3419020/diff/4001/5004#newcode25
chrome/browser/resources/options/advanced_options.js:25: * Initialize the page.
Initialize -> Initializes, per our JavaScript style guide. You might want to fix
other comments, but as long as it's consistent in the file, I don't have a
strong feeling.

http://codereview.chromium.org/3419020/diff/4001/5004#newcode42
chrome/browser/resources/options/advanced_options.js:42: if
($('metricsReportingEnabled')) {
Would be nice to add a comment like

// "metricsReportingEnabled" element is only present on Chrome branded builds.

http://codereview.chromium.org/3419020/diff/4001/5004#newcode45
chrome/browser/resources/options/advanced_options.js:45:
[String($('metricsReportingEnabled').checked)]);
I think you can use "event.target.checked" instead of
$('metricsReportingEnabled')

[csilv, 2010-09-24 18:05:31]

Updated the CL description to account for alert changes.

http://codereview.chromium.org/3419020/diff/4001/5001
File chrome/browser/dom_ui/advanced_options_handler.cc (right):

http://codereview.chromium.org/3419020/diff/4001/5001#newcode270
chrome/browser/dom_ui/advanced_options_handler.cc:270:
SetupMetricsReportingCheckbox(enabled == is_enabled);
On 2010/09/24 03:58:58, satorux1 wrote:
> Giving it a name would make the code a bit easier to read?
> 
> bool user_changed = (enabled == is_enabled);

Done.

http://codereview.chromium.org/3419020/diff/4001/5004
File chrome/browser/resources/options/advanced_options.js (right):

http://codereview.chromium.org/3419020/diff/4001/5004#newcode25
chrome/browser/resources/options/advanced_options.js:25: * Initialize the page.
On 2010/09/24 03:58:58, satorux1 wrote:
> Initialize -> Initializes, per our JavaScript style guide. You might want to
fix
> other comments, but as long as it's consistent in the file, I don't have a
> strong feeling. 

Done.

http://codereview.chromium.org/3419020/diff/4001/5004#newcode42
chrome/browser/resources/options/advanced_options.js:42: if
($('metricsReportingEnabled')) {
On 2010/09/24 03:58:58, satorux1 wrote:
> Would be nice to add a comment like
> 
> // "metricsReportingEnabled" element is only present on Chrome branded builds.
> 

Done.

http://codereview.chromium.org/3419020/diff/4001/5004#newcode45
chrome/browser/resources/options/advanced_options.js:45:
[String($('metricsReportingEnabled').checked)]);
On 2010/09/24 03:58:58, satorux1 wrote:
> I think you can use "event.target.checked" instead of
> $('metricsReportingEnabled')

Done.

[satorux1, 2010-09-27 02:08:31]

LGTM++

On 2010/09/24 18:05:31, csilv wrote:
> Updated the CL description to account for alert changes.
> 
> http://codereview.chromium.org/3419020/diff/4001/5001
> File chrome/browser/dom_ui/advanced_options_handler.cc (right):
> 
> http://codereview.chromium.org/3419020/diff/4001/5001#newcode270
> chrome/browser/dom_ui/advanced_options_handler.cc:270:
> SetupMetricsReportingCheckbox(enabled == is_enabled);
> On 2010/09/24 03:58:58, satorux1 wrote:
> > Giving it a name would make the code a bit easier to read?
> > 
> > bool user_changed = (enabled == is_enabled);
> 
> Done.
> 
> http://codereview.chromium.org/3419020/diff/4001/5004
> File chrome/browser/resources/options/advanced_options.js (right):
> 
> http://codereview.chromium.org/3419020/diff/4001/5004#newcode25
> chrome/browser/resources/options/advanced_options.js:25: * Initialize the
page.
> On 2010/09/24 03:58:58, satorux1 wrote:
> > Initialize -> Initializes, per our JavaScript style guide. You might want to
> fix
> > other comments, but as long as it's consistent in the file, I don't have a
> > strong feeling. 
> 
> Done.
> 
> http://codereview.chromium.org/3419020/diff/4001/5004#newcode42
> chrome/browser/resources/options/advanced_options.js:42: if
> ($('metricsReportingEnabled')) {
> On 2010/09/24 03:58:58, satorux1 wrote:
> > Would be nice to add a comment like
> > 
> > // "metricsReportingEnabled" element is only present on Chrome branded
builds.
> > 
> 
> Done.
> 
> http://codereview.chromium.org/3419020/diff/4001/5004#newcode45
> chrome/browser/resources/options/advanced_options.js:45:
> [String($('metricsReportingEnabled').checked)]);
> On 2010/09/24 03:58:58, satorux1 wrote:
> > I think you can use "event.target.checked" instead of
> > $('metricsReportingEnabled')
> 
> Done.