Set trusted SPDY proxy dynamically on per-profile basis

Set trusted SPDY proxy dynamically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc: https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682
Committed: https://crrev.com/28e68f81ac373c44d7aa9f12dc8fe91178a6040e
Cr-Commit-Position: refs/heads/master@{#373442}

[tbansal1, 2015-12-28 21:30:38]

Description was changed from

==========
Set trusted spdy proxy

BUG=
==========

to

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Goal is to test the performance (page load time etc.) of 
server-push on Dev and Beta (<=10%) channel.

BUG=558682
==========

[tbansal1, 2015-12-28 21:31:15]

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

[tbansal1, 2015-12-28 21:31:31]

bengr: PTAL. Thanks.

[tbansal1, 2015-12-28 21:33:55]

Patchset #1 (id:1) has been deleted

[bengr, 2015-12-29 17:46:01]

https://codereview.chromium.org/1547273003/diff/20001/chrome/browser/profiles...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1307: // Set trusted SPDY proxy if it
is not set through command line switch, data
Set -> Set the
through -> through the
data -> the data
group of -> group of the

https://codereview.chromium.org/1547273003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1310: if
(params.trusted_spdy_proxy.empty() && IsDataReductionProxyEnabled() &&
I would move this block to a helper method.

https://codereview.chromium.org/1547273003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1313: if
(data_reduction_proxy_io_data()->config()) {
Why not do the following so that a lot of the business logic moves into DRP
code:

if (!params.trusted_spdy_proxy.empty() ||
    !IsDataReductionProxyEnabled()) {
  return;
}

DataReductionProxyConfig* config =
    data_reduction_proxy_io_data->config();

if (!config || !config->IsTrustedSpdyProxyFieldTrialEnabled())
  return;

config->MaybeSetTrustedSpdyProxy(&params.trusted_spdy_proxy);

https://codereview.chromium.org/1547273003/diff/20001/chrome/browser/profiles...
chrome/browser/profiles/profile_io_data.cc:1326: }
Once you rewrite and move to a helper method, please write tests for the helper
and for the method(s) you add to DRPConfig.

https://codereview.chromium.org/1547273003/diff/20001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1547273003/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:182:
// Returns the HTTP proxy servers to be used.
I think this is the wrong interface to expose. Try limiting to the minimum. See
my comment on the previous file.

[tbansal1, 2015-12-29 21:06:40]

Description was changed from

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Goal is to test the performance (page load time etc.) of 
server-push on Dev and Beta (<=10%) channel.

BUG=558682
==========

to

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Goal is to test the performance (page load time etc.) of 
server-push on Dev and Beta channels.

BUG=558682
==========

[tbansal1, 2015-12-29 21:09:19]

Patchset #2 (id:40001) has been deleted

[tbansal1, 2015-12-29 23:03:26]

Patchset #2 (id:60001) has been deleted

[tbansal1, 2015-12-29 23:12:21]

Patchset #2 (id:80001) has been deleted

[tbansal1, 2015-12-29 23:21:27]

Patchset #2 (id:100001) has been deleted

[tbansal1, 2015-12-29 23:25:48]

Patchset #2 (id:120001) has been deleted

[tbansal1, 2015-12-29 23:34:22]

bengr: ptal. thanks.

https://chromiumcodereview.appspot.com/1547273003/diff/20001/chrome/browser/p...
File chrome/browser/profiles/profile_io_data.cc (right):

https://chromiumcodereview.appspot.com/1547273003/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.cc:1307: // Set trusted SPDY proxy if it
is not set through command line switch, data
On 2015/12/29 17:46:00, bengr wrote:
> Set -> Set the
> through -> through the
> data -> the data
> group of -> group of the

Done.

https://chromiumcodereview.appspot.com/1547273003/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.cc:1310: if
(params.trusted_spdy_proxy.empty() && IsDataReductionProxyEnabled() &&
On 2015/12/29 17:46:00, bengr wrote:
> I would move this block to a helper method.

Obsolete, since this is a single line change now.

https://chromiumcodereview.appspot.com/1547273003/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.cc:1313: if
(data_reduction_proxy_io_data()->config()) {
On 2015/12/29 17:46:00, bengr wrote:
> Why not do the following so that a lot of the business logic moves into DRP
> code:
> 
> if (!params.trusted_spdy_proxy.empty() ||
>     !IsDataReductionProxyEnabled()) {
>   return;
> }
> 
> DataReductionProxyConfig* config =
>     data_reduction_proxy_io_data->config();
> 
> if (!config || !config->IsTrustedSpdyProxyFieldTrialEnabled())
>   return;
> 
> config->MaybeSetTrustedSpdyProxy(&params.trusted_spdy_proxy);

Done.

https://chromiumcodereview.appspot.com/1547273003/diff/20001/chrome/browser/p...
chrome/browser/profiles/profile_io_data.cc:1326: }
On 2015/12/29 17:46:00, bengr wrote:
> Once you rewrite and move to a helper method, please write tests for the
helper
> and for the method(s) you add to DRPConfig.

Added tests for methods in DRPConfig.

https://chromiumcodereview.appspot.com/1547273003/diff/20001/components/data_...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://chromiumcodereview.appspot.com/1547273003/diff/20001/components/data_...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:182:
// Returns the HTTP proxy servers to be used.
On 2015/12/29 17:46:00, bengr wrote:
> I think this is the wrong interface to expose. Try limiting to the minimum.
See
> my comment on the previous file.

Done.

[bengr, 2016-01-05 18:42:39]

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1338: void
ProfileIOData::SetTrustedSpdyProxy(std::string* trusted_spdy_proxy) const {
This is a getter not a setter. I would call it:

bool GetDataReductionPrimaryHTTPSProxy(std::string* proxy_name) const;

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1340: if
(IsDataReductionProxyEnabled() && data_reduction_proxy_io_data() &&
Is it ever true that IsDataReductionProyEnabled() returns true and
data_reduction_proxy_io_data() or config() is null? Regardless, I think this
code is busted. What happens if the user enables the DRP during a session? The
trusted SPDY proxy wouldn't be set, right?

Instead I would do somthing like:

if (params.trusted_spdy_proxy.empty())
  if (IsDataReductionProxyConfigured())
    params.trusted_spdy_proxy = GetDataReductionProxyHTTPSProxy();

I'm not sure what IsDataReductionProxyConfigured() would do. If it's possible
that config() is null (I don't think it is), then it would check that. 

I would instead use:

const std::string& GetDataReductionPrimaryHTTPSProxy() const;

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1345: }
How does this work when the client config code is used? I don't think it can if
you set the trusted proxy only when you create the network session. I think
you'll need a different interface.

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:487: // is enabled by the user.
trusted_spdy_proxy must be an empty string.
Why must it be empty? Why can't I reuse a variable?

[tbansal1, 2016-01-07 22:10:50]

Description was changed from

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Goal is to test the performance (page load time etc.) of 
server-push on Dev and Beta channels.

BUG=558682
==========

to

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server.
the new 

BUG=558682
==========

[tbansal1, 2016-01-07 22:11:02]

Description was changed from

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server.
the new 

BUG=558682
==========

to

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server.

BUG=558682
==========

[tbansal1, 2016-01-07 22:11:18]

Description was changed from

==========
Set trusted SPDY proxy on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server.

BUG=558682
==========

to

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server.

BUG=558682
==========

[tbansal1, 2016-01-07 22:12:06]

Description was changed from

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server.

BUG=558682
==========

to

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

[tbansal1, 2016-01-08 00:45:56]

Patchset #3 (id:160001) has been deleted

[tbansal1, 2016-01-08 02:00:48]

Patchset #3 (id:180001) has been deleted

[tbansal1, 2016-01-08 02:01:15]

bengr: PTAL. Thanks.

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1338: void
ProfileIOData::SetTrustedSpdyProxy(std::string* trusted_spdy_proxy) const {
On 2016/01/05 18:42:39, bengr wrote:
> This is a getter not a setter. I would call it:
> 
> bool GetDataReductionPrimaryHTTPSProxy(std::string* proxy_name) const;

Done except the return type is void. if |proxy_name| is empty, then it means, it
was not updated by the function.

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1340: if
(IsDataReductionProxyEnabled() && data_reduction_proxy_io_data() &&
On 2016/01/05 18:42:38, bengr wrote:
> Is it ever true that IsDataReductionProyEnabled() returns true and
> data_reduction_proxy_io_data() or config() is null? Regardless, I think this
> code is busted. What happens if the user enables the DRP during a session? The
> trusted SPDY proxy wouldn't be set, right?
> 
> Instead I would do somthing like:
> 
> if (params.trusted_spdy_proxy.empty())
>   if (IsDataReductionProxyConfigured())
>     params.trusted_spdy_proxy = GetDataReductionProxyHTTPSProxy();
> 
> I'm not sure what IsDataReductionProxyConfigured() would do. If it's possible
> that config() is null (I don't think it is), then it would check that. 
> 
> I would instead use:
> 
> const std::string& GetDataReductionPrimaryHTTPSProxy() const;

Done.

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1345: }
On 2016/01/05 18:42:38, bengr wrote:
> How does this work when the client config code is used? I don't think it can
if
> you set the trusted proxy only when you create the network session. I think
> you'll need a different interface.

Done.

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1547273003/diff/140001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:487: // is enabled by the user.
trusted_spdy_proxy must be an empty string.
On 2016/01/05 18:42:39, bengr wrote:
> Why must it be empty? Why can't I reuse a variable?

Done.

[tbansal1, 2016-01-08 02:35:27]

Patchset #3 (id:200001) has been deleted

[tbansal1, 2016-01-08 02:39:00]

Patchset #3 (id:220001) has been deleted

[bengr, 2016-01-08 20:49:43]

Initial thoughts.

https://codereview.chromium.org/1547273003/diff/240001/net/base/trusted_spdy_...
File net/base/trusted_spdy_proxy_getter.h (right):

https://codereview.chromium.org/1547273003/diff/240001/net/base/trusted_spdy_...
net/base/trusted_spdy_proxy_getter.h:5: #ifndef
NET_BASE_TRUSTED_SPDY_PROXY_GETTER_H_
Why is this not in net/spdy? Suggest calling this TrustedSpdyProxyProvider. Why
does it need to support WeakPtr? Why is it not an interface w/ pure virtuals?

https://codereview.chromium.org/1547273003/diff/240001/net/base/trusted_spdy_...
net/base/trusted_spdy_proxy_getter.h:22: explicit TrustedSpdyProxyGetter(const
std::string& trusted_spdy_proxy);
Why is this a string and not a net::ProxyServer?

[tbansal1, 2016-01-10 20:19:31]

Patchset #4 (id:260001) has been deleted

[tbansal1, 2016-01-10 20:41:53]

Patchset #4 (id:280001) has been deleted

[tbansal1, 2016-01-10 22:33:34]

Patchset #4 (id:270030) has been deleted

[tbansal1, 2016-01-11 19:57:39]

Patchset #4 (id:310001) has been deleted

[tbansal1, 2016-01-11 19:58:53]

ptal. thanks.

https://codereview.chromium.org/1547273003/diff/240001/net/base/trusted_spdy_...
File net/base/trusted_spdy_proxy_getter.h (right):

https://codereview.chromium.org/1547273003/diff/240001/net/base/trusted_spdy_...
net/base/trusted_spdy_proxy_getter.h:5: #ifndef
NET_BASE_TRUSTED_SPDY_PROXY_GETTER_H_
On 2016/01/08 20:49:43, bengr wrote:
> Why is this not in net/spdy? 
Because this could be a non-spdy proxy in future (e.g., QUIC which currently has
its own SPDY code for maintaining sessions).
> Suggest calling this TrustedSpdyProxyProvider. 
Done.
> Why
> does it need to support WeakPtr?
There is no guarantee that TrustedSpdyProxyProvider (e.g., DRP config) will
outlive net stack.
> Why is it not an interface w/ pure virtuals?
Done to make it clearer. Added 2 implementations: First, provides TrustedProxy
based on command line flag. Second, provides based on DRP config.

https://codereview.chromium.org/1547273003/diff/240001/net/base/trusted_spdy_...
net/base/trusted_spdy_proxy_getter.h:22: explicit TrustedSpdyProxyGetter(const
std::string& trusted_spdy_proxy);
On 2016/01/08 20:49:43, bengr wrote:
> Why is this a string and not a net::ProxyServer?

Fixed, this was just an artifact of older code.

[bengr, 2016-01-11 22:29:35]

Here are a few comments.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:976: trusted_spdy_proxy,
net::ProxyServer::SCHEME_HTTP)));
This should take a const ProxyServer&

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread.h:212: // |trusted_spdy_proxy_provider| provides a
trusted SPDY proxy that is
There's no need to use || if what's enclosed is obviously code.

provides a trusted SPDY proxy that is allowed to push cross-origin resources ->
provides a proxy server that is allowed to use SPDY to push resources from other
origins.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread.h:215: scoped_ptr<net::TrustedSpdyProxyProvider>
trusted_spdy_proxy_provider;
I wonder if we should instead have a net::TrustedProxyProvider, so that QUIC
could use it too.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:197: "example.com:443",
net::ProxyServer::SCHEME_HTTP);
Once you fix things up so that everything uses ProxyServer instead of
std::string, also verify that no trusted SPDY proxy is set if the proxy server
set on the command line is invalid.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:202: net::ProxyServer
got_trusted_spdy_proxy;
Call this trusted_spdy_proxy.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1340: if (IsOffTheRecord())
Why not implement this method in ProfileImplIOData instead?

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1347: // Check if
TrustedSpdyProxyProvider is already set.
Why are you checking for a global provider first? I don't think you need lines
1347-1357.

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:655:
const std::vector<net::ProxyServer> proxies_for_http =
Should this be a const std::vector<net::ProxyServer>& ?

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:329:
net::ProxyServer::SCHEME_QUIC},
Add a test to verify that there's no trusted proxy for a Chrome that is not part
of the experiment.

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:952:
scoped_ptr<DataReductionProxyMutableConfigValues> config_values =
#include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:973:
EXPECT_EQ(expected_trusted_spdy_proxy, trusted_spdy_proxy) << i;
What is supposed to happen with QUIC?

[bengr, 2016-01-11 23:58:10]

The current code has each SPDY connection asks for the proxy that is trusted and
then allows cross-origin push if the connection is to that proxy. 

It would be cleaner instead to have each connect ask "am I connected to a
trusted proxy". You could get away from messy interfaces that return a
ProxyServer or ProxyList and instead provide a simple one that returns a bool.

[tbansal1, 2016-01-12 20:50:51]

ptal. thanks.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:976: trusted_spdy_proxy,
net::ProxyServer::SCHEME_HTTP)));
On 2016/01/11 22:29:35, bengr wrote:
> This should take a const ProxyServer&

I do not understand. It is taking a const ProxyServer &.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread.h:212: // |trusted_spdy_proxy_provider| provides a
trusted SPDY proxy that is
On 2016/01/11 22:29:35, bengr wrote:
> There's no need to use || if what's enclosed is obviously code.
> 
> provides a trusted SPDY proxy that is allowed to push cross-origin resources
->
> provides a proxy server that is allowed to use SPDY to push resources from
other
> origins.

Done.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread.h:215: scoped_ptr<net::TrustedSpdyProxyProvider>
trusted_spdy_proxy_provider;
On 2016/01/11 22:29:35, bengr wrote:
> I wonder if we should instead have a net::TrustedProxyProvider, so that QUIC
> could use it too. 

Changed to TrustedSpdyProxyVerifier.

TrustedProxyProvider would not convey that this is only a HTTP2/SPDY  proxy
verifier.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:197: "example.com:443",
net::ProxyServer::SCHEME_HTTP);
On 2016/01/11 22:29:35, bengr wrote:
> Once you fix things up so that everything uses ProxyServer instead of
> std::string, also verify that no trusted SPDY proxy is set if the proxy server
> set on the command line is invalid.

Done.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:202: net::ProxyServer
got_trusted_spdy_proxy;
On 2016/01/11 22:29:35, bengr wrote:
> Call this trusted_spdy_proxy.

obsolete.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1347: // Check if
TrustedSpdyProxyProvider is already set.
On 2016/01/11 22:29:35, bengr wrote:
> Why are you checking for a global provider first? I don't think you need lines
> 1347-1357.

IOThread will set the global provider if a trusted SPDY proxy has been set
through command line flags. We want to pass that to HttpNetworkSession Params
since it has higher preference.

If no trusted SPDY proxy was set from command line, then global provider would
be null. In that case, we should pass DRP_config to HttpNetworkSession Params.

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:655:
const std::vector<net::ProxyServer> proxies_for_http =
On 2016/01/11 22:29:35, bengr wrote:
> Should this be a const std::vector<net::ProxyServer>& ?

Done.

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:329:
net::ProxyServer::SCHEME_QUIC},
On 2016/01/11 22:29:35, bengr wrote:
> Add a test to verify that there's no trusted proxy for a Chrome that is not
part
> of the experiment.

I added that test is in config_unittest.cc.

But, I added here too for more coverage!!

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:952:
scoped_ptr<DataReductionProxyMutableConfigValues> config_values =
On 2016/01/11 22:29:35, bengr wrote:
> #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/1547273003/diff/330001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:973:
EXPECT_EQ(expected_trusted_spdy_proxy, trusted_spdy_proxy) << i;
On 2016/01/11 22:29:35, bengr wrote:
> What is supposed to happen with QUIC?

With QUIC, trusted proxy should not be set (both expected_trusted_spdy_proxy and
trusted_spdy_proxy would be invalid).

[bengr, 2016-01-12 21:43:03]

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:976: trusted_spdy_proxy,
net::ProxyServer::SCHEME_HTTP)));
On 2016/01/12 20:50:50, tbansal1 wrote:
> On 2016/01/11 22:29:35, bengr wrote:
> > This should take a const ProxyServer&
> 
> I do not understand. It is taking a const ProxyServer &.

Whoops. Ack.

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:973: // set on command line.
on the

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:189: // Tests that trusted SPDY proxy is
set correctly from the command line switch.
It seems you lost some changes.

Add the "the" back in.

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:657:
// Trusted SPDY proxy should be a valid secure proxy.
secure -> HTTPS

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:197:
// Overrides net::TrustedSpdyProxyProvider implementation that sets
Revert to:
// Implements net::... and sets |trusted...| to a secure ...

[tbansal1, 2016-01-12 23:31:30]

Patchset #6 (id:370001) has been deleted

[tbansal1, 2016-01-12 23:42:47]

Patchset #6 (id:390001) has been deleted

[tbansal1, 2016-01-12 23:43:38]

Patchset #6 (id:410001) has been deleted

[tbansal1, 2016-01-12 23:43:49]

ptal. thanks.

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
chrome/browser/io_thread.cc:973: // set on command line.
On 2016/01/12 21:43:03, bengr wrote:
> on the

Done.

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
File chrome/browser/io_thread_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/350001/chrome/browser/io_thre...
chrome/browser/io_thread_unittest.cc:189: // Tests that trusted SPDY proxy is
set correctly from the command line switch.
On 2016/01/12 21:43:03, bengr wrote:
> It seems you lost some changes.
> 
> Add the "the" back in.

Done.

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:657:
// Trusted SPDY proxy should be a valid secure proxy.
On 2016/01/12 21:43:03, bengr wrote:
> secure -> HTTPS

Done.

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1547273003/diff/350001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:197:
// Overrides net::TrustedSpdyProxyProvider implementation that sets
On 2016/01/12 21:43:03, bengr wrote:
> Revert to:
> // Implements net::... and sets |trusted...| to a secure ...

Done.

[tbansal1, 2016-01-13 02:11:25]

ptal. thanks.

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/330001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1340: if (IsOffTheRecord())
On 2016/01/11 22:29:35, bengr wrote:
> Why not implement this method in ProfileImplIOData instead?

Because, ProfileIOData implements CreateHttpNetworkSession, and the trusted SPDY
proxy provider is added to HttpNetworkSession. So, it seems meaningful to add it
to HttpNetworkSession where HttpNetworkSession is created.

[tbansal1, 2016-01-15 16:53:07]

Description was changed from

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

to

==========
DO NOT REVIEW 

Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

[bengr, 2016-01-15 20:07:15]

"DO NOT REVIEW" was added because the CL is pending offline agreement that this
is the right design.

[tbansal1, 2016-01-15 22:39:48]

Description was changed from

==========
DO NOT REVIEW 

Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

to

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

[tbansal1, 2016-01-15 22:40:04]

Please review.
Design doc here:
https://docs.google.com/document/d/1-_GAfA9D89UafxZ_8aaQOjnew-PjXtxAca2Wj9CS9...

[tbansal1, 2016-01-15 22:42:26]

Description was changed from

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

to

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Design doc:
https://docs.google.com/document/d/1-_GAfA9D89UafxZ_8aaQOjnew-PjXtxAca2Wj9CS9...

BUG=558682
==========

[tbansal1, 2016-01-16 00:07:22]

Description was changed from

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Design doc:
https://docs.google.com/document/d/1-_GAfA9D89UafxZ_8aaQOjnew-PjXtxAca2Wj9CS9...

BUG=558682
==========

to

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

[bengr, 2016-01-19 04:02:42]

On 2016/01/15 22:40:04, tbansal1 wrote:
> Please review.
> Design doc here:
>
https://docs.google.com/document/d/1-_GAfA9D89UafxZ_8aaQOjnew-PjXtxAca2Wj9CS9...

If you are going to attach a doc to a CL, please make it one that is accessible
publicly. I'd prefer a public doc referenced in a public bug.

[tbansal1, 2016-01-21 19:07:44]

Description was changed from

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

BUG=558682
==========

to

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc:
https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682
==========

[tbansal1, 2016-01-21 19:11:58]

Updated according to the design doc (link in CL description). PTAL! Thanks.

[tbansal1, 2016-01-21 19:12:15]

Description was changed from

==========
Set trusted SPDY proxy dymaically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc:
https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682
==========

to

==========
Set trusted SPDY proxy dynamically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc:
https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682
==========

[tbansal1, 2016-01-26 01:04:43]

ping!

[bengr, 2016-01-26 19:39:55]

Here are some comments to get started.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/io_thre...
chrome/browser/io_thread.h:13: #include <string>
I don't think you need this include anymore.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1333: void
ProfileIOData::SetProxyDelegate(
Line 1302 already does this, so remove this method.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1339: if (IsOffTheRecord())
Can you implement this in ProfileImplIOData instead so you don't need to do this
check?

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1342: // Set proxy_delegate to Data
Reduction Proxy.
Proxy -> Proxy's

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1345: return;
Remove the return and the curly braces.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:436: // Sets the |proxy_delegate| in
|params| on IO thread. |params| should not be
on -> on the
should -> must

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:659:
if (enabled_by_user_ && proxies_for_http.size() >= 1 &&
First, I would flip the logic so you return early if false:
if(!enabled_by_user_ || proxies_for_http.size() == 0 || 
   !trusted_spdy_proxy.is_https()) {
  return false;
}

Then shouldn't you check if trusted_spdy_proxy is anywhere in proxies_for_http?

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:195:
// should be allowed to push cross-origin resources.
proxy, -> proxy

Explain all of the criteria for being allowed to push cross-origin resources.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:347:
base::FieldTrialList::CreateFieldTrial(
How about this?

base::FieldTrialList::CreateFieldTrial(
    params::GetTrustedSpdyProxyFieldTrialName(),
    tests[i].enable_trusted_spdy_proxy_field_trial ? "Enabled" : "Control");

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:390:
bool expect_proxy_as_trusted =
expect_proxy_as_trusted -> expect_proxy_is_trusted?

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:947:
expect_proxy_as_trusted = true;
expect_proxy _is_ trusted?

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:951:
proxies_for_http.push_back(tests[i].proxy);
Shouldn't you try configurations with 0 proxies and with more than one proxy?

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:960:
config->enabled_by_user_ = true;
config->enabled_by_user_ = tests[i].is_data_reduction_proxy_enabled;

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:963:
if (tests[i].is_in_trusted_spdy_proxy_field_trial) {
Use a ternary operator instead of the if/else.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:88:
return base::FieldTrialList::FindFullName(GetTrustedSpdyProxyFieldTrialName())
Do you really need both methods?

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_session.cc:18: #include "net/base/proxy_delegate.h"
Can this be a forward declaration?

[bengr, 2016-01-26 23:23:27]

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7691:
TEST_P(HttpNetworkTransactionTest, CrossOriginProxyPush) {
CrossOriginSPDYProxyPush

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7692: // Configure proxy delegate
to enable cross-origin push.
proxy -> the proxy
enable -> allow
push -> SPDY pushes

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7812: // Configure proxy delegate
to enable cross-origin push.
See above wording suggestions.

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool_unittest.cc (left):

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:64: class TestProxyDelegate :
public ProxyDelegate {
This has nothing to do with this CL. Please do this in a separate CL.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2700: // stream. Bypass this check if and only if this
session is with a trusted
trusted SPDY proxy -> SPDY proxy that is trusted explicitly as determined by the
proxy_delegate_.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session....
net/spdy/spdy_session.h:1187: // proxy_delegate_ verifies if a given proxy is a
trusted SPDY proxy that is
proxy_delegate_ -> The proxy_delegate_
if -> that
proxy that -> proxy, which

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session_...
File net/spdy/spdy_session_pool.h (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool.h:224: const ProxyDelegate* proxy_delegate_;
// Determines if a proxy is a trusted SPDY proxy, which is allowed...

Remove the "in non-incognito sessions" clause. That is a detail of the embedder.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.cc:20: #include
"net/http/http_response_headers.h"
You also need to include "http_request_headers.h"

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:619: ProxyInfo* result) override;
Forward declare ProxyInfo.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:624: void OnBeforeSendHeaders(URLRequest*
request,
Forward declare URLRequest.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:626: HttpRequestHeaders* headers) override;
Forward declare HttpRequestHeaders.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:632: const HttpResponseHeaders&
response_headers) override;
Forward declare HttpResponseHeaders.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:639: net::ProxyServer trusted_spdy_proxy_;
#include "net/proxy/proxy_server.h"

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:640: HostPortPair
on_tunnel_request_completed_endpoint_;
#include "net/base/host_port_pair.h"

[tbansal1, 2016-01-27 00:13:35]

bengr: ptal. thanks.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/io_thre...
File chrome/browser/io_thread.h (right):

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/io_thre...
chrome/browser/io_thread.h:13: #include <string>
On 2016/01/26 19:39:55, bengr wrote:
> I don't think you need this include anymore.

string is used at other places in this file. e.g., see:
Optional<std::string> quic_user_agent_id;

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1333: void
ProfileIOData::SetProxyDelegate(
On 2016/01/26 19:39:55, bengr wrote:
> Line 1302 already does this, so remove this method.

Obsolete.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1339: if (IsOffTheRecord())
On 2016/01/26 19:39:55, bengr wrote:
> Can you implement this in ProfileImplIOData instead so you don't need to do
this
> check?

Obsolete.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1342: // Set proxy_delegate to Data
Reduction Proxy.
On 2016/01/26 19:39:55, bengr wrote:
> Proxy -> Proxy's

Obsolete.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1345: return;
On 2016/01/26 19:39:55, bengr wrote:
> Remove the return and the curly braces.

Obsolete.

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/1547273003/diff/450001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.h:436: // Sets the |proxy_delegate| in
|params| on IO thread. |params| should not be
On 2016/01/26 19:39:55, bengr wrote:
> on -> on the
> should -> must

Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:659:
if (enabled_by_user_ && proxies_for_http.size() >= 1 &&
On 2016/01/26 19:39:55, bengr wrote:
> First, I would flip the logic so you return early if false:
> if(!enabled_by_user_ || proxies_for_http.size() == 0 || 
>    !trusted_spdy_proxy.is_https()) {
>   return false;
> }
Done
> 
> Then shouldn't you check if trusted_spdy_proxy is anywhere in
proxies_for_http?
See: "return trusted_spdy_proxy == proxies_for_http[0];"

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:195:
// should be allowed to push cross-origin resources.
On 2016/01/26 19:39:55, bengr wrote:
> proxy, -> proxy
> 
> Explain all of the criteria for being allowed to push cross-origin resources.

Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:347:
base::FieldTrialList::CreateFieldTrial(
On 2016/01/26 19:39:55, bengr wrote:
> How about this?
> 
> base::FieldTrialList::CreateFieldTrial(
>     params::GetTrustedSpdyProxyFieldTrialName(),
>     tests[i].enable_trusted_spdy_proxy_field_trial ? "Enabled" : "Control");

Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_service_client_unittest.cc:390:
bool expect_proxy_as_trusted =
On 2016/01/26 19:39:55, bengr wrote:
> expect_proxy_as_trusted -> expect_proxy_is_trusted?

Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:947:
expect_proxy_as_trusted = true;
On 2016/01/26 19:39:55, bengr wrote:
> expect_proxy _is_ trusted?

Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:951:
proxies_for_http.push_back(tests[i].proxy);
On 2016/01/26 19:39:55, bengr wrote:
> Shouldn't you try configurations with 0 proxies and with more than one proxy?

Good point. Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:960:
config->enabled_by_user_ = true;
On 2016/01/26 19:39:55, bengr wrote:
> config->enabled_by_user_ = tests[i].is_data_reduction_proxy_enabled;

Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config_unittest.cc:963:
if (tests[i].is_in_trusted_spdy_proxy_field_trial) {
On 2016/01/26 19:39:55, bengr wrote:
> Use a ternary operator instead of the if/else.

Done.

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
File components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc
(right):

https://codereview.chromium.org/1547273003/diff/450001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_params.cc:88:
return base::FieldTrialList::FindFullName(GetTrustedSpdyProxyFieldTrialName())
On 2016/01/26 19:39:55, bengr wrote:
> Do you really need both methods?

Yes, GetTrustedSpdyProxyFieldTrialName() is used in tests. Also, that's the
local style.

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
File net/http/http_network_session.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_session.cc:18: #include "net/base/proxy_delegate.h"
On 2016/01/26 19:39:55, bengr wrote:
> Can this be a forward declaration?

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
File net/http/http_network_transaction_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7691:
TEST_P(HttpNetworkTransactionTest, CrossOriginProxyPush) {
On 2016/01/26 23:23:26, bengr wrote:
> CrossOriginSPDYProxyPush

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7692: // Configure proxy delegate
to enable cross-origin push.
On 2016/01/26 23:23:26, bengr wrote:
> proxy -> the proxy
> enable -> allow
> push -> SPDY pushes

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_network_...
net/http/http_network_transaction_unittest.cc:7812: // Configure proxy delegate
to enable cross-origin push.
On 2016/01/26 23:23:26, bengr wrote:
> See above wording suggestions.

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_proxy_cl...
File net/http/http_proxy_client_socket_pool_unittest.cc (left):

https://codereview.chromium.org/1547273003/diff/450001/net/http/http_proxy_cl...
net/http/http_proxy_client_socket_pool_unittest.cc:64: class TestProxyDelegate :
public ProxyDelegate {
On 2016/01/26 23:23:26, bengr wrote:
> This has nothing to do with this CL. Please do this in a separate CL.

This has been moved to  net/spdy/spdy_test_util_common.h so that
http_network_transaction_unittest.cc can use it. Without this, I would not be
able to write tests in http_network_transaction_unittest.cc.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2700: // stream. Bypass this check if and only if this
session is with a trusted
On 2016/01/26 23:23:26, bengr wrote:
> trusted SPDY proxy -> SPDY proxy that is trusted explicitly as determined by
the
> proxy_delegate_.

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session....
net/spdy/spdy_session.h:1187: // proxy_delegate_ verifies if a given proxy is a
trusted SPDY proxy that is
On 2016/01/26 23:23:26, bengr wrote:
> proxy_delegate_ -> The proxy_delegate_
> if -> that
> proxy that -> proxy, which

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session_...
File net/spdy/spdy_session_pool.h (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_session_...
net/spdy/spdy_session_pool.h:224: const ProxyDelegate* proxy_delegate_;
On 2016/01/26 23:23:26, bengr wrote:
> // Determines if a proxy is a trusted SPDY proxy, which is allowed...
> 
> Remove the "in non-incognito sessions" clause. That is a detail of the
embedder.

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.cc (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.cc:20: #include
"net/http/http_response_headers.h"
On 2016/01/26 23:23:26, bengr wrote:
> You also need to include "http_request_headers.h" 

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:619: ProxyInfo* result) override;
On 2016/01/26 23:23:27, bengr wrote:
> Forward declare ProxyInfo.

Done but is it necessary since this class is overriding a base class. The base
class must have included/forward declared ProxyInfo. Similarly, if the base
class stops using ProxyInfo, then this class would also need to stop using it.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:624: void OnBeforeSendHeaders(URLRequest*
request,
On 2016/01/26 23:23:27, bengr wrote:
> Forward declare URLRequest.

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:626: HttpRequestHeaders* headers) override;
On 2016/01/26 23:23:26, bengr wrote:
> Forward declare HttpRequestHeaders.

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:632: const HttpResponseHeaders&
response_headers) override;
On 2016/01/26 23:23:26, bengr wrote:
> Forward declare HttpResponseHeaders.

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:639: net::ProxyServer trusted_spdy_proxy_;
On 2016/01/26 23:23:27, bengr wrote:
> #include "net/proxy/proxy_server.h"

Done.

https://codereview.chromium.org/1547273003/diff/450001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:640: HostPortPair
on_tunnel_request_completed_endpoint_;
On 2016/01/26 23:23:26, bengr wrote:
> #include "net/base/host_port_pair.h"

Done.

[bengr, 2016-01-30 01:30:01]

https://codereview.chromium.org/1547273003/diff/470001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/470001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1302: DCHECK(!IsOffTheRecord());
I don't think this DCHECK is necessary.

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:194:
// Returns true if trusted_spdy_proxy is a HTTPS data reduction proxy and
One could argue that we don't even need this method because any DRP proxy would
be trusted for cross-origin pushes over SPDY. Maybe instead add (or replace the
existing IsDataReductionProxy()):

bool IsDataReductionProxy(const net::ProxyServer& proxy) const;

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:66:
return config_ && config_->IsTrustedSpdyProxy(proxy_server);
Couldn't you just verify the scheme and then check IsDRP()?

[bengr, 2016-01-30 01:36:43]

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:659:
if (proxies_for_http.size() == 0 || !proxies_for_http[0].is_valid() ||
Why are we only checking against the first proxy?

[tbansal1, 2016-02-01 22:01:42]

Patchset #9 (id:490001) has been deleted

[tbansal1, 2016-02-01 22:02:02]

bengr: ptal.

https://codereview.chromium.org/1547273003/diff/470001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1547273003/diff/470001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:1302: DCHECK(!IsOffTheRecord());
On 2016/01/30 01:30:01, bengr wrote:
> I don't think this DCHECK is necessary.

Removed.

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc
(right):

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:659:
if (proxies_for_http.size() == 0 || !proxies_for_http[0].is_valid() ||
On 2016/01/30 01:36:43, bengr wrote:
> Why are we only checking against the first proxy?

Done.

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h
(right):

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:194:
// Returns true if trusted_spdy_proxy is a HTTPS data reduction proxy and
On 2016/01/30 01:30:01, bengr wrote:
> One could argue that we don't even need this method because any DRP proxy
would
> be trusted for cross-origin pushes over SPDY. Maybe instead add (or replace
the
> existing IsDataReductionProxy()):
> 
> bool IsDataReductionProxy(const net::ProxyServer& proxy) const;

Done.

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc
(right):

https://codereview.chromium.org/1547273003/diff/470001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_delegate.cc:66:
return config_ && config_->IsTrustedSpdyProxy(proxy_server);
On 2016/01/30 01:30:01, bengr wrote:
> Couldn't you just verify the scheme and then check IsDRP()?

Done.

[tbansal1, 2016-02-01 22:59:12]

Patchset #9 (id:510001) has been deleted

[bengr, 2016-02-02 01:00:57]

lgtm

[tbansal1, 2016-02-02 01:04:58]

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

[tbansal1, 2016-02-02 01:04:59]

mmenke: PTAL at everything outside components/d_r_p.
Thanks!

[mmenke, 2016-02-02 16:51:25]

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

[mmenke, 2016-02-02 16:51:26]

[+bnc] for HTTP2 changes.

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
jingle/glue/proxy_resolving_client_socket.cc:87:
session_params.forced_spdy_exclusions =
You should talk to the owners of this file about this change.  This doesn't copy
the ProxyDelegate*, so you're changing behavior here.

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegat...
net/base/proxy_delegate.h:70: // Returns true if proxy_server is a trusted SPDY
proxy that is allowed to
|proxy_server|

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegat...
net/base/proxy_delegate.h:71: // push cross-origin resources.
+"Only affects HTTP2 proxies"?

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegat...
net/base/proxy_delegate.h:73: const net::ProxyServer& proxy_server) const = 0;
Suggest removing the const

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2710: ProxyServer(ProxyServer::SCHEME_HTTPS,
host_port_pair()))) {
I'm assuming this isn't done for non-proxy SpdySessions, but I don't see why
not.

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.h:1191: // The proxy_delegate_ verifies that a given proxy
is a trusted SPDY proxy,
nit:  |proxy_delegate_|

https://codereview.chromium.org/1547273003/diff/530001/net/url_request/url_re...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/1547273003/diff/530001/net/url_request/url_re...
net/url_request/url_request_context_builder.h:89: ProxyDelegate* proxy_delegate;
Doesn't look like this is used anywhere, so let's get rid of it, for now.

[tbansal1, 2016-02-02 18:18:19]

Patchset #10 (id:550001) has been deleted

[tbansal1, 2016-02-02 18:18:44]

bnc: PTAL. Thanks.

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
jingle/glue/proxy_resolving_client_socket.cc:87:
session_params.forced_spdy_exclusions =
On 2016/02/02 16:51:26, mmenke wrote:
> You should talk to the owners of this file about this change.  This doesn't
copy
> the ProxyDelegate*, so you're changing behavior here.

I do not understand this comment. Currently, ProxyDelegate* is not being copied.
And, this CL is not changing that behavior.

Also, this code is related to XMPP/WebRTC where it is not not useful to enable
SPDY push.

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegate.h
File net/base/proxy_delegate.h (right):

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegat...
net/base/proxy_delegate.h:70: // Returns true if proxy_server is a trusted SPDY
proxy that is allowed to
On 2016/02/02 16:51:26, mmenke wrote:
> |proxy_server|

Done.

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegat...
net/base/proxy_delegate.h:71: // push cross-origin resources.
On 2016/02/02 16:51:26, mmenke wrote:
> +"Only affects HTTP2 proxies"?

Done.

https://codereview.chromium.org/1547273003/diff/530001/net/base/proxy_delegat...
net/base/proxy_delegate.h:73: const net::ProxyServer& proxy_server) const = 0;
On 2016/02/02 16:51:26, mmenke wrote:
> Suggest removing the const

Done.

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2710: ProxyServer(ProxyServer::SCHEME_HTTPS,
host_port_pair()))) {
On 2016/02/02 16:51:26, mmenke wrote:
> I'm assuming this isn't done for non-proxy SpdySessions, but I don't see why
> not.

Right, it isn't done for non-proxy sessions because they are not the
authoritative source of the resource -- if the client had fetched the resource
by itself, it would have fetched from a different server.

For proxies (especially data reduction proxy), the proxy is from where the
client would have fetched the resource anyways.

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.h:1191: // The proxy_delegate_ verifies that a given proxy
is a trusted SPDY proxy,
On 2016/02/02 16:51:26, mmenke wrote:
> nit:  |proxy_delegate_|

Done.

https://codereview.chromium.org/1547273003/diff/530001/net/url_request/url_re...
File net/url_request/url_request_context_builder.h (right):

https://codereview.chromium.org/1547273003/diff/530001/net/url_request/url_re...
net/url_request/url_request_context_builder.h:89: ProxyDelegate* proxy_delegate;
On 2016/02/02 16:51:26, mmenke wrote:
> Doesn't look like this is used anywhere, so let's get rid of it, for now.

Done.

[mmenke, 2016-02-02 18:45:02]

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
jingle/glue/proxy_resolving_client_socket.cc:87:
session_params.forced_spdy_exclusions =
On 2016/02/02 18:18:44, tbansal1 wrote:
> On 2016/02/02 16:51:26, mmenke wrote:
> > You should talk to the owners of this file about this change.  This doesn't
> copy
> > the ProxyDelegate*, so you're changing behavior here.
> 
> I do not understand this comment. Currently, ProxyDelegate* is not being
copied.
> And, this CL is not changing that behavior.
> 
> Also, this code is related to XMPP/WebRTC where it is not not useful to enable
> SPDY push.

Right, but trusted_spdy_proxy was being copied, as is the network_delegate.

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2710: ProxyServer(ProxyServer::SCHEME_HTTPS,
host_port_pair()))) {
On 2016/02/02 18:18:44, tbansal1 wrote:
> On 2016/02/02 16:51:26, mmenke wrote:
> > I'm assuming this isn't done for non-proxy SpdySessions, but I don't see why
> > not.
> 
> Right, it isn't done for non-proxy sessions because they are not the
> authoritative source of the resource -- if the client had fetched the resource
> by itself, it would have fetched from a different server.
> 
> For proxies (especially data reduction proxy), the proxy is from where the
> client would have fetched the resource anyways.

I'm sorry, I meant I don't see in the code why this code isn't run.  There's no
check in this method that this is a proxy / the non-authoritative source of the
resource.

[tbansal1, 2016-02-02 19:34:32]

ptal.

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
File jingle/glue/proxy_resolving_client_socket.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/jingle/glue/proxy_reso...
jingle/glue/proxy_resolving_client_socket.cc:87:
session_params.forced_spdy_exclusions =
On 2016/02/02 18:45:02, mmenke wrote:
> On 2016/02/02 18:18:44, tbansal1 wrote:
> > On 2016/02/02 16:51:26, mmenke wrote:
> > > You should talk to the owners of this file about this change.  This
doesn't
> > copy
> > > the ProxyDelegate*, so you're changing behavior here.
> > 
> > I do not understand this comment. Currently, ProxyDelegate* is not being
> copied.
> > And, this CL is not changing that behavior.
> > 
> > Also, this code is related to XMPP/WebRTC where it is not not useful to
enable
> > SPDY push.
> 
> Right, but trusted_spdy_proxy was being copied, as is the network_delegate.

Acked, the concept of trusted SPDY proxy is getting removed by this CL. I am
pretty sure it is okay but I will check with the jingle/ OWNERS.

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2710: ProxyServer(ProxyServer::SCHEME_HTTPS,
host_port_pair()))) {
On 2016/02/02 18:45:02, mmenke wrote:
> On 2016/02/02 18:18:44, tbansal1 wrote:
> > On 2016/02/02 16:51:26, mmenke wrote:
> > > I'm assuming this isn't done for non-proxy SpdySessions, but I don't see
why
> > > not.
> > 
> > Right, it isn't done for non-proxy sessions because they are not the
> > authoritative source of the resource -- if the client had fetched the
resource
> > by itself, it would have fetched from a different server.
> > 
> > For proxies (especially data reduction proxy), the proxy is from where the
> > client would have fetched the resource anyways.
> 
> I'm sorry, I meant I don't see in the code why this code isn't run.  There's
no
> check in this method that this is a proxy / the non-authoritative source of
the
> resource.

Yeah, SPDY Session is shared for proxied and non-proxied resources. e.g., I can
use proxy.googlezip.net (Flywheel proxy) to download proxy resources, and the
same SPDY session will be used to download proxy.googlezip.net/index.html.

Conceptually, it seems that it should be the responsibility of ProxyDelegate to
handle this. At some level, this CL does that. e.g., it checks if DRP is
enabled, and if the specified proxy is an HTTPS DRP proxy or not.

[mmenke, 2016-02-02 19:50:45]

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2710: ProxyServer(ProxyServer::SCHEME_HTTPS,
host_port_pair()))) {
On 2016/02/02 19:34:32, tbansal1 wrote:
> On 2016/02/02 18:45:02, mmenke wrote:
> > On 2016/02/02 18:18:44, tbansal1 wrote:
> > > On 2016/02/02 16:51:26, mmenke wrote:
> > > > I'm assuming this isn't done for non-proxy SpdySessions, but I don't see
> why
> > > > not.
> > > 
> > > Right, it isn't done for non-proxy sessions because they are not the
> > > authoritative source of the resource -- if the client had fetched the
> resource
> > > by itself, it would have fetched from a different server.
> > > 
> > > For proxies (especially data reduction proxy), the proxy is from where the
> > > client would have fetched the resource anyways.
> > 
> > I'm sorry, I meant I don't see in the code why this code isn't run.  There's
> no
> > check in this method that this is a proxy / the non-authoritative source of
> the
> > resource.
> 
> Yeah, SPDY Session is shared for proxied and non-proxied resources. e.g., I
can
> use http://proxy.googlezip.net (Flywheel proxy) to download proxy resources,
and the
> same SPDY session will be used to download proxy.googlezip.net/index.html.
> 
> Conceptually, it seems that it should be the responsibility of ProxyDelegate
to
> handle this. At some level, this CL does that. e.g., it checks if DRP is
> enabled, and if the specified proxy is an HTTPS DRP proxy or not.

Wait...Does that mean this check prevents a trusted HTTPS SPDY proxy from
pushing its own resources?

[tbansal1, 2016-02-02 20:02:00]

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session.cc
File net/spdy/spdy_session.cc (right):

https://codereview.chromium.org/1547273003/diff/530001/net/spdy/spdy_session....
net/spdy/spdy_session.cc:2710: ProxyServer(ProxyServer::SCHEME_HTTPS,
host_port_pair()))) {
On 2016/02/02 19:50:45, mmenke wrote:
> On 2016/02/02 19:34:32, tbansal1 wrote:
> > On 2016/02/02 18:45:02, mmenke wrote:
> > > On 2016/02/02 18:18:44, tbansal1 wrote:
> > > > On 2016/02/02 16:51:26, mmenke wrote:
> > > > > I'm assuming this isn't done for non-proxy SpdySessions, but I don't
see
> > why
> > > > > not.
> > > > 
> > > > Right, it isn't done for non-proxy sessions because they are not the
> > > > authoritative source of the resource -- if the client had fetched the
> > resource
> > > > by itself, it would have fetched from a different server.
> > > > 
> > > > For proxies (especially data reduction proxy), the proxy is from where
the
> > > > client would have fetched the resource anyways.
> > > 
> > > I'm sorry, I meant I don't see in the code why this code isn't run. 
There's
> > no
> > > check in this method that this is a proxy / the non-authoritative source
of
> > the
> > > resource.
> > 
> > Yeah, SPDY Session is shared for proxied and non-proxied resources. e.g., I
> can
> > use http://proxy.googlezip.net (Flywheel proxy) to download proxy resources,
> and the
> > same SPDY session will be used to download proxy.googlezip.net/index.html.
> > 
> > Conceptually, it seems that it should be the responsibility of ProxyDelegate
> to
> > handle this. At some level, this CL does that. e.g., it checks if DRP is
> > enabled, and if the specified proxy is an HTTPS DRP proxy or not.
> 
> Wait...Does that mean this check prevents a trusted HTTPS SPDY proxy from
> pushing its own resources?

Right, trusted HTTPS SPDY proxy cannot push its own resources. I will send
another CL to fix  that.

[Bence, 2016-02-03 00:40:04]

net/spdy LGTM.

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session....
net/spdy/spdy_session.h:1193: // those of their associated streams. May be NULL.
Optional: s/NULL/nullptr/ as recommended by https://chromium-cpp.appspot.com.

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session_...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:4937: scoped_ptr<TestProxyDelegate>
proxy_delegate(new TestProxyDelegate());
Move these new lines down to just above line 4968 where they are used.

[tbansal1, 2016-02-03 01:22:49]

mmenke: ptal. Thanks.

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session.h
File net/spdy/spdy_session.h (right):

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session....
net/spdy/spdy_session.h:1193: // those of their associated streams. May be NULL.
On 2016/02/03 00:40:04, Bence wrote:
> Optional: s/NULL/nullptr/ as recommended by https://chromium-cpp.appspot.com.

Done.

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session_...
File net/spdy/spdy_session_unittest.cc (right):

https://codereview.chromium.org/1547273003/diff/570001/net/spdy/spdy_session_...
net/spdy/spdy_session_unittest.cc:4937: scoped_ptr<TestProxyDelegate>
proxy_delegate(new TestProxyDelegate());
On 2016/02/03 00:40:04, Bence wrote:
> Move these new lines down to just above line 4968 where they are used.

Done.

[tbansal1, 2016-02-03 01:23:51]

tbansal@chromium.org changed reviewers:
+ pkasting@chromium.org, zea@chromium.org

[tbansal1, 2016-02-03 01:23:52]

zea: ptal at jingle/glue/proxy_resolving_client_socket.cc
pkasting: ptal at chrome/browser/ui/startup/bad_flags_prompt.cc

Thanks!

[Bence, 2016-02-03 01:42:28]

Please remove kTrustedSpdyProxy from chrome/common/chrome_switches.h as well.

Please remove the comment referring to the --trusted-spdy-proxy switch from
spdy_session.cc.  (This will be a merge conflict with
https://crrev.com/1665523002, which hopefully will land soon so it should not
delay this CL.)

Thank you.

https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
File chrome/browser/ui/startup/bad_flags_prompt.cc (left):

https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
chrome/browser/ui/startup/bad_flags_prompt.cc:56: // These flags disable or
undermine the Same Origin Policy.
I do not know what kTranslateSecurityOrigin does.  Are you sure you mean to
remove this comment?

[Peter Kasting, 2016-02-03 01:45:37]

LGTM

https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
File chrome/browser/ui/startup/bad_flags_prompt.cc (left):

https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
chrome/browser/ui/startup/bad_flags_prompt.cc:56: // These flags disable or
undermine the Same Origin Policy.
On 2016/02/03 01:42:28, Bence wrote:
> I do not know what kTranslateSecurityOrigin does.  Are you sure you mean to
> remove this comment?

Yeah, removing this comment doesn't seem right.  It should be updated to be
singular instead.

[tbansal1, 2016-02-03 01:55:12]

On 2016/02/03 01:42:28, Bence wrote:
> Please remove kTrustedSpdyProxy from chrome/common/chrome_switches.h as well.
Done. Thanks for catching this.
> 
> Please remove the comment referring to the --trusted-spdy-proxy switch from
> spdy_session.cc.  (This will be a merge conflict with
> https://crrev.com/1665523002, which hopefully will land soon so it should not
> delay this CL.)
This CL already removes that from the comment in spdy_session.cc.
> 
> Thank you.
> 
>
https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
> File chrome/browser/ui/startup/bad_flags_prompt.cc (left):
> 
>
https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
> chrome/browser/ui/startup/bad_flags_prompt.cc:56: // These flags disable or
> undermine the Same Origin Policy.
> I do not know what kTranslateSecurityOrigin does.  Are you sure you mean to
> remove this comment?
Done.

[tbansal1, 2016-02-03 01:55:33]

https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
File chrome/browser/ui/startup/bad_flags_prompt.cc (left):

https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
chrome/browser/ui/startup/bad_flags_prompt.cc:56: // These flags disable or
undermine the Same Origin Policy.
On 2016/02/03 01:42:28, Bence wrote:
> I do not know what kTranslateSecurityOrigin does.  Are you sure you mean to
> remove this comment?

Done.

https://codereview.chromium.org/1547273003/diff/590001/chrome/browser/ui/star...
chrome/browser/ui/startup/bad_flags_prompt.cc:56: // These flags disable or
undermine the Same Origin Policy.
On 2016/02/03 01:45:37, Peter Kasting wrote:
> On 2016/02/03 01:42:28, Bence wrote:
> > I do not know what kTranslateSecurityOrigin does.  Are you sure you mean to
> > remove this comment?
> 
> Yeah, removing this comment doesn't seem right.  It should be updated to be
> singular instead.

Done.

[Bence, 2016-02-03 01:58:32]

On 2016/02/03 01:55:12, tbansal1 wrote:
> On 2016/02/03 01:42:28, Bence wrote:
> > Please remove kTrustedSpdyProxy from chrome/common/chrome_switches.h as
well.
> Done. Thanks for catching this.
> > 
> > Please remove the comment referring to the --trusted-spdy-proxy switch from
> > spdy_session.cc.  (This will be a merge conflict with
> > https://crrev.com/1665523002, which hopefully will land soon so it should
not
> > delay this CL.)
> This CL already removes that from the comment in spdy_session.cc.
>

You are right.  I'm sorry I overlooked it.

[Nicolas Zea, 2016-02-03 02:48:27]

jingle LGTM

[mmenke, 2016-02-03 22:43:04]

Sorry, forgot about this CL.  Just one comment.  Otherwise, LGTM (Didn't look at
net/spdy, but did look at other net files and io_thread).

https://codereview.chromium.org/1547273003/diff/610001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1547273003/diff/610001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:595: class TestProxyDelegate : public
ProxyDelegate {
This seems like a weird place for this file.  Maybe move it into
net/base/test_proxy_delegate?

[tbansal1, 2016-02-04 00:25:13]

Patchset #14 (id:650001) has been deleted

[tbansal1, 2016-02-04 00:36:52]

https://codereview.chromium.org/1547273003/diff/610001/net/spdy/spdy_test_uti...
File net/spdy/spdy_test_util_common.h (right):

https://codereview.chromium.org/1547273003/diff/610001/net/spdy/spdy_test_uti...
net/spdy/spdy_test_util_common.h:595: class TestProxyDelegate : public
ProxyDelegate {
On 2016/02/03 22:43:04, mmenke wrote:
> This seems like a weird place for this file.  Maybe move it into
> net/base/test_proxy_delegate?

net/base/test_proxy_delegate definitely makes more sense. Done.

[tbansal1, 2016-02-04 02:18:47]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2016-02-04 02:18:47]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org,
bnc@chromium.org, pkasting@chromium.org, zea@chromium.org, mmenke@chromium.org
Link to the patchset: https://codereview.chromium.org/1547273003/#ps670001
(title: "Added net/base/test_proxy_delegate.{h,cc}")

[commit-bot: I haz the power, 2016-02-04 02:21:29]

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

[commit-bot: I haz the power, 2016-02-04 02:56:22]

Description was changed from

==========
Set trusted SPDY proxy dynamically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc:
https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682
==========

to

==========
Set trusted SPDY proxy dynamically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc:
https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682
==========

[commit-bot: I haz the power, 2016-02-04 02:56:24]

Committed patchset #14 (id:670001)

[commit-bot: I haz the power, 2016-02-04 02:57:32]

Description was changed from

==========
Set trusted SPDY proxy dynamically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY 
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if 
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in 
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session 
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc:
https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682
==========

to

==========
Set trusted SPDY proxy dynamically on per-profile basis

Set trusted SPDY proxy on per-profile basis if the profile
has DRP enabled, and if Chrome is part of the field trial.
Prior to this CL, it was possible to set the trusted SPDY
proxy only from command line switch.

Setting it on per-profile ensures that it is set only if
DRP is enabled (which is per-profile).

Setting it dynamically ensures that if there is a change in
DataReductionProxy config (e.g., the API tells Chrome to
connect to a different DRP server), then the SPDY session
to the different server has the trusted SPDY proxy set
correctly to the new server. Prior to this CL, trusted
SPDY proxy was set once at the time of Chrome start, and it
was not possible to change it.

Link to design doc:
https://code.google.com/p/chromium/issues/detail?id=558682#c11

BUG=558682

Committed: https://crrev.com/28e68f81ac373c44d7aa9f12dc8fe91178a6040e
Cr-Commit-Position: refs/heads/master@{#373442}
==========

[commit-bot: I haz the power, 2016-02-04 02:57:33]

Patchset 14 (id:??) landed as
https://crrev.com/28e68f81ac373c44d7aa9f12dc8fe91178a6040e
Cr-Commit-Position: refs/heads/master@{#373442}