Modify IPC call to properly record the PLT histograms for LoFi.

Modify IPC call to Data Reduction Proxy to properly record
the PLT histograms for LoFi.

This CL adds 2 more boolean parameters to an existing IPC call
that tells us whether the page load happened with LoFi on with
bad network conditions or LoFi off with
bad network  conditions. The methods in the data reduction
proxy code currently return false and would be fixed in the
forthcoming CL.

Auto LoFi status keeps changing (on a page load basis)
depending on the network conditions (e.g., enabled only when network
quality is bad). Just because session is in a field trial does
not guarantee that LoFi would ever be used.  Using a synthetic
trial or only using the field trial will give very
noisy data in this case.

This is a temporary CL for LoFi Android experiment. Eventually,
we want to read the LoFi status using the headers of the
response instead of reading the current LoFi status (which might
be a bit stale) but that requires putting custom
HTTP headers in the response message from data saver proxy.

BUG=485633
Committed: https://crrev.com/9b2675127847bbdecbbd630ee4c13fec33e67f4b
Cr-Commit-Position: refs/heads/master@{#329776}

[tbansal1, 2015-05-11 16:23:23]

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

[tbansal1, 2015-05-11 16:23:23]

ptal. thanks.

[jeremyim, 2015-05-11 17:30:11]

On 2015/05/11 16:23:23, tbansal1 wrote:
> ptal. thanks.

You may want to tweak the CL description; we don't actually know if the page
load happened with LoFi on or not (since that requires knowledge of some kind of
page request identifier), only whether LoFi is on or not when the IPC call is
made. If this is a temporary implementation, please call that out.

[jeremyim, 2015-05-11 17:30:21]

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

https://codereview.chromium.org/1140443002/diff/20001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:830: DCHECK(!(lofi_enabled &&
lofi_control));
Shouldn't this just be an enum if it's a tri-state? Then you can get rid of the
DCHECKs.

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

https://codereview.chromium.org/1140443002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:234:
// network quality estimated by NQE.
At least add the thread_checker_ DCHECK.

[tbansal1, 2015-05-11 21:22:53]

thanks for the comments. ptal

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

https://codereview.chromium.org/1140443002/diff/20001/chrome/renderer/page_lo...
chrome/renderer/page_load_histograms.cc:830: DCHECK(!(lofi_enabled &&
lofi_control));
On 2015/05/11 17:30:21, jeremyim wrote:
> Shouldn't this just be an enum if it's a tri-state? Then you can get rid of
the
> DCHECKs.

Done.

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

https://codereview.chromium.org/1140443002/diff/20001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.cc:234:
// network quality estimated by NQE.
On 2015/05/11 17:30:21, jeremyim wrote:
> At least add the thread_checker_ DCHECK.

Done.

[jeremyim, 2015-05-12 03:36:50]

Is there anything preventing the use of headers right now? If possible, it would
be nice not to have a temporary change for IPC.

[tbansal1, 2015-05-12 05:04:12]

On 2015/05/12 03:36:50, jeremyim wrote:
> Is there anything preventing the use of headers right now? If possible, it
would
> be nice not to have a temporary change for IPC.

We do not have any special header in the responses coming from Data
saver proxy (unlike the requests which have "chrome-Proxy" header).
Not sure if we want to add a new custom header for this experiment
while it is in Beta.
That might also require significant changes on the server side.
WDYT?

[jeremyim, 2015-05-12 18:54:30]

On 2015/05/12 05:04:12, tbansal1 wrote:
> On 2015/05/12 03:36:50, jeremyim wrote:
> > Is there anything preventing the use of headers right now? If possible, it
> would
> > be nice not to have a temporary change for IPC.
> 
> We do not have any special header in the responses coming from Data
> saver proxy (unlike the requests which have "chrome-Proxy" header).
> Not sure if we want to add a new custom header for this experiment
> while it is in Beta.
> That might also require significant changes on the server side.
> WDYT?

My preference would be for the change to be on the server side rather than
putting temporary code into Chromium. Once code is out there, it can't be
revoked.

[tbansal1, 2015-05-12 19:41:51]

On 2015/05/12 18:54:30, jeremyim wrote:
> On 2015/05/12 05:04:12, tbansal1 wrote:
> > On 2015/05/12 03:36:50, jeremyim wrote:
> > > Is there anything preventing the use of headers right now? If possible, it
> > would
> > > be nice not to have a temporary change for IPC.
> > 
> > We do not have any special header in the responses coming from Data
> > saver proxy (unlike the requests which have "chrome-Proxy" header).
> > Not sure if we want to add a new custom header for this experiment
> > while it is in Beta.
> > That might also require significant changes on the server side.
> > WDYT?
> 
> My preference would be for the change to be on the server side rather than
> putting temporary code into Chromium. Once code is out there, it can't be
> revoked.

As discussed offline, this is not necessary for now.

[tbansal1, 2015-05-12 19:42:09]

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

[tbansal1, 2015-05-12 19:44:43]

tbansal@chromium.org changed reviewers:
+ asvitkine@google.com, tonyg@chromium.org

[tbansal1, 2015-05-12 19:45:26]

tbansal@chromium.org changed reviewers:
+ asvitkine@chromium.org
- asvitkine@google.com

[tbansal1, 2015-05-12 19:45:27]

asvitkine@chromium.org: Please review changes in 
histograms.xml

tonyg: page_load_histograms.cc

[Tom Sepez, 2015-05-12 19:46:20]

Messages themselves LGTM after fixing nits.

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(right):

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:20:
data_reduction_proxy::
nit: Don't break at the ::, comment on next line as needed.

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

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:89:
};
nit: generally, the technique here is to add

AUTO_LOFI_STATUS_LAST = AUTO_LOFI_STATUS_ON

and use that in the IPC message file.  That way, you can add an enum without
having to muck with the other file.

[jeremyim, 2015-05-12 19:46:21]

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc
(right):

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:49:
AutoLoFiStatus* lofi_response) {
nit: lofi_status

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h
(right):

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:32:
AutoLoFiStatus* lofi_response);
nit: lofi_status

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

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:78:
enum AutoLoFiStatus {
I think the enum should be in a separate file which has the IPC security
reviewers as the noparent OWNER.

[Alexei Svitkine (slow), 2015-05-12 20:46:54]

What's the reason for client-splitting these histograms as opposed to launching
LoFi as a field trial, which gives you automatic histogram splitting without the
need for extra client code?

[tbansal1, 2015-05-12 20:55:04]

On 2015/05/12 20:46:54, Alexei Svitkine wrote:
> What's the reason for client-splitting these histograms as opposed to
launching
> LoFi as a field trial, which gives you automatic histogram splitting without
the
> need for extra client code?

Reason is that Auto LoFi status keeps changing (on a page load basis)
depending on the network conditions (e.g., enabled only when network
quality is bad). Just because session is in a finch trial does 
not guarantee that LoFi would ever be used. 

Using a synthetic trial or only using the finch trial will give very
noisy data in this case.

[Alexei Svitkine (slow), 2015-05-12 21:51:13]

That makes sense, might be worth calling out specifically in the CL description
(you do mention something similar there, but not as clear the explanation you
just gave).

[tbansal1, 2015-05-12 21:54:19]

On 2015/05/12 21:51:13, Alexei Svitkine wrote:
> That makes sense, might be worth calling out specifically in the CL
description
> (you do mention something similar there, but not as clear the explanation you
> just gave).

done :)

[Alexei Svitkine (slow), 2015-05-12 21:55:09]

lgtm % nits

https://codereview.chromium.org/1140443002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1140443002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:66053: +  <suffix
name="HTTPS_DataReductionProxy_AutoLoFiOn"
Nit: Order the two HTTPS ones together?

https://codereview.chromium.org/1140443002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:66059: +  <suffix
name="HTTPS_DataReductionProxy_Auto LoFiOff"
Seems you have an extra space here?

[tbansal1, 2015-05-12 22:12:42]

ptal

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc
(right):

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.cc:49:
AutoLoFiStatus* lofi_response) {
On 2015/05/12 19:46:21, jeremyim wrote:
> nit: lofi_status

Done.

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h
(right):

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/browser/data_reduction_proxy_message_filter.h:32:
AutoLoFiStatus* lofi_response);
On 2015/05/12 19:46:21, jeremyim wrote:
> nit: lofi_status

Done.

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
File
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h
(right):

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/content/common/data_reduction_proxy_messages.h:20:
data_reduction_proxy::
On 2015/05/12 19:46:20, Tom Sepez wrote:
> nit: Don't break at the ::, comment on next line as needed.

Done.

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

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:78:
enum AutoLoFiStatus {
On 2015/05/12 19:46:21, jeremyim wrote:
> I think the enum should be in a separate file which has the IPC security
> reviewers as the noparent OWNER.

It seems that there are lot of places where enum is not in a seaparate
file. e.g. Material
(https://code.google.com/p/chromium/codesearch#chromium/src/content/common/cc_...)
If it is not necessary, then it is probably better to keep enum in DRP code for
ease of ownership.

https://codereview.chromium.org/1140443002/diff/60001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_config.h:89:
};
On 2015/05/12 19:46:20, Tom Sepez wrote:
> nit: generally, the technique here is to add
> 
> AUTO_LOFI_STATUS_LAST = AUTO_LOFI_STATUS_ON
> 
> and use that in the IPC message file.  That way, you can add an enum without
> having to muck with the other file.

Done.

https://codereview.chromium.org/1140443002/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1140443002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:66053: +  <suffix
name="HTTPS_DataReductionProxy_AutoLoFiOn"
On 2015/05/12 21:55:09, Alexei Svitkine wrote:
> Nit: Order the two HTTPS ones together?

Done.

https://codereview.chromium.org/1140443002/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:66059: +  <suffix
name="HTTPS_DataReductionProxy_Auto LoFiOff"
On 2015/05/12 21:55:09, Alexei Svitkine wrote:
> Seems you have an extra space here?

Done.

[jeremyim, 2015-05-13 00:16:53]

data_reduction_proxy* lgtm

[tbansal1, 2015-05-13 20:04:01]

tonyg@: ping for chrome/renderer/page_load_histograms.cc
thanks!

[tbansal1, 2015-05-13 20:04:01]

tonyg@: ping for chrome/renderer/page_load_histograms.cc
thanks!

[tonyg, 2015-05-13 22:13:51]

On 2015/05/13 20:04:01, tbansal1 wrote:
> tonyg@: ping for chrome/renderer/page_load_histograms.cc
> thanks!

rubber stamp lgtm

[tbansal1, 2015-05-13 22:56:54]

jeremyim@: Mind taking a quick look at chrome/renderer/page_load_histograms.cc?
thanks!

[jeremyim, 2015-05-13 23:12:09]

page_load_histograms lgtm % nit

https://codereview.chromium.org/1140443002/diff/100001/chrome/renderer/page_l...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/1140443002/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_histograms.cc:826: enum
data_reduction_proxy::AutoLoFiStatus auto_lofi_status =
Remove enum keyword.

[tbansal1, 2015-05-13 23:51:39]

https://codereview.chromium.org/1140443002/diff/100001/chrome/renderer/page_l...
File chrome/renderer/page_load_histograms.cc (right):

https://codereview.chromium.org/1140443002/diff/100001/chrome/renderer/page_l...
chrome/renderer/page_load_histograms.cc:826: enum
data_reduction_proxy::AutoLoFiStatus auto_lofi_status =
On 2015/05/13 23:12:08, jeremyim wrote:
> Remove enum keyword.

Done.

[tbansal1, 2015-05-13 23:51:45]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2015-05-13 23:51:45]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
asvitkine@chromium.org, jeremyim@chromium.org, tonyg@chromium.org
Link to the patchset: https://codereview.chromium.org/1140443002/#ps120001
(title: "Removed enum keyword")

[commit-bot: I haz the power, 2015-05-13 23:53:46]

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

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

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

[commit-bot: I haz the power, 2015-05-14 00:10:59]

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

[tbansal1, 2015-05-14 00:27:46]

The CQ bit was checked by tbansal@chromium.org

[tbansal1, 2015-05-14 00:27:46]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
jeremyim@chromium.org, tonyg@chromium.org, asvitkine@chromium.org
Link to the patchset: https://codereview.chromium.org/1140443002/#ps140001
(title: "Fixed minor typo in test")

[commit-bot: I haz the power, 2015-05-14 00:28:41]

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

[commit-bot: I haz the power, 2015-05-14 01:44:21]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2015-05-14 01:44:59]

Patchset 8 (id:??) landed as
https://crrev.com/9b2675127847bbdecbbd630ee4c13fec33e67f4b
Cr-Commit-Position: refs/heads/master@{#329776}