Bypass data reduction proxy when using VPN

Bypass data reduction proxy when using VPN

If the tun interface is present, disable the data reduction proxy.

BUG=357129
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=283478

[bengr, 2014-07-08 17:06:35]

isherman: metrics
marq: data_reduction_proxy

[Ilya Sherman, 2014-07-08 23:43:04]

https://codereview.chromium.org/373153003/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/373153003/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2208: </action>
Do you need timestamps for these events?  If not, I'd recommend using histograms
instead.

[bengr, 2014-07-10 18:22:40]

https://codereview.chromium.org/373153003/diff/40001/tools/metrics/actions/ac...
File tools/metrics/actions/actions.xml (right):

https://codereview.chromium.org/373153003/diff/40001/tools/metrics/actions/ac...
tools/metrics/actions/actions.xml:2208: </action>
On 2014/07/08 23:43:04, Ilya Sherman wrote:
> Do you need timestamps for these events?  If not, I'd recommend using
histograms
> instead.

We don't need timestamps for these events, Are you just saying use an enumerated
histogram with each of these as a bucket?

[Ilya Sherman, 2014-07-11 03:45:53]

Thanks.  Metrics LGTM % nits.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:51:
CHANGE_EVENT_TYPE_MAX =2
nit: Please add a space after the '=' sign.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:578:
CHANGE_EVENT_TYPE_MAX);
nit: Please factor out a method for emitting to this histogram, so that there's
less chance that one of the codepaths will have a typo in it.

[marq (ping after 24h), 2014-07-11 16:31:56]

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:44: //
The client IP address changed.
Use a more vertically-compact formatting:

  
  IP_CHANGED = 0,  // The client IP address changed.
  DISABLED_ON_VPN = 1,   // The proxy is disabled because a VPN is running.
  CHANGE_EVENT_TYPE_MAX = 2  // This must always be last.

Also, the prevailing style in Chrome seems to be to name the last UMA enum value
<something>_COUNT, so CHANGE_EVENT_TYPE_COUNT here (or maybe
NETWORK_CHANGE_EVENT_COUNT, to match the UMA name).

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:453: 
Extra line.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:564:
const std::string vpn_interface_name_prefix = "tun";
Please add some comments explaining how this logic determines if the device is
on a VPN. What's the significance of "tun"?

Also, on what platforms is "tun" the interface prefix for the VPN?  If this
logic isn't supported on all platforms, appropriate comments should be made in
the header file.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:578:
CHANGE_EVENT_TYPE_MAX);
On 2014/07/11 03:45:53, Ilya Sherman wrote:
> nit: Please factor out a method for emitting to this histogram, so that
there's
> less chance that one of the codepaths will have a typo in it.

Or at least factor the UMA name into a constant.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:19:
#include "net/base/net_util.h"
Why this include and not a forward declaration of net::NetworkInterfaceList?

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:212: //
Virtualized for mocking
mocking.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:219:
virtual void GetNetworkList(net::NetworkInterfaceList* interfaces,
Needs a comment. *all* method declarations in header files need some kind of
comment (so the two declarations above this one also need them).

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/browser/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings_unittest.cc:312:
0,
Add inline comments documenting the params, eg:

"tun0",  /* interface name */
"tun0",  /* once more with feeling */
0,          /* widget count */ 

... etc.

[bengr, 2014-07-11 22:49:16]

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:44: //
The client IP address changed.
On 2014/07/11 16:31:56, marq_ wrote:
> Use a more vertically-compact formatting:
> 
>   
>   IP_CHANGED = 0,  // The client IP address changed.
>   DISABLED_ON_VPN = 1,   // The proxy is disabled because a VPN is running.
>   CHANGE_EVENT_TYPE_MAX = 2  // This must always be last.
> 
> Also, the prevailing style in Chrome seems to be to name the last UMA enum
value
> <something>_COUNT, so CHANGE_EVENT_TYPE_COUNT here (or maybe
> NETWORK_CHANGE_EVENT_COUNT, to match the UMA name).

Done.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:51:
CHANGE_EVENT_TYPE_MAX =2
On 2014/07/11 03:45:52, Ilya Sherman wrote:
> nit: Please add a space after the '=' sign.

Done.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:453: 
On 2014/07/11 16:31:56, marq_ wrote:
> Extra line.

Done.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:564:
const std::string vpn_interface_name_prefix = "tun";
On 2014/07/11 16:31:56, marq_ wrote:
> Please add some comments explaining how this logic determines if the device is
> on a VPN. What's the significance of "tun"?
> 
> Also, on what platforms is "tun" the interface prefix for the VPN?  If this
> logic isn't supported on all platforms, appropriate comments should be made in
> the header file.

I added a comment. This will work on all Unix-derived platforms. (We'll need to
verify on iOS.) When we port the proxy to Windows, we might need special logic.
I added a TODO.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:578:
CHANGE_EVENT_TYPE_MAX);
On 2014/07/11 03:45:53, Ilya Sherman wrote:
> nit: Please factor out a method for emitting to this histogram, so that
there's
> less chance that one of the codepaths will have a typo in it.

Done.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.cc:578:
CHANGE_EVENT_TYPE_MAX);
On 2014/07/11 03:45:53, Ilya Sherman wrote:
> nit: Please factor out a method for emitting to this histogram, so that
there's
> less chance that one of the codepaths will have a typo in it.

Done.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:19:
#include "net/base/net_util.h"
On 2014/07/11 16:31:56, marq_ wrote:
> Why this include and not a forward declaration of net::NetworkInterfaceList?

NetworkInterfaceList is a vector of network interfaces. I could forward declare
the typedef, but not the NetworkInterface, I think.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:212: //
Virtualized for mocking
On 2014/07/11 16:31:56, marq_ wrote:
> mocking.

Done.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:219:
virtual void GetNetworkList(net::NetworkInterfaceList* interfaces,
On 2014/07/11 16:31:56, marq_ wrote:
> Needs a comment. *all* method declarations in header files need some kind of
> comment (so the two declarations above this one also need them)

Done, but that's not true. I think the style is that all non trivial *public*
methods need documentation.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File
components/data_reduction_proxy/browser/data_reduction_proxy_settings_unittest.cc
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings_unittest.cc:312:
0,
On 2014/07/11 16:31:56, marq_ wrote:
> Add inline comments documenting the params, eg:
> 
> "tun0",  /* interface name */
> "tun0",  /* once more with feeling */
> 0,          /* widget count */ 
> 
> ... etc.

Done.

[marq (ping after 24h), 2014-07-11 23:51:19]

LGTM with more comment nits.

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:219:
virtual void GetNetworkList(net::NetworkInterfaceList* interfaces,
On 2014/07/11 22:49:16, bengr1 wrote:
> On 2014/07/11 16:31:56, marq_ wrote:
> > Needs a comment. *all* method declarations in header files need some kind of
> > comment (so the two declarations above this one also need them)
> 
> Done, but that's not true. I think the style is that all non trivial *public*
> methods need documentation.

That's what I once thought, but the C++ style guide is clear on the matter, and
nothing in the Chromium style guide contradicts it. "Every function declaration
should have comments immediately preceding it that describe what the function
does and how to use it."

https://codereview.chromium.org/373153003/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/373153003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:216: //
Virtualized for mocking.
"Virtualized for mocking" doesn't describe what this method does or how to use
it.

[bengr, 2014-07-14 18:44:57]

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/373153003/diff/80001/components/data_reductio...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:219:
virtual void GetNetworkList(net::NetworkInterfaceList* interfaces,
On 2014/07/11 23:51:19, marq_ wrote:
> On 2014/07/11 22:49:16, bengr1 wrote:
> > On 2014/07/11 16:31:56, marq_ wrote:
> > > Needs a comment. *all* method declarations in header files need some kind
of
> > > comment (so the two declarations above this one also need them)
> > 
> > Done, but that's not true. I think the style is that all non trivial
*public*
> > methods need documentation.
> 
> That's what I once thought, but the C++ style guide is clear on the matter,
and
> nothing in the Chromium style guide contradicts it. "Every function
declaration
> should have comments immediately preceding it that describe what the function
> does and how to use it." 

Acknowledged.

https://codereview.chromium.org/373153003/diff/100001/components/data_reducti...
File components/data_reduction_proxy/browser/data_reduction_proxy_settings.h
(right):

https://codereview.chromium.org/373153003/diff/100001/components/data_reducti...
components/data_reduction_proxy/browser/data_reduction_proxy_settings.h:216: //
Virtualized for mocking.
On 2014/07/11 23:51:19, marq_ wrote:
> "Virtualized for mocking" doesn't describe what this method does or how to use
> it.

Done.

[bengr, 2014-07-16 00:11:41]

The CQ bit was checked by bengr@chromium.org

[commit-bot: I haz the power, 2014-07-16 00:17:54]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/bengr@chromium.org/373153003/130001

[commit-bot: I haz the power, 2014-07-16 20:14:38]

Change committed as 283478