MD Settings: implement metrics reporting checkbox on desktop

MD Settings: implement metrics reporting checkbox on desktop

BUG=614265, 632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a620af406275d8e5bf37832661f773ec60f3802a
Cr-Commit-Position: refs/heads/master@{#411276}

[Dan Beam, 2016-08-09 18:31:18]

Description was changed from

==========
MD Settings: show controlled indicator next to UMA checkbox on desktop

BUG=614265,632443
==========

to

==========
MD Settings: show controlled indicator next to UMA checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-08-09 22:28:17]

Description was changed from

==========
MD Settings: show controlled indicator next to UMA checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-08-09 23:00:02]

Patchset #1 (id:1) has been deleted

[Dan Beam, 2016-08-09 23:08:33]

Patchset #1 (id:20001) has been deleted

[Dan Beam, 2016-08-10 01:19:25]

Patchset #1 (id:40001) has been deleted

[Dan Beam, 2016-08-10 04:39:16]

Patchset #2 (id:80001) has been deleted

[Dan Beam, 2016-08-10 04:39:23]

Patchset #1 (id:60001) has been deleted

[Dan Beam, 2016-08-10 04:39:55]

dbeam@chromium.org changed reviewers:
+ dschuyler@chromium.org

[Dan Beam, 2016-08-10 04:39:56]

soooo, this is pretty big.

we can potentially split if you'd like

[Dan Beam, 2016-08-10 04:40:06]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 04:40:38]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 04:43:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 04:43:32]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_x86-ge...)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Dan Beam, 2016-08-10 04:54:56]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 04:55:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2016-08-10 04:55:38]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 04:55:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 05:00:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 05:00:53]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dschuyler, 2016-08-10 18:55:16]

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.js:56:
this.addWebUIListener('metrics-reporting-change', boundSetMetricsReporting);
Not even a nit, but just a preference: I know it
doesn't matter in this case, it's just preference, 
but I it reads better to me if the listener is set up
before the query is initiated. i.e. having line 56
somewhere above line 54.

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc
(right):

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc:70:
policy::MockConfigurationPolicyProvider* provider() { return &provider_; }
For test_web_ui(), map(), and provider() can we 
return references?

[Dan Beam, 2016-08-10 19:41:59]

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/resourc...
File chrome/browser/resources/settings/privacy_page/privacy_page.js (right):

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/resourc...
chrome/browser/resources/settings/privacy_page/privacy_page.js:56:
this.addWebUIListener('metrics-reporting-change', boundSetMetricsReporting);
On 2016/08/10 18:55:16, dschuyler wrote:
> Not even a nit, but just a preference: I know it
> doesn't matter in this case, it's just preference, 
> but I it reads better to me if the listener is set up
> before the query is initiated. i.e. having line 56
> somewhere above line 54.

Done.

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/ui/webu...
File chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc
(right):

https://codereview.chromium.org/2233443002/diff/120001/chrome/browser/ui/webu...
chrome/browser/ui/webui/settings/metrics_reporting_handler_unittest.cc:70:
policy::MockConfigurationPolicyProvider* provider() { return &provider_; }
On 2016/08/10 18:55:16, dschuyler wrote:
> For test_web_ui(), map(), and provider() can we 
> return references?

we could... but what's the benefit?

the google c++ style guide discourages accepting *params* as non-const
references.

returning mutable pointers to internal state is way more common in chrome:

$ git grep -P '(?<!const)\s[:\w]+\&\s[\w:]+\(' | grep -v third_party | wc -l
763

$ git grep -P '(?<!const)\s[:\w]+\*\s[\w:]+\(' | grep -v third_party | wc -l
19676

[Dan Beam, 2016-08-10 19:42:05]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 19:43:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 19:50:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 19:50:11]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[dschuyler, 2016-08-10 21:23:44]

LGTM

(Just chatting, feel free to ignore):

The benefit is that references cannot be 
null. I acknowledge the guide about
params, but this is different, I don't 
think the guides provide guidelines on
returning references vs pointers. I expect 
the difference in return types be 
due to the age of the code base (rather 
than an indication of which would 
be preferable).

[Dan Beam, 2016-08-10 21:57:17]

On 2016/08/10 21:23:44, dschuyler wrote:
> LGTM
> 
> (Just chatting, feel free to ignore):
> 
> The benefit is that references cannot be 
> null.

Right, but the downside is that some of these things must be unique_ptr's to
defer initialization, so *deferencing* null could also be bad (i.e. *nullptr).

> I acknowledge the guide about
> params, but this is different, I don't 
> think the guides provide guidelines on
> returning references vs pointers. I expect 
> the difference in return types be 
> due to the age of the code base (rather 
> than an indication of which would 
> be preferable).

ehhhhhhh, maybe, but lots of new code gets written in the style of of the old
code every day, so this adds confusion (if your theory is right) :(

[Dan Beam, 2016-08-10 22:07:51]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 22:09:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Dan Beam, 2016-08-10 22:10:05]

The CQ bit was unchecked by dbeam@chromium.org

[Dan Beam, 2016-08-10 22:25:31]

+asvitkine@ for chrome/browser/metrics/chrome_metrics_service_accessor.h

[Dan Beam, 2016-08-10 22:25:46]

dbeam@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Dan Beam, 2016-08-10 22:25:47]

+asvitkine@ for realz

[Dan Beam, 2016-08-11 03:04:02]

The CQ bit was checked by dbeam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 03:04:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 04:11:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 04:11:37]

Dry run: This issue passed the CQ dry run.

[Dan Beam, 2016-08-11 05:21:47]

Description was changed from

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=asvitkine@chromium.org
==========

[Alexei Svitkine (slow), 2016-08-11 06:00:07]

asvitkine@chromium.org changed reviewers:
+ gayane@chromium.org

[Alexei Svitkine (slow), 2016-08-11 06:00:08]

+gayane who last touched the logic for this checkbox in the old settings UI.

From my side, LGTM for the metrics service accessor change, but please have
Gayane review the new logic.

[Dan Beam, 2016-08-11 06:03:15]

Description was changed from

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=asvitkine@chromium.org
==========

to

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[Dan Beam, 2016-08-11 06:04:53]

gayane@: "this looks good"

we've been talking about this along the way (why gayane@ is on the CC list ;))

[Dan Beam, 2016-08-11 06:05:00]

The CQ bit was checked by dbeam@chromium.org

[Dan Beam, 2016-08-11 06:05:00]

The patchset sent to the CQ was uploaded after l-g-t-m from
dschuyler@chromium.org
Link to the patchset: https://codereview.chromium.org/2233443002/#ps180001
(title: "remove 2 using")

[commit-bot: I haz the power, 2016-08-11 06:05:47]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 06:09:47]

Description was changed from

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-08-11 06:09:48]

Committed patchset #5 (id:180001)

[commit-bot: I haz the power, 2016-08-11 06:12:05]

Description was changed from

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: implement metrics reporting checkbox on desktop

BUG=614265,632443
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/a620af406275d8e5bf37832661f773ec60f3802a
Cr-Commit-Position: refs/heads/master@{#411276}
==========

[commit-bot: I haz the power, 2016-08-11 06:12:06]

Patchset 5 (id:??) landed as
https://crrev.com/a620af406275d8e5bf37832661f773ec60f3802a
Cr-Commit-Position: refs/heads/master@{#411276}