[Android] Simplify "network predictions" preference to a boolean value.

[Android] Simplify "network predictions" preference to a boolean value.

This changes the network predictions preference on Android from a
tri-state (always, only on wifi, never) to a two-state (on, off). The
privacy settings screen now shows a simple checkbox to control this
preference.

"Always" and "only on wifi" are effectively merged into the new "on"
state, which has the same behavior as the old "only on wifi" state:
prefetching and prerendering are allowed only on wifi, while DNS
preresolving and TCP preconnect can happen on all connection types.

Under the hood, the "always" and "only on wifi" states still exist
temporarily, but are treated identically. This change doesn't affect
other platforms, which already use "always" and "only on wifi"
interchangably and show a simple checkbox in the UI.

BUG=585297
Committed: https://crrev.com/1f354ca0573b81547e0921f2a015915a7ccc3dbd
Cr-Commit-Position: refs/heads/master@{#377716}

[newt (away), 2016-02-19 02:57:23]

newt@chromium.org changed reviewers:
+ mmenke@chromium.org

[newt (away), 2016-02-19 02:57:23]

mmenke: Could you take a look at prediction_options.[cc|h] and maybe
pref_service_bridge.cc?

https://codereview.chromium.org/1712943002/diff/1/chrome/browser/net/predicti...
File chrome/browser/net/prediction_options.h (right):

https://codereview.chromium.org/1712943002/diff/1/chrome/browser/net/predicti...
chrome/browser/net/prediction_options.h:21: // TODO(newt): collapse ALWAYS and
WIFI_ONLY into a single value. Possibly
mmenke: Should we keep this enum (and reduce it down to two values, or deprecate
the WIFI_ONLY value), or replace it with a boolean? The former option makes
migration easier (we could even avoid migration altogether). The latter option
seems simpler in the long run. Thoughts?

I'd like to do any changes here as a follow-up CL, though.

[newt (away), 2016-02-19 03:02:37]

newt@chromium.org changed reviewers:
+ donnd@chromium.org

[newt (away), 2016-02-19 03:02:37]

donnd: PTAL at ContextualSearchPolicy.java

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:101:
if (!PrefServiceBridge.getInstance().getNetworkPredictionEnabled()) {
donnd: Is this code correct? Do we really want to be calling
PrefServiceBridge.getNetworkPredictionEnabled() -- which returns the value of
the "network predictions" checkbox in settings, regardless of whether the user
is currently on wi-fi or mobile -- as opposed to
PrefServiceBridge.canPrefetchAndPrerender() -- which returns true only if
network predictions are enabled in settings *and* the user is currently on
wi-fi. I'm not changing the behavior; I just want to make sure we're checking
the right value.

[newt (away), 2016-02-19 03:04:11]

newt@chromium.org changed reviewers:
+ yusufo@chromium.org

[newt (away), 2016-02-19 03:04:11]

yusuf: could you take a look at everything?

[mmenke, 2016-02-19 15:48:40]

https://codereview.chromium.org/1712943002/diff/1/chrome/browser/net/predicti...
File chrome/browser/net/prediction_options.h (right):

https://codereview.chromium.org/1712943002/diff/1/chrome/browser/net/predicti...
chrome/browser/net/prediction_options.h:21: // TODO(newt): collapse ALWAYS and
WIFI_ONLY into a single value. Possibly
On 2016/02/19 02:57:23, newt wrote:
> mmenke: Should we keep this enum (and reduce it down to two values, or
deprecate
> the WIFI_ONLY value), or replace it with a boolean? The former option makes
> migration easier (we could even avoid migration altogether). The latter option
> seems simpler in the long run. Thoughts?
> 
> I'd like to do any changes here as a follow-up CL, though.

My preference is to keep the enum, but get rid of the unused option (Which is
ALWAYS, not WIFI_ONLY, I believe?).

[Yusuf, 2016-02-19 18:28:28]

lgtm with a nit. Thanks newt!

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:529:
public boolean obsoleteNetworkPredictionOptionsHasUserSetting() {
Should we include a bit more about the "obsolete" part in the docs? Maybe a link
to a bug
to date things?

[donnd, 2016-02-19 19:11:03]

donnd@google.com changed reviewers:
+ donnd@google.com, pedrosimonetti@chromium.org

[donnd, 2016-02-19 19:11:04]

Adding Pedro in case I'm missing a broader context for this change/review (I'm
OOO today and just responding to the narrow question posed).

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:101:
if (!PrefServiceBridge.getInstance().getNetworkPredictionEnabled()) {
On 2016/02/19 03:02:37, newt wrote:
> donnd: Is this code correct? Do we really want to be calling
> PrefServiceBridge.getNetworkPredictionEnabled() -- which returns the value of
> the "network predictions" checkbox in settings, regardless of whether the user
> is currently on wi-fi or mobile -- as opposed to
> PrefServiceBridge.canPrefetchAndPrerender() -- which returns true only if
> network predictions are enabled in settings *and* the user is currently on
> wi-fi. I'm not changing the behavior; I just want to make sure we're checking
> the right value.

Yes, this is the intended policy -- to always prefetch unless the user has
disabled prefetching entirely.  With SERP technology like the current "Lite" for
low/slow connections this seems as reasonable as always, though we had some
in-depth discussion about what policy was really best.

[newt (away), 2016-02-19 21:41:53]

newt@chromium.org changed reviewers:
+ dfalcantara@chromium.org

[newt (away), 2016-02-19 21:41:54]

+dan for ChromeActivityTestCaseBase.java

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java
(right):

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/contextualsearch/ContextualSearchPolicy.java:101:
if (!PrefServiceBridge.getInstance().getNetworkPredictionEnabled()) {
On 2016/02/19 19:11:04, donnd wrote:
> On 2016/02/19 03:02:37, newt wrote:
> > donnd: Is this code correct? Do we really want to be calling
> > PrefServiceBridge.getNetworkPredictionEnabled() -- which returns the value
of
> > the "network predictions" checkbox in settings, regardless of whether the
user
> > is currently on wi-fi or mobile -- as opposed to
> > PrefServiceBridge.canPrefetchAndPrerender() -- which returns true only if
> > network predictions are enabled in settings *and* the user is currently on
> > wi-fi. I'm not changing the behavior; I just want to make sure we're
checking
> > the right value.
> 
> Yes, this is the intended policy -- to always prefetch unless the user has
> disabled prefetching entirely.  With SERP technology like the current "Lite"
for
> low/slow connections this seems as reasonable as always, though we had some
> in-depth discussion about what policy was really best.

Ok. Thanks for confirming.

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java
(right):

https://codereview.chromium.org/1712943002/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:529:
public boolean obsoleteNetworkPredictionOptionsHasUserSetting() {
On 2016/02/19 18:28:28, Yusuf wrote:
> Should we include a bit more about the "obsolete" part in the docs? Maybe a
link
> to a bug
> to date things?

Done.

https://codereview.chromium.org/1712943002/diff/1/chrome/browser/net/predicti...
File chrome/browser/net/prediction_options.h (right):

https://codereview.chromium.org/1712943002/diff/1/chrome/browser/net/predicti...
chrome/browser/net/prediction_options.h:21: // TODO(newt): collapse ALWAYS and
WIFI_ONLY into a single value. Possibly
On 2016/02/19 15:48:40, mmenke wrote:
> On 2016/02/19 02:57:23, newt wrote:
> > mmenke: Should we keep this enum (and reduce it down to two values, or
> deprecate
> > the WIFI_ONLY value), or replace it with a boolean? The former option makes
> > migration easier (we could even avoid migration altogether). The latter
option
> > seems simpler in the long run. Thoughts?
> > 
> > I'd like to do any changes here as a follow-up CL, though.
> 
> My preference is to keep the enum, but get rid of the unused option (Which is
> ALWAYS, not WIFI_ONLY, I believe?).

Keeping the enum sounds good to me :)

Neither "ALWAYS" nor "WIFI_ONLY" is an accurate description of the "on" state
(which allows prerendering and prefetching on wi-fi only, but allows dns
preresolving and tcp preconnecting on all connection types). I think it makes
sense to rename the option that we keep to something like "ON" or "ENABLED" or
"ALLOWED".

[pedro (no code reviews), 2016-02-19 23:37:36]

lgtm

lgtm for ContextualSearchPolicy.java

[newt (away), 2016-02-20 00:19:13]

Matt, does chrome/browser/net look good to you?

[newt (away), 2016-02-20 00:19:22]

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

[commit-bot: I haz the power, 2016-02-20 00:22:07]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712943002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712943002/60001

[commit-bot: I haz the power, 2016-02-20 01:11:45]

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

[commit-bot: I haz the power, 2016-02-20 01:11:46]

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

[mmenke, 2016-02-22 16:40:53]

browser/net LGTM...Does look like you have some test failures, though.

[gone, 2016-02-22 19:37:56]

That singular file you asked me to look at lgtm

https://chromiumcodereview.appspot.com/1712943002/diff/60001/chrome/test/andr...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
(right):

https://chromiumcodereview.appspot.com/1712943002/diff/60001/chrome/test/andr...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:206:
* Lets tests specify whether they want prerendering turned on.
nit: update the Javadoc

[newt (away), 2016-02-22 19:38:32]

On 2016/02/22 16:40:53, mmenke wrote:
> browser/net LGTM...Does look like you have some test failures, though.

Thanks. I've updated the tests now to test the new behavior.

[newt (away), 2016-02-22 19:43:34]

newt@chromium.org changed reviewers:
+ gavinp@chromium.org, pasko@chromium.org

[newt (away), 2016-02-22 19:43:35]

pasko: PTAL at chrome/browser/predictors/resource_prefetch_common_unittest.c

gavinp: PTAL at chrome/browser/prefetch/prefetch_browsertest.cc

https://codereview.chromium.org/1712943002/diff/60001/chrome/test/android/jav...
File
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java
(right):

https://codereview.chromium.org/1712943002/diff/60001/chrome/test/android/jav...
chrome/test/android/javatests/src/org/chromium/chrome/test/ChromeActivityTestCaseBase.java:206:
* Lets tests specify whether they want prerendering turned on.
On 2016/02/22 19:37:56, dfalcantara wrote:
> nit: update the Javadoc

Done.

[newt (away), 2016-02-22 19:45:38]

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/predict...
File chrome/browser/predictors/resource_prefetch_common_unittest.cc (left):

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/predict...
chrome/browser/predictors/resource_prefetch_common_unittest.cc:309:
SetPreference(NetworkPredictionOptions::NETWORK_PREDICTION_ALWAYS);
The "ALWAYS" state is being merged into the WIFI_ONLY state. Right now, their
behavior is identical. In a future CL, the states will actually be merged.

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/prefetc...
File chrome/browser/prefetch/prefetch_browsertest.cc (left):

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/prefetc...
chrome/browser/prefetch/prefetch_browsertest.cc:116: // Set preference to
ALWAYS: always prefetch.
The "ALWAYS" state is being merged into the WIFI_ONLY state. Right now, their
behavior is identical. In a future CL, the states will actually be merged.

[newt (away), 2016-02-23 05:35:19]

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

[commit-bot: I haz the power, 2016-02-23 05:35:49]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712943002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712943002/100001

[commit-bot: I haz the power, 2016-02-23 06:18:18]

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

[commit-bot: I haz the power, 2016-02-23 06:18:19]

Dry run: This issue passed the CQ dry run.

[newt (away), 2016-02-24 17:42:21]

gavinp and pasko: friendly ping?

[gavinp, 2016-02-25 16:55:24]

On 2016/02/24 17:42:21, newt wrote:
> gavinp and pasko: friendly ping?

newt: I'm on it. I think pasko is still out of town. I was working on a p0, and
very headdown. Happy to be back above water now, I'm going over this now.

[gavinp, 2016-02-25 16:57:06]

lgtm

[newt (away), 2016-02-25 17:13:57]

newt@chromium.org changed reviewers:
+ zhenw@chromium.org

[newt (away), 2016-02-25 17:13:58]

zhenw: Could you take a look at the small change in
resource_prefetch_common_unittest.cc? Seems like pasko may be OOO. Thanks!

[pasko, 2016-02-25 21:43:04]

chrome/browser/predictors lgtm

sorry for delay, took some time figuring out what our plan is.

[pasko, 2016-02-25 21:43:20]

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/net/pre...
File chrome/browser/net/prediction_options.h (right):

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/net/pre...
chrome/browser/net/prediction_options.h:21: // TODO(newt): collapse ALWAYS and
WIFI_ONLY into a single value.
nit: maybe mention crbug here as well?

[newt (away), 2016-02-25 21:51:09]

The CQ bit was checked by newt@chromium.org

[newt (away), 2016-02-25 21:51:09]

The patchset sent to the CQ was uploaded after l-g-t-m from yusufo@chromium.org,
pedrosimonetti@chromium.org, mmenke@chromium.org, dfalcantara@chromium.org,
gavinp@chromium.org, pasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1712943002/#ps120001
(title: "added bug link; rebased")

[newt (away), 2016-02-25 21:51:24]

Thanks all for reviewing!

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/net/pre...
File chrome/browser/net/prediction_options.h (right):

https://codereview.chromium.org/1712943002/diff/100001/chrome/browser/net/pre...
chrome/browser/net/prediction_options.h:21: // TODO(newt): collapse ALWAYS and
WIFI_ONLY into a single value.
On 2016/02/25 21:43:20, pasko wrote:
> nit: maybe mention crbug here as well?

Done.

[commit-bot: I haz the power, 2016-02-25 21:59:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1712943002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1712943002/120001

[commit-bot: I haz the power, 2016-02-25 23:53:33]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-02-25 23:54:35]

Description was changed from

==========
[Android] Simplify "network predictions" preference to a boolean value.

This changes the network predictions preference on Android from a
tri-state (always, only on wifi, never) to a two-state (on, off). The
privacy settings screen now shows a simple checkbox to control this
preference.

"Always" and "only on wifi" are effectively merged into the new "on"
state, which has the same behavior as the old "only on wifi" state:
prefetching and prerendering are allowed only on wifi, while DNS
preresolving and TCP preconnect can happen on all connection types.

Under the hood, the "always" and "only on wifi" states still exist
temporarily, but are treated identically. This change doesn't affect
other platforms, which already use "always" and "only on wifi"
interchangably and show a simple checkbox in the UI.

BUG=585297
==========

to

==========
[Android] Simplify "network predictions" preference to a boolean value.

This changes the network predictions preference on Android from a
tri-state (always, only on wifi, never) to a two-state (on, off). The
privacy settings screen now shows a simple checkbox to control this
preference.

"Always" and "only on wifi" are effectively merged into the new "on"
state, which has the same behavior as the old "only on wifi" state:
prefetching and prerendering are allowed only on wifi, while DNS
preresolving and TCP preconnect can happen on all connection types.

Under the hood, the "always" and "only on wifi" states still exist
temporarily, but are treated identically. This change doesn't affect
other platforms, which already use "always" and "only on wifi"
interchangably and show a simple checkbox in the UI.

BUG=585297

Committed: https://crrev.com/1f354ca0573b81547e0921f2a015915a7ccc3dbd
Cr-Commit-Position: refs/heads/master@{#377716}
==========

[commit-bot: I haz the power, 2016-02-25 23:54:36]

Patchset 7 (id:??) landed as
https://crrev.com/1f354ca0573b81547e0921f2a015915a7ccc3dbd
Cr-Commit-Position: refs/heads/master@{#377716}