Disable precache if data_reduction_proxy enabled.

Disable precache if data_reduction_proxy enabled.

Disable precache if data_reduction_proxy is enabled on this client and the
disable_if_data_reduction_proxy variation parameter is set (non-empty) on the
field trial.

BUG=670070
Committed: https://crrev.com/1190ea49edece62ceb6f1b469364fd026134c695
Cr-Commit-Position: refs/heads/master@{#437386}

[twifkak, 2016-11-30 22:07:47]

twifkak@chromium.org changed reviewers:
+ bengr@chromium.org

[twifkak, 2016-11-30 22:08:03]

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

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

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-11-30 22:12:11]

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

[commit-bot: I haz the power, 2016-11-30 22:12:12]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  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_...)

[twifkak, 2016-11-30 22:52:28]

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

[commit-bot: I haz the power, 2016-11-30 22:52:46]

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-11-30 23:16:23]

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

[commit-bot: I haz the power, 2016-11-30 23:16:24]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[twifkak, 2016-11-30 23:36:20]

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

[commit-bot: I haz the power, 2016-11-30 23:36:45]

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-12-01 00:46:06]

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

[commit-bot: I haz the power, 2016-12-01 00:46:07]

Dry run: This issue passed the CQ dry run.

[twifkak, 2016-12-03 00:19:47]

Description was changed from

==========
Disable precache if data_reduction_proxy enabled.

BUG=670070
==========

to

==========
Disable precache if data_reduction_proxy enabled.

Disable precache if data_reduction_proxy is enabled on this client and the
disable_if_data_reduction_proxy variation parameter is set (non-empty) on the
field trial.

BUG=670070
==========

[twifkak, 2016-12-03 00:19:56]

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

[commit-bot: I haz the power, 2016-12-03 00:21:14]

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-12-03 02:18:02]

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

[commit-bot: I haz the power, 2016-12-03 02:18:03]

Dry run: This issue passed the CQ dry run.

[twifkak, 2016-12-03 02:18:36]

bengr: I added the variation param per our discussion.

[bengr, 2016-12-07 00:18:25]

I presume you tested this and it works?

https://codereview.chromium.org/2544613003/diff/60001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/2544613003/diff/60001/components/precache/con...
components/precache/content/precache_manager.cc:110:
(!data_reduction_proxy_settings_ ||
You want to disable if you don't have a settings object? I would have thought
the condition would be:

(data_reduction_proxy_settings_ &&
!data_reduction_proxy_settings->IsDataReductionProxyEnabled())

[twifkak, 2016-12-07 02:13:35]

Yes, it works.

https://codereview.chromium.org/2544613003/diff/60001/components/precache/con...
File components/precache/content/precache_manager.cc (right):

https://codereview.chromium.org/2544613003/diff/60001/components/precache/con...
components/precache/content/precache_manager.cc:110:
(!data_reduction_proxy_settings_ ||
On 2016/12/07 00:18:25, bengr wrote:
> You want to disable if you don't have a settings object? I would have thought
> the condition would be:
> 
> (data_reduction_proxy_settings_ &&
> !data_reduction_proxy_settings->IsDataReductionProxyEnabled())

I believe the answer to your question is "Yes", but let me ask you, since you
know more about data_reduction_proxy than me.

When does DataReductionProxyChromeSettingsFactory::GetForBrowserContext() return
nullptr? My assumption was one of:

- The user visits a page in cognito. ChromeNetworkDelegate calls
precache::UpdatePrecacheMetricsAndState (in order to maybe record UMAs).
- Something horrible has happened.

In both cases, I'd want to act conservatively and disable.

There's a third possible scenario:
- Native libs just loaded and browser is setting up.

But I just tested this scenario and it doesn't seem to a problem.

[bengr, 2016-12-08 18:25:14]

On 2016/12/07 02:13:35, twifkak wrote:
> Yes, it works.
> 
>
https://codereview.chromium.org/2544613003/diff/60001/components/precache/con...
> File components/precache/content/precache_manager.cc (right):
> 
>
https://codereview.chromium.org/2544613003/diff/60001/components/precache/con...
> components/precache/content/precache_manager.cc:110:
> (!data_reduction_proxy_settings_ ||
> On 2016/12/07 00:18:25, bengr wrote:
> > You want to disable if you don't have a settings object? I would have
thought
> > the condition would be:
> > 
> > (data_reduction_proxy_settings_ &&
> > !data_reduction_proxy_settings->IsDataReductionProxyEnabled())
> 
> I believe the answer to your question is "Yes", but let me ask you, since you
> know more about data_reduction_proxy than me.
> 
> When does DataReductionProxyChromeSettingsFactory::GetForBrowserContext()
return
> nullptr? My assumption was one of:
> 
> - The user visits a page in cognito. ChromeNetworkDelegate calls
> precache::UpdatePrecacheMetricsAndState (in order to maybe record UMAs).
> - Something horrible has happened.
> 
> In both cases, I'd want to act conservatively and disable.
> 
> There's a third possible scenario:
> - Native libs just loaded and browser is setting up.
> 
> But I just tested this scenario and it doesn't seem to a problem.

That's what I would have thought too. I was a little worried that there was a
race at startup (your third scenario), but being conservative sgtm.

LGTM

[twifkak, 2016-12-08 20:51:42]

The CQ bit was checked by twifkak@chromium.org

[commit-bot: I haz the power, 2016-12-08 20:52:50]

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-12-08 20:56:20]

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

[commit-bot: I haz the power, 2016-12-08 20:56:21]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  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_...)

[twifkak, 2016-12-08 21:09:43]

The CQ bit was checked by twifkak@chromium.org

[twifkak, 2016-12-08 21:09:43]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2544613003/#ps80001
(title: "Rebase.")

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

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-12-08 22:56:06]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1481231383653400,
"parent_rev": "71eb6231a7870364ddaa652b8c5ca40cc07b4450", "commit_rev":
"ce561f6d1bf4c7b822afce8c710de1916ee8edee"}

[commit-bot: I haz the power, 2016-12-08 22:56:36]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-08 22:58:15]

Description was changed from

==========
Disable precache if data_reduction_proxy enabled.

Disable precache if data_reduction_proxy is enabled on this client and the
disable_if_data_reduction_proxy variation parameter is set (non-empty) on the
field trial.

BUG=670070
==========

to

==========
Disable precache if data_reduction_proxy enabled.

Disable precache if data_reduction_proxy is enabled on this client and the
disable_if_data_reduction_proxy variation parameter is set (non-empty) on the
field trial.

BUG=670070

Committed: https://crrev.com/1190ea49edece62ceb6f1b469364fd026134c695
Cr-Commit-Position: refs/heads/master@{#437386}
==========

[commit-bot: I haz the power, 2016-12-08 22:58:16]

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