Domain Reliability: Don't upload when metrics reporting is off.

Domain Reliability: Don't upload when metrics reporting is off.

Tie Domain Reliability uploads to metrics reporting -- halt uploads when
reporting is turned off. (Keep recording, since we will eventually be able to
access reports through JavaScript as well.)

BUG=407170
Committed: https://crrev.com/fa8427f940699df3073889af4ab20a1fb2b9ed37
Cr-Commit-Position: refs/heads/master@{#291740}

[Deprecated (see juliatuttle), 2014-08-20 20:37:47]

PTAL, davidben.

[davidben, 2014-08-20 21:49:03]

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:48: void
DomainReliabilityMonitor::InitReportingPref(
Is this called anywhere outside of test code yet?

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:64: OnReportingPrefChanged();
Doesn't this require that |uploader_| be initialized? But |uploader_| is only
initialized in InitURLRequestContext which is called later. (Indeed the tests
seem to segfault.)

It's also strange that OnReportingPrefChanged is usually called on the IO
thread, but here, if I understand correctly, will be called on the UI thread.

Should this line be moved to InitURLRequestContext?

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor.h (right):

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.h:53: // Initializes the Monitor's pref
member that monitors the reporting pref.
Could you document the threading requirements of all these init methods? I've
since forgotten how this code is initialized and am a little confused.
InitReportingPref is called first and then called on the UI thread (or whatever
the generic name for it is... the thread a PrefService lives on)? And then
|network_message_loop_proxy| is where it'll live from now on, and
InitURLRequestContext is required to be called on that task runner? (Which then
is the one it's passed in?)

It would probably helpful if InitReportingPref were hooked up so I could see how
it's supposed to fit together.

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.h:58: network_message_loop_proxy);
s/message_loop_proxy/task_runner/?

[Deprecated (see juliatuttle), 2014-08-21 18:35:29]

PTAL, davidven.

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:48: void
DomainReliabilityMonitor::InitReportingPref(
On 2014/08/20 21:49:02, David Benjamin wrote:
> Is this called anywhere outside of test code yet?

It is now!

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.cc:64: OnReportingPrefChanged();
On 2014/08/20 21:49:03, David Benjamin wrote:
> Doesn't this require that |uploader_| be initialized? But |uploader_| is only
> initialized in InitURLRequestContext which is called later. (Indeed the tests
> seem to segfault.)
> 
> It's also strange that OnReportingPrefChanged is usually called on the IO
> thread, but here, if I understand correctly, will be called on the UI thread.
> 
> Should this line be moved to InitURLRequestContext?

Well-spotted.

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
File components/domain_reliability/monitor.h (right):

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.h:53: // Initializes the Monitor's pref
member that monitors the reporting pref.
On 2014/08/20 21:49:03, David Benjamin wrote:
> Could you document the threading requirements of all these init methods? I've
> since forgotten how this code is initialized and am a little confused.
> InitReportingPref is called first and then called on the UI thread (or
whatever
> the generic name for it is... the thread a PrefService lives on)? And then
> |network_message_loop_proxy| is where it'll live from now on, and
> InitURLRequestContext is required to be called on that task runner? (Which
then
> is the one it's passed in?)
> 
> It would probably helpful if InitReportingPref were hooked up so I could see
how
> it's supposed to fit together.

Done.

https://codereview.chromium.org/491753003/diff/40001/components/domain_reliab...
components/domain_reliability/monitor.h:58: network_message_loop_proxy);
On 2014/08/20 21:49:03, David Benjamin wrote:
> s/message_loop_proxy/task_runner/?

Done.

[Deprecated (see juliatuttle), 2014-08-21 18:35:38]

*davidben

[brettw, 2014-08-21 21:41:31]

base lgtm

[davidben, 2014-08-22 19:00:40]

lgtm with one comment on OnReportingPrefChanged.

Also a suggestion on initializing that might be cleaner, but I didn't look if
it'd work with how the service is initialized. (I'm fine leaving it as a
follow-up or skipping if you don't think it's better that way.)

https://codereview.chromium.org/491753003/diff/100001/components/domain_relia...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/491753003/diff/100001/components/domain_relia...
components/domain_reliability/monitor.cc:67: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
I wonder if the thread_checker_ should be replaced with a
scoped_refptr<base::SingleThreadTaskRunner> that's saved once in
InitReportingPref. Then you don't need to pass it in twice. Three times, really,
if you include the service. Would something like this work with all the
plumbing?

- Move InitReportingPref parameters to Monitor ctor and Service::CreateMonitor
- Service::CreateMonitor passes parameters to Monitor ctor 
- ctor saves network_task_runner as a member and calls MoveToThread.
- InitURLRequestContext must be called on network_task_runner, add DCHECK.
- Instead of passing task_runner as parameter, used the saved one.

https://codereview.chromium.org/491753003/diff/100001/components/domain_relia...
components/domain_reliability/monitor.cc:262: void
DomainReliabilityMonitor::OnReportingPrefChanged() {
Probably want to add a check for uploader_ being NULL here, otherwise you have
another subtle ordering constraint: between InitReportingPref and
InitURLRequestContext, the preference cannot change and fire an IO thread event.

[davidben, 2014-08-22 19:02:10]

https://codereview.chromium.org/491753003/diff/100001/components/domain_relia...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/491753003/diff/100001/components/domain_relia...
components/domain_reliability/monitor.cc:67: const
scoped_refptr<base::SingleThreadTaskRunner>& task_runner) {
On 2014/08/22 19:00:40, David Benjamin wrote:
> - InitURLRequestContext must be called on network_task_runner, add DCHECK.

Er, that got lost in rewording. By 'add DCHECK', I mean add DCHECK for
RunsTasksOnCurrentThread() which I think does the same thing as the
ThreadChecker?

[Deprecated (see juliatuttle), 2014-08-23 00:10:29]

https://codereview.chromium.org/491753003/diff/100001/components/domain_relia...
File components/domain_reliability/monitor.cc (right):

https://codereview.chromium.org/491753003/diff/100001/components/domain_relia...
components/domain_reliability/monitor.cc:262: void
DomainReliabilityMonitor::OnReportingPrefChanged() {
On 2014/08/22 19:00:40, David Benjamin wrote:
> Probably want to add a check for uploader_ being NULL here, otherwise you have
> another subtle ordering constraint: between InitReportingPref and
> InitURLRequestContext, the preference cannot change and fire an IO thread
event.

Done.

[Deprecated (see juliatuttle), 2014-08-23 00:10:39]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-08-23 04:56:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/491753003/160001

[commit-bot: I haz the power, 2014-08-23 06:04:46]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[commit-bot: I haz the power, 2014-08-23 06:07:29]

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

[commit-bot: I haz the power, 2014-08-23 06:07:30]

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

[Deprecated (see juliatuttle), 2014-08-25 16:53:17]

PTAL at profiles, noms.

[noms (inactive), 2014-08-25 16:57:10]

profiles lgtm with a potential nit

https://codereview.chromium.org/491753003/diff/160001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/491753003/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:18: #include
"chrome/browser/browser_process.h"
Is this include needed?

[Deprecated (see juliatuttle), 2014-08-25 17:04:49]

Thanks!

https://codereview.chromium.org/491753003/diff/160001/chrome/browser/profiles...
File chrome/browser/profiles/profile_impl_io_data.cc (right):

https://codereview.chromium.org/491753003/diff/160001/chrome/browser/profiles...
chrome/browser/profiles/profile_impl_io_data.cc:18: #include
"chrome/browser/browser_process.h"
On 2014/08/25 16:57:10, Monica Dinculescu wrote:
> Is this include needed?

Oh, not anymore, thanks.

[Deprecated (see juliatuttle), 2014-08-25 17:04:56]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-08-25 17:05:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/491753003/180001

[commit-bot: I haz the power, 2014-08-25 18:16:38]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[commit-bot: I haz the power, 2014-08-25 18:19:15]

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

[commit-bot: I haz the power, 2014-08-25 18:19:16]

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

[Deprecated (see juliatuttle), 2014-08-25 19:31:53]

ttuttle@chromium.org changed reviewers:
+ michaeln@chromium.org

[Deprecated (see juliatuttle), 2014-08-25 19:31:54]

PTAL at browsing_data, michaeln.

[michaeln, 2014-08-25 19:33:17]

On 2014/08/25 19:31:54, ttuttle wrote:
> PTAL at browsing_data, michaeln.

r/s lgtm

[Deprecated (see juliatuttle), 2014-08-25 19:33:55]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-08-25 19:34:23]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/ttuttle@chromium.org/491753003/180001

[commit-bot: I haz the power, 2014-08-25 19:38:19]

Committed patchset #10 (180001) as 84591c3ab30b2595a0a6936d5305720b3c9d7ac8

[commit-bot: I haz the power, 2014-09-10 02:36:50]

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