Domain Reliability: Get correct reporting pref once, on startup

Domain Reliability: Get correct reporting pref once, on startup

Instead of wiring the reporting pref all the way down to the Monitor,
grab it once in the ProfileImplIOData, where it's easily accessible and
we already have code to check the right pref on ChromeOS/Android/*.

BUG=409967
Committed: https://crrev.com/91f1bbd1feb4acf767f5fb886b5155a227f56490
Cr-Commit-Position: refs/heads/master@{#298394}

[Deprecated (see juliatuttle), 2014-10-02 20:44:03]

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

[Deprecated (see juliatuttle), 2014-10-02 20:44:04]

PTAL, davidben.

[davidben, 2014-10-03 00:12:25]

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

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
components/domain_reliability/monitor.cc:240: const RequestInfo& request) {
Probably want to
  DCHECK(discard_uploads_set_);
and then fix the tests too. It's a little unfortunate that, by being a friend
and calling private functions, the test didn't notice that it was no longer
honoring Monitor's contract.

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

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
components/domain_reliability/monitor.h:85: // |InitURLRequestContext|.
It also must be called before OnBeforeRedirect and OnCompleted, right? Probably
worth adding that to the documentation.

(Or it could be folded into InitURLRequestContext so the caller can't actually
call it wrong.)

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
File components/domain_reliability/monitor_unittest.cc (right):

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:62:
monitor_.InitURLRequestContext(url_request_context_getter_);
Shouldn't this also call SetDiscardUploads? It looks like these tests are
passing only because they call OnRequestLegComplete directly by being a friend
class.

[Deprecated (see juliatuttle), 2014-10-03 00:42:09]

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

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
components/domain_reliability/monitor.cc:240: const RequestInfo& request) {
On 2014/10/03 00:12:25, David Benjamin wrote:
> Probably want to
>   DCHECK(discard_uploads_set_);
> and then fix the tests too. It's a little unfortunate that, by being a friend
> and calling private functions, the test didn't notice that it was no longer
> honoring Monitor's contract.

Done.

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

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
components/domain_reliability/monitor.h:85: // |InitURLRequestContext|.
On 2014/10/03 00:12:25, David Benjamin wrote:
> It also must be called before OnBeforeRedirect and OnCompleted, right?
Probably
> worth adding that to the documentation.
> 
> (Or it could be folded into InitURLRequestContext so the caller can't actually
> call it wrong.)

Done.

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
File components/domain_reliability/monitor_unittest.cc (right):

https://codereview.chromium.org/615313006/diff/20001/components/domain_reliab...
components/domain_reliability/monitor_unittest.cc:62:
monitor_.InitURLRequestContext(url_request_context_getter_);
On 2014/10/03 00:12:25, David Benjamin wrote:
> Shouldn't this also call SetDiscardUploads? It looks like these tests are
> passing only because they call OnRequestLegComplete directly by being a friend
> class.

Done.

[Deprecated (see juliatuttle), 2014-10-03 17:41:24]

PTAL, davidben.

[davidben, 2014-10-03 17:44:36]

lgtm. Though I'd still prefer SetDiscardUploads be merged into a parameter of
InitURLRequestContext. That way the compiler can ensure you didn't forget to do
it.

[Deprecated (see juliatuttle), 2014-10-03 17:48:30]

On 2014/10/03 17:44:36, David Benjamin wrote:
> lgtm. Though I'd still prefer SetDiscardUploads be merged into a parameter of
> InitURLRequestContext. That way the compiler can ensure you didn't forget to
do
> it.

I'm leaving it separate since it can be toggled at will later. Right now, we
just set it once at start, but I want to make it easy to follow the pref in real
time if we decide we want that.

[Deprecated (see juliatuttle), 2014-10-03 17:48:35]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-10-03 17:49:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615313006/60001

[commit-bot: I haz the power, 2014-10-03 17:56:39]

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

[commit-bot: I haz the power, 2014-10-03 17:56:44]

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

[noms (inactive), 2014-10-03 18:04:22]

noms@chromium.org changed reviewers:
+ noms@chromium.org

[noms (inactive), 2014-10-03 18:04:22]

profiles lgtm

[Deprecated (see juliatuttle), 2014-10-06 17:21:58]

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

[Deprecated (see juliatuttle), 2014-10-06 17:22:00]

mkwst, PTAL at chrome/browser/browsing_data

[Mike West, 2014-10-06 18:26:20]

c/b/browsing_data LGTM.

https://codereview.chromium.org/615313006/diff/60001/chrome/browser/browsing_...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/615313006/diff/60001/chrome/browser/browsing_...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:604: OVERRIDE {
Nit: s/OVERRIDE/override/g

You could do this in a followup CL for the whole file, if you like. :)

[Deprecated (see juliatuttle), 2014-10-06 18:29:33]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-10-06 18:30:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615313006/60001

[commit-bot: I haz the power, 2014-10-06 18:44:45]

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

[commit-bot: I haz the power, 2014-10-06 18:44:46]

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-10-06 19:49:09]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-10-06 19:51:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615313006/80001

[commit-bot: I haz the power, 2014-10-06 22:23:16]

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

[commit-bot: I haz the power, 2014-10-06 22:23:17]

Exceeded time limit waiting for builds to trigger.

[Deprecated (see juliatuttle), 2014-10-07 04:16:19]

The CQ bit was checked by ttuttle@chromium.org

[commit-bot: I haz the power, 2014-10-07 04:18:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/615313006/80001

[commit-bot: I haz the power, 2014-10-07 05:58:05]

Committed patchset #5 (id:80001) as 224fecfd888d680a077bb8cc3a0594ef4dc9207f

[commit-bot: I haz the power, 2014-10-07 05:58:52]

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