dom-ui options: record user metrics for pref changes....

dom-ui options: record user metrics for pref changes.

BUG=52520
TEST=Verify metrics are saved when changing options in dom-ui options window (--enable-tabbed-options).

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

[csilv, 2010-08-18 23:05:34]

+zelidrag, +stuartmorgan for review

For this CL, I collected a list of all user metrics recorded by the platform
specific option windows and made sure they are now reflected in the dom-ui
version.  There are a few differences, but for the most part the set of options
is consistent.

Note that I only addressed browser-specific metrics, I did not attempt to add
any metric reporting for chromeos panels.  But the plumbing I added should make
it easy to add metric reporting to additional panels.

There are basically two methods for implementing a metric. If you have a control
that sets a preference automatically via the 'pref' attribute, you can also add
a 'metric' attribute.  Other cases that are not tied to a pref key can be done
either in the javascript or C++ handler.

[stuartmorgan, 2010-08-19 15:13:44]

http://codereview.chromium.org/3190004/diff/23001/24001
File chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/23001/24001#newcode65
chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc:65: if
(metric_string.length()) {
Minor nit, but maybe swap the order here so you only make the copy if there is a
metric?

http://codereview.chromium.org/3190004/diff/23001/24005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/23001/24005#newcode176
chrome/browser/dom_ui/core_options_handler.cc:176: pref_service);
Move this to a protected helper method, so the code doesn't have to be
duplicated in the subclass.

http://codereview.chromium.org/3190004/diff/23001/24017
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/3190004/diff/23001/24017#newcode47
chrome/browser/resources/options/preferences.js:47: var array = [name, value ?
'true' : 'false'];
s/array/arguments/ in all these functions.

[csilv, 2010-08-19 21:06:59]

http://codereview.chromium.org/3190004/diff/23001/24001
File chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/23001/24001#newcode65
chrome/browser/chromeos/dom_ui/core_chromeos_options_handler.cc:65: if
(metric_string.length()) {
On 2010/08/19 15:13:44, stuartmorgan wrote:
> Minor nit, but maybe swap the order here so you only make the copy if there is
a
> metric?

Done.

http://codereview.chromium.org/3190004/diff/23001/24005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/23001/24005#newcode176
chrome/browser/dom_ui/core_options_handler.cc:176: pref_service);
On 2010/08/19 15:13:44, stuartmorgan wrote:
> Move this to a protected helper method, so the code doesn't have to be
> duplicated in the subclass.

Done.

http://codereview.chromium.org/3190004/diff/23001/24017
File chrome/browser/resources/options/preferences.js (right):

http://codereview.chromium.org/3190004/diff/23001/24017#newcode47
chrome/browser/resources/options/preferences.js:47: var array = [name, value ?
'true' : 'false'];
On 2010/08/19 15:13:44, stuartmorgan wrote:
> s/array/arguments/ in all these functions.

Done.

[stuartmorgan, 2010-08-19 21:16:25]

LGTM

http://codereview.chromium.org/3190004/diff/31001/32005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/31001/32005#newcode176
chrome/browser/dom_ui/core_options_handler.cc:176: if (metric.length()) {
Probably cleaner as
if (metric.length() == 0)
  return;
...
but not a big deal either way.

[csilv, 2010-08-19 21:22:56]

http://codereview.chromium.org/3190004/diff/31001/32005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/31001/32005#newcode176
chrome/browser/dom_ui/core_options_handler.cc:176: if (metric.length()) {
On 2010/08/19 21:16:25, stuartmorgan wrote:
> Probably cleaner as
> if (metric.length() == 0)
>   return;
> ...
> but not a big deal either way.

Done.

[Evan Stade, 2010-08-19 21:34:36]

http://codereview.chromium.org/3190004/diff/31001/32005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/31001/32005#newcode176
chrome/browser/dom_ui/core_options_handler.cc:176: if (metric.length()) {
metric.empty()

[csilv, 2010-08-19 22:01:02]

http://codereview.chromium.org/3190004/diff/31001/32005
File chrome/browser/dom_ui/core_options_handler.cc (right):

http://codereview.chromium.org/3190004/diff/31001/32005#newcode176
chrome/browser/dom_ui/core_options_handler.cc:176: if (metric.length()) {
On 2010/08/19 21:34:36, Evan Stade wrote:
> metric.empty()

Done.

[Evan Stade, 2010-08-20 23:58:20]

(that was just a flyby nit, no need to wait for a review from me)

[csilv, 2010-08-21 01:25:57]

Thx.  I'm waiting for the trybot blessing of green.  Probably monday.