Helper functions in NetworkChangeNotifier related to WWAN use

Helper functions in NetworkChangeNotifier related to WWAN use

On iOS, this is used to disable certain features when WWAN is active in order to
save network traffic and/or battery.

BUG=145954
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=165929

[cbentzel, 2012-10-12 13:18:01]

+pauljensen, who is working on the NCN.

[cbentzel, 2012-10-12 13:20:26]

We may also want to look at making this common with some of how ChromeOS exposes
current radio. We use that to disable prerendering and other speculative work
when over WWAN, for example.

Since this is an API change, may be worth grabbing a few other folks to figure
out where to do it. I agree that NetworkChangeNotifier is probably the right
place to centralize the network state.

[droger, 2012-10-12 13:23:54]

On 2012/10/12 13:20:26, cbentzel wrote:
> We may also want to look at making this common with some of how ChromeOS
exposes
> current radio. We use that to disable prerendering and other speculative work
> when over WWAN, for example.

We also disable pre-rendering on WWAN.
I think this API should be in the network stack (and not at a higher level)
because the network stack itself could behave differently over WWAN.

[pauljensen, 2012-10-12 14:01:15]

I agree centralizing this in the network stack is a good idea, especially when
the alternative is duplicating it for Android, iOS, and CrOS.

I'm fine with the code as it stands now but do have some minor preferences.

http://codereview.chromium.org/11115009/diff/1/net/base/network_change_notifi...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/11115009/diff/1/net/base/network_change_notifi...
net/base/network_change_notifier.cc:289: return type >= CONNECTION_2G && type <=
CONNECTION_4G;
I'd prefer a switch statement.  If you want to stick with using a range of
enumeration values its probably a good idea to add a comment at the enumeration
definition noting that anyone modifying the enumeration needs to understand
certain ranges have connotations.

http://codereview.chromium.org/11115009/diff/1/net/base/network_change_notifi...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/11115009/diff/1/net/base/network_change_notifi...
net/base/network_change_notifier.h:135: // Returns true if the current
connection type uses WWAN.
I'd prefer "Celluar" to "WWAN" as its more common terminology (262M versus 6M
Google.com hits).

http://codereview.chromium.org/11115009/diff/1/net/base/network_change_notifi...
net/base/network_change_notifier.h:136: static bool IsOverWWAN();
Slight preference to "IsOn" or "IsUsing" over "IsOver" just because "On" and
"Using" IMHO imply online to a greater degree than "Over".  Communication on my
cell phone could be over cellular but that doesn't mean its actively using the
connection.

[droger, 2012-10-12 14:28:46]

Thanks, I applied your comments.

I noticed a similar function in chromeos here:
http://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/chromeos/gd...

however I don't see that file in my checkout, I wonder if this still exists.

[droger, 2012-10-12 14:36:57]

Nevermind, I found it, the code was moved.
I'll make that code use the new method.

[Ryan Sleevi, 2012-10-12 17:49:56]

http://codereview.chromium.org/11115009/diff/6003/net/base/network_change_not...
File net/base/network_change_notifier.cc (right):

http://codereview.chromium.org/11115009/diff/6003/net/base/network_change_not...
net/base/network_change_notifier.cc:299: default:
If you remove this "default", you should get a compile error for any unhandled
types in the future.

As written, you're relying on runtime detection (hitting a NOTREACHED in Debug
mode), which is less than ideal.

http://codereview.chromium.org/11115009/diff/6003/net/base/network_change_not...
File net/base/network_change_notifier.h (right):

http://codereview.chromium.org/11115009/diff/6003/net/base/network_change_not...
net/base/network_change_notifier.h:139: static bool
IsConnectionCellular(ConnectionType type);
Having two APIs to do something so functionally close seems less than ideal.
From a usability standpoint, it would be better to pick one idiom.

I suspect the preferred API is the IsConnectionCellcular(ConnectionType) form.


Additionally, I think comments similar to those on line 128-132 are important to
add here, to clarify how much faith callers should put in the return value. For
example, there's no guarantee that a "false" return means they aren't on
cellular - they could be tethered over wifi, for example.

[pauljensen, 2012-10-12 20:35:42]

Would be great if we could replace CrOS's
PrerenderConditionNetwork::CanPrerender() with a call to IsConnectionCellular. 
Maybe for a future CL.

[droger, 2012-10-15 10:09:56]

I applied the comments.

On 2012/10/12 20:35:42, pauljensen wrote:
> Would be great if we could replace CrOS's
> PrerenderConditionNetwork::CanPrerender() with a call to IsConnectionCellular.

> Maybe for a future CL.

That's a good idea. However, I'd prefer doing it in another CL since the
replacement is not straightforward (behavior in the  CONNECTION_UNKNOWN case).

[droger, 2012-10-22 14:00:41]

ping

[pauljensen, 2012-10-22 14:05:07]

https://codereview.chromium.org/11115009/diff/23001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_sync_client.cc (right):

https://codereview.chromium.org/11115009/diff/23001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_sync_client.cc:197:
net::NetworkChangeNotifier::IsConnectionCellular())
How does this build?  I thought you got rid of this function?

[droger, 2012-10-22 15:11:32]

https://codereview.chromium.org/11115009/diff/23001/chrome/browser/chromeos/d...
File chrome/browser/chromeos/drive/drive_sync_client.cc (right):

https://codereview.chromium.org/11115009/diff/23001/chrome/browser/chromeos/d...
chrome/browser/chromeos/drive/drive_sync_client.cc:197:
net::NetworkChangeNotifier::IsConnectionCellular())
On 2012/10/22 14:05:07, pauljensen wrote:
> How does this build?  I thought you got rid of this function?

Oops, I forgot to update this call, thanks.
Fixed now.

[pauljensen, 2012-10-22 15:33:36]

Seems fine to me other than the little typo.  I'm not a committer yet so things
can't appear "good" to me.

https://codereview.chromium.org/11115009/diff/29002/net/base/network_change_n...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/11115009/diff/29002/net/base/network_change_n...
net/base/network_change_notifier.h:136: // Returns false if |type| is
CONNECTION_UNKNOWN., and thus, depending on the
I think ".," is a typo

[Ryan Sleevi, 2012-10-22 17:34:00]

Still a nit on the comment, but the implementation looks good.

https://codereview.chromium.org/11115009/diff/27003/net/base/network_change_n...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/11115009/diff/27003/net/base/network_change_n...
net/base/network_change_notifier.h:139: // current connection is cellular.
I'm not sure this comment still fully captures the cases.

It suggests that CONNECTION_UNKNOWN is the only reason why IsConnectionCellular
would return a mismatch, but the other examples include things such as wifi
tethering / use of a mifi device, where the connection type is Wifi, but the
actual connection to the network is 3G.

Perhaps highlight that GetConnectionType/IsConnectionCellular refers only to the
devices current connection, and may not reflect the actual connection to the
network? I'm sure you can find a better way to word it.

[droger, 2012-10-22 17:41:06]

https://codereview.chromium.org/11115009/diff/27003/net/base/network_change_n...
File net/base/network_change_notifier.h (right):

https://codereview.chromium.org/11115009/diff/27003/net/base/network_change_n...
net/base/network_change_notifier.h:139: // current connection is cellular.
On 2012/10/22 17:34:00, Ryan Sleevi wrote:
> I'm not sure this comment still fully captures the cases.
> 
> It suggests that CONNECTION_UNKNOWN is the only reason why
IsConnectionCellular
> would return a mismatch, but the other examples include things such as wifi
> tethering / use of a mifi device, where the connection type is Wifi, but the
> actual connection to the network is 3G.
> 
> Perhaps highlight that GetConnectionType/IsConnectionCellular refers only to
the
> devices current connection, and may not reflect the actual connection to the
> network? I'm sure you can find a better way to word it.

It seems to me that this discussion applies to the GetConnectionType() function
rather than to IsConnectionCellular().
That said, the point still holds -- I'll try to come up with a good comment.

[droger, 2012-10-24 09:07:46]

I tried to improve the comment.

[Ryan Sleevi, 2012-10-24 23:39:12]

lgtm

[droger, 2012-10-25 09:20:08]

+Zachary for drive changes

[droger, 2012-10-29 17:35:05]

@zork: ping

[droger, 2012-10-31 08:53:28]

Trying with kochi as drive OWNER reviewer (zork seems unresponsive).

[kochi, 2012-11-01 02:32:14]

On 2012/10/31 08:53:28, droger wrote:
> Trying with kochi as drive OWNER reviewer (zork seems unresponsive).

zork is on leave now.
Looking.

[kochi, 2012-11-01 02:36:39]

LGTM

[commit-bot: I haz the power, 2012-11-05 09:53:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/11115009/44001

[commit-bot: I haz the power, 2012-11-05 13:08:19]

Retried try job too often for step(s) content_browsertests

[commit-bot: I haz the power, 2012-11-05 13:10:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/droger@chromium.org/11115009/44001

[commit-bot: I haz the power, 2012-11-05 14:05:57]

Change committed as 165929