Add DataReductionProxy IPC to determine if the Data Reduction Proxy was used.

Add DataReductionProxy IPC to determine if the Data Reduction Proxy was used.

The current mechanism is to create an instance of DataReductionProxyParams
inside of page_load_histogram, and to then check the proxy server inside of
DocumentState against DataReductionProxyParams. However, the plan is to make
the configuration dynamic, so we can no longer use DataReductionProxyParams.

BUG=452773
Committed: https://crrev.com/364ac1187dced06102a369d0610ed0a853577b27
Cr-Commit-Position: refs/heads/master@{#318917}

[jeremyim, 2015-02-26 20:32:17]

jeremyim@chromium.org changed reviewers:
+ bengr@chromium.org, brettw@chromium.org, creis@chromium.org, ppi@chromium.org,
tsepez@chromium.org

[jeremyim, 2015-02-26 20:32:17]

PTAL, and thank you! =)

tsepez => ipc/ipc_message_start.h

creis => content/public/renderer/document_state.h

ppi => chrome/renderer/page_load_histograms.*

bengr => components/data_reduction_proxy.*,
chrome/browser/chrome_content_browser_client.cc

brettw => *.gyp/*.gypi/*.GN, chrome/browser/chrome_content_browser_client.cc

[bengr, 2015-02-26 22:52:52]

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc
(right):

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:20:
if (settings != nullptr)
Why is this not a DCHECK?

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:24:
DataReductionProxyMessageFilter::~DataReductionProxyMessageFilter() {}
Move close curly to next line.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:40:
DataReductionProxyViewHostMsg_WasDataReductionProxyUsed::ID)
Indent 4 and enclose the next line in braces.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:46:
if (config_) {
These don't need curly braces.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h
(right):

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:20:
class DataReductionProxyMessageFilter
Add a class comment and comment for each method.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:25:
void OnWasDataReductionProxyUsed(const net::HostPortPair& proxy_server,
This name is awful.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(right):

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:15:
bool /* data reduction proxy was used */)
Data Reduction Proxy

[Tom Sepez, 2015-02-26 22:54:55]

You'll need to make security team owners of your new message file in
components/data_reduction_proxy/OWNERS
per-file *_messages.*=set noparent
per-file *_messages.*=dcheng@chromium.org
per-file *_messages.*=inferno@chromium.org
per-file *_messages.*=jln@chromium.org
per-file *_messages.*=jschuh@chromium.org
per-file *_messages.*=kenrb@chromium.org
per-file *_messages.*=mkwst@chromium.org
per-file *_messages.*=nasko@chromium.org
per-file *_messages.*=palmer@chromium.org
per-file *_messages.*=tsepez@chromium.org
per-file *_messages.*=wfh@chromium.org

[jeremyim, 2015-02-27 04:38:41]

OWNERS file updated.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc
(right):

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:20:
if (settings != nullptr)
On 2015/02/26 22:52:52, bengr wrote:
> Why is this not a DCHECK?

Done.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:24:
DataReductionProxyMessageFilter::~DataReductionProxyMessageFilter() {}
On 2015/02/26 22:52:52, bengr wrote:
> Move close curly to next line.

Done.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:40:
DataReductionProxyViewHostMsg_WasDataReductionProxyUsed::ID)
On 2015/02/26 22:52:52, bengr wrote:
> Indent 4 and enclose the next line in braces.

Done.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:46:
if (config_) {
On 2015/02/26 22:52:52, bengr wrote:
> These don't need curly braces.

Done.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h
(right):

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:20:
class DataReductionProxyMessageFilter
On 2015/02/26 22:52:52, bengr wrote:
> Add a class comment and comment for each method.

Done.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:25:
void OnWasDataReductionProxyUsed(const net::HostPortPair& proxy_server,
On 2015/02/26 22:52:52, bengr wrote:
> This name is awful.

Done.

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(right):

https://codereview.chromium.org/966443002/diff/1/components/data_reduction_pr...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:15:
bool /* data reduction proxy was used */)
On 2015/02/26 22:52:52, bengr wrote:
> Data Reduction Proxy

Done.

[ppi, 2015-02-27 10:28:00]

chrome/renderer/page_load_histograms lgtm

https://codereview.chromium.org/966443002/diff/20001/chrome/renderer/page_loa...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/966443002/diff/20001/chrome/renderer/page_loa...
chrome/renderer/page_load_histograms.cc:754: data_reduction_proxy_was_used =
false;
nit: no need for that else block.

https://codereview.chromium.org/966443002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(right):

https://codereview.chromium.org/966443002/diff/20001/components/data_reductio...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:14:
net::HostPortPair /* Proxy server */,
by convention, don't capitalize the first word in these comments

https://codereview.chromium.org/966443002/diff/20001/components/data_reductio...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:15:
bool /* Proxy server is a Data Reduction Proxy */)
Maybe "True iff the proxy server is ..." so that it's clear that you're defining
the contract for the bool.

[jeremyim, 2015-02-27 16:42:53]

https://codereview.chromium.org/966443002/diff/20001/chrome/renderer/page_loa...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/966443002/diff/20001/chrome/renderer/page_loa...
chrome/renderer/page_load_histograms.cc:754: data_reduction_proxy_was_used =
false;
On 2015/02/27 10:28:00, ppi wrote:
> nit: no need for that else block.

*forehead smack*. Right. Done.

https://codereview.chromium.org/966443002/diff/20001/components/data_reductio...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(right):

https://codereview.chromium.org/966443002/diff/20001/components/data_reductio...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:14:
net::HostPortPair /* Proxy server */,
On 2015/02/27 10:28:00, ppi wrote:
> by convention, don't capitalize the first word in these comments

Done.

https://codereview.chromium.org/966443002/diff/20001/components/data_reductio...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:15:
bool /* Proxy server is a Data Reduction Proxy */)
On 2015/02/27 10:28:00, ppi wrote:
> Maybe "True iff the proxy server is ..." so that it's clear that you're
defining
> the contract for the bool.

Done.

[Tom Sepez, 2015-02-27 17:19:39]

lgtm

[Charlie Reis, 2015-02-27 22:07:42]

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
content/public/renderer/document_state.h:164: net::HostPortPair& proxy_server()
{ return proxy_server_; }
Why do you need this change?

[jeremyim, 2015-02-27 22:17:01]

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
content/public/renderer/document_state.h:164: net::HostPortPair& proxy_server()
{ return proxy_server_; }
On 2015/02/27 22:07:42, Charlie Reis wrote:
> Why do you need this change?

I can put it in a separate CL; I noticed that we weren't returning it via ref,
which means that we might be performing a copy when returning the information to
callers.

[bengr, 2015-02-27 22:33:45]

components/data_reduction_proxy.* and
chrome/browser/chrome_content_browser_client.cc lgtm

[Charlie Reis, 2015-02-27 22:37:36]

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
content/public/renderer/document_state.h:164: net::HostPortPair& proxy_server()
{ return proxy_server_; }
On 2015/02/27 22:17:01, jeremyim wrote:
> On 2015/02/27 22:07:42, Charlie Reis wrote:
> > Why do you need this change?
> 
> I can put it in a separate CL; I noticed that we weren't returning it via ref,
> which means that we might be performing a copy when returning the information
to
> callers.

Style guide is against passing around non-const references.  Please keep the
existing const and return a const ref if you want to avoid the copy.

[jeremyim, 2015-02-27 22:50:55]

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/966443002/diff/60001/content/public/renderer/...
content/public/renderer/document_state.h:164: net::HostPortPair& proxy_server()
{ return proxy_server_; }
On 2015/02/27 22:37:36, Charlie Reis wrote:
> On 2015/02/27 22:17:01, jeremyim wrote:
> > On 2015/02/27 22:07:42, Charlie Reis wrote:
> > > Why do you need this change?
> > 
> > I can put it in a separate CL; I noticed that we weren't returning it via
ref,
> > which means that we might be performing a copy when returning the
information
> to
> > callers.
> 
> Style guide is against passing around non-const references.  Please keep the
> existing const and return a const ref if you want to avoid the copy.

Done.

[Charlie Reis, 2015-02-27 23:35:31]

https://codereview.chromium.org/966443002/diff/80001/content/public/renderer/...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/966443002/diff/80001/content/public/renderer/...
content/public/renderer/document_state.h:165: void set_proxy_server(const
net::HostPortPair& proxy_server) {
I don't think you want to make this change on the setter.  Callers may not keep
the original proxy_server alive as long as this object.

[jeremyim, 2015-02-27 23:37:42]

https://codereview.chromium.org/966443002/diff/80001/content/public/renderer/...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/966443002/diff/80001/content/public/renderer/...
content/public/renderer/document_state.h:165: void set_proxy_server(const
net::HostPortPair& proxy_server) {
On 2015/02/27 23:35:31, Charlie Reis wrote:
> I don't think you want to make this change on the setter.  Callers may not
keep
> the original proxy_server alive as long as this object.

Wouldn't we call the copy constructor, since it's stored locally?

[Charlie Reis, 2015-02-27 23:58:39]

Ok, content/public LGTM.

https://codereview.chromium.org/966443002/diff/80001/content/public/renderer/...
File content/public/renderer/document_state.h (right):

https://codereview.chromium.org/966443002/diff/80001/content/public/renderer/...
content/public/renderer/document_state.h:165: void set_proxy_server(const
net::HostPortPair& proxy_server) {
On 2015/02/27 23:37:42, jeremyim wrote:
> On 2015/02/27 23:35:31, Charlie Reis wrote:
> > I don't think you want to make this change on the setter.  Callers may not
> keep
> > the original proxy_server alive as long as this object.
> 
> Wouldn't we call the copy constructor, since it's stored locally?

Most classes in Chrome have DISALLOW_COPY_AND_ASSIGN.  It looks like you're
right in the HostPortPair case, though, so this should work.

[jeremyim, 2015-03-03 00:05:22]

The CQ bit was checked by jeremyim@chromium.org

[jeremyim, 2015-03-03 00:05:23]

The patchset sent to the CQ was uploaded after l-g-t-m from ppi@chromium.org,
tsepez@chromium.org, bengr@chromium.org, creis@chromium.org
Link to the patchset: https://codereview.chromium.org/966443002/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-03-03 00:05:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/966443002/100001

[commit-bot: I haz the power, 2015-03-03 00:10:56]

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

[commit-bot: I haz the power, 2015-03-03 00:10:57]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[jeremyim, 2015-03-03 00:58:06]

The CQ bit was checked by jeremyim@chromium.org

[jeremyim, 2015-03-03 00:58:07]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
creis@chromium.org, bengr@chromium.org, ppi@chromium.org
Link to the patchset: https://codereview.chromium.org/966443002/#ps120001
(title: "Rebase to head")

[commit-bot: I haz the power, 2015-03-03 00:58:29]

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

[commit-bot: I haz the power, 2015-03-03 01:42:29]

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

[commit-bot: I haz the power, 2015-03-03 01:42:30]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[brettw, 2015-03-03 18:42:35]

lgtm

[jeremyim, 2015-03-03 18:43:51]

The CQ bit was checked by jeremyim@chromium.org

[commit-bot: I haz the power, 2015-03-03 18:45:23]

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

[commit-bot: I haz the power, 2015-03-03 18:50:20]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-03-03 18:50:54]

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

[jeremyim, 2015-03-03 18:52:59]

On 2015/03/03 18:50:54, I haz the power (commit-bot) wrote:
> Patchset 7 (id:??) landed as
> https://crrev.com/364ac1187dced06102a369d0610ed0a853577b27
> Cr-Commit-Position: refs/heads/master@{#318917}

Thanks all for reviewing this!