Adding opt out and previews type information to DRP pingback

Adding opt out and previews type information to DRP pingback

This CL adds the previews type that was shown for a page load and also
reports when a page load was an opt out (clicked show original). Due to
backgrounding behavior, when app background occurs or there is not a
previews shown a third opt out state of unknown is reported. LoFi page
state is determined if any subresources were LoFi images. Lite page uses
the main resource to determine preview state.

BUG=713236
Review-Url: https://codereview.chromium.org/2833523002
Cr-Commit-Position: refs/heads/master@{#466376}
Committed: https://chromium.googlesource.com/chromium/src/+/6ffce94e929ce46e50e364b726e07e2b4773c82b

[RyanSturm, 2017-04-19 16:55:11]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 16:55:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 16:59:33]

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

[commit-bot: I haz the power, 2017-04-19 16:59:34]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[RyanSturm, 2017-04-19 17:44:35]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 17:45:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 18:10:54]

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

[commit-bot: I haz the power, 2017-04-19 18:10:56]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[RyanSturm, 2017-04-19 18:14:51]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 18:15:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-04-19 18:21:35]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 18:21:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 18:51:57]

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

[commit-bot: I haz the power, 2017-04-19 18:51:58]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[RyanSturm, 2017-04-19 19:00:30]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 19:00:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-04-19 19:43:13]

ryansturm@chromium.org changed reviewers:
+ bengr@chromium.org, csharrison@chromium.org

[RyanSturm, 2017-04-19 19:43:14]

csharrison, bengr: PTAL

[commit-bot: I haz the power, 2017-04-19 19:45:36]

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

[commit-bot: I haz the power, 2017-04-19 19:45:37]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-04-19 21:02:33]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 21:03:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 21:49:58]

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

[commit-bot: I haz the power, 2017-04-19 21:49:59]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-04-19 22:30:44]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-19 22:31:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-19 22:38:10]

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

[commit-bot: I haz the power, 2017-04-19 22:38:14]

Dry run: This issue passed the CQ dry run.

[bengr, 2017-04-20 01:02:33]

I skimmed quickly. Seems reasonable. I'll give it a careful read soon.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:114:
bool app_background_occured);
spelling.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:151:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
I'm assuming this never gets run from incognito?

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:397:
const net::HttpResponseHeaders* original_response_headers,
Is this guaranteed to be non-null?

[Charlie Harrison, 2017-04-20 14:09:00]

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152:
tab_identifier_key_ = navigation_handle->GetWebContents();
This makes me really nervous. Is there any evidence that this is a unique key?
Maybe you can do something with the render view routing id of the main frame +
process id?

[RyanSturm, 2017-04-20 15:37:58]

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152:
tab_identifier_key_ = navigation_handle->GetWebContents();
On 2017/04/20 14:09:00, Charlie Harrison wrote:
> This makes me really nervous. Is there any evidence that this is a unique key?
> Maybe you can do something with the render view routing id of the main frame +
> process id?

I agree this is a little odd. How about I rework this to use session_key (server
generated random key) and page_id, which is assigned to main frame requests
uniquely. I can clearkeyasync call from didfinishnavigate as well as the
destructor for the previously committed pair of IDs. That will remove any sense
of tab from the pingback client. It seems more robust as well. Both of these are
already in drpdata FYI. Wdyt?

[Charlie Harrison, 2017-04-20 16:35:06]

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152:
tab_identifier_key_ = navigation_handle->GetWebContents();
On 2017/04/20 15:37:57, Ryan Sturm wrote:
> On 2017/04/20 14:09:00, Charlie Harrison wrote:
> > This makes me really nervous. Is there any evidence that this is a unique
key?
> > Maybe you can do something with the render view routing id of the main frame
+
> > process id?
> 
> I agree this is a little odd. How about I rework this to use session_key
(server
> generated random key) and page_id, which is assigned to main frame requests
> uniquely. I can clearkeyasync call from didfinishnavigate as well as the
> destructor for the previously committed pair of IDs. That will remove any
sense
> of tab from the pingback client. It seems more robust as well. Both of these
are
> already in drpdata FYI. Wdyt?

This approach sounds much better, though I'm not sure what you mean by
clearkeyasync?

[bengr, 2017-04-20 17:36:19]

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:355: bool
was_lofi_reponse,
What is a lofi response? Does this bit get set on client-side lofi range
responses? Also, this seems like a special case of used_d_r_p. Should that be an
enum instead? Or a struct? Copy and plumb drp_data all the way through?

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:416:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
Nit: Remove the extra space in the comments here and below.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:420:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
Why are there no core page load metrics observer tests that cover
was_lofi_response == true?

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152:
tab_identifier_key_ = navigation_handle->GetWebContents();
On 2017/04/20 16:35:06, Charlie Harrison wrote:
> On 2017/04/20 15:37:57, Ryan Sturm wrote:
> > On 2017/04/20 14:09:00, Charlie Harrison wrote:
> > > This makes me really nervous. Is there any evidence that this is a unique
> key?
> > > Maybe you can do something with the render view routing id of the main
frame
> +
> > > process id?
> > 
> > I agree this is a little odd. How about I rework this to use session_key
> (server
> > generated random key) and page_id, which is assigned to main frame requests
> > uniquely. I can clearkeyasync call from didfinishnavigate as well as the
> > destructor for the previously committed pair of IDs. That will remove any
> sense
> > of tab from the pingback client. It seems more robust as well. Both of these
> are
> > already in drpdata FYI. Wdyt?
> 
> This approach sounds much better, though I'm not sure what you mean by
> clearkeyasync?

Likewise, I'm not sure either.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:277:
bool app_background_occured) {
occurred!

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:144:
// A key that relates to the tab.
relates how? uniquely identifies? why does the observer need tab identification?

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:67:
tab_identifier_key_ = tab_identifier_key;
Which cases will have no tab identifier key? Document somewhere, and is that ok?

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:456:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
you have two spaces after /*

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc:59:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
two spaces.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:260:
false /* was_lofi_response */,
Why no test coverage for was_lofi_response == true?

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:74:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
two spaces.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:78:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
why no tests for was_lofi_response == true?

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_tab_helper.h (right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_tab_helper.h:34: const
base::Optional<uint64_t>& committed_data_saver_page_id() {
#include <stdint.h>

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:77:
const base::Optional<uint64_t>& page_id() const { return page_id_; }
#include <stdint.h>

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:58:
EXPECT_EQ(std::string(), data->session_key());
#include <string>

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:52:
// Adds an opt out for |tab_identifier_key| for a data saver |page_id|.
What's an opt out? Define somewhere.

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:61:
// True if app background forced tracking to stop for the page load.
I can't quite parse this comment.

[RyanSturm, 2017-04-20 20:25:37]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[RyanSturm, 2017-04-20 20:25:46]

Addressed all comments (a lot were obsolete due to changing keys)

jpfeiff: PTAL at pageload_metrics.proto

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:355: bool
was_lofi_reponse,
On 2017/04/20 17:36:18, bengr wrote:
> What is a lofi response? Does this bit get set on client-side lofi range
> responses? Also, this seems like a special case of used_d_r_p. Should that be
an
> enum instead? Or a struct? Copy and plumb drp_data all the way through? 

Obsolete. Changed to DRPData.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:416:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
On 2017/04/20 17:36:18, bengr wrote:
> Nit: Remove the extra space in the comments here and below.

Acknowledged.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:420:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
On 2017/04/20 17:36:18, bengr wrote:
> Why are there no core page load metrics observer tests that cover
> was_lofi_response == true?

These are unused in most observers. I switched to DRPData, let me know if you
want me to run a few extra requests for DRPData.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:152:
tab_identifier_key_ = navigation_handle->GetWebContents();
On 2017/04/20 17:36:18, bengr wrote:
> On 2017/04/20 16:35:06, Charlie Harrison wrote:
> > On 2017/04/20 15:37:57, Ryan Sturm wrote:
> > > On 2017/04/20 14:09:00, Charlie Harrison wrote:
> > > > This makes me really nervous. Is there any evidence that this is a
unique
> > key?
> > > > Maybe you can do something with the render view routing id of the main
> frame
> > +
> > > > process id?
> > > 
> > > I agree this is a little odd. How about I rework this to use session_key
> > (server
> > > generated random key) and page_id, which is assigned to main frame
requests
> > > uniquely. I can clearkeyasync call from didfinishnavigate as well as the
> > > destructor for the previously committed pair of IDs. That will remove any
> > sense
> > > of tab from the pingback client. It seems more robust as well. Both of
these
> > are
> > > already in drpdata FYI. Wdyt?
> > 
> > This approach sounds much better, though I'm not sure what you mean by
> > clearkeyasync?
> 
> Likewise, I'm not sure either.

Done.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:277:
bool app_background_occured) {
On 2017/04/20 17:36:18, bengr wrote:
> occurred!

Done.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:114:
bool app_background_occured);
On 2017/04/20 01:02:33, bengr wrote:
> spelling.

Done.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.h:144:
// A key that relates to the tab.
On 2017/04/20 17:36:18, bengr wrote:
> relates how? uniquely identifies? why does the observer need tab
identification?

Acknowledged.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:67:
tab_identifier_key_ = tab_identifier_key;
On 2017/04/20 17:36:18, bengr wrote:
> Which cases will have no tab identifier key? Document somewhere, and is that
ok?

Acknowledged.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer_unittest.cc:456:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
On 2017/04/20 17:36:18, bengr wrote:
> you have two spaces after /*

Acknowledged.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/media_page_load_metrics_observer_unittest.cc:59:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
On 2017/04/20 17:36:18, bengr wrote:
> two spaces.

Acknowledged.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/subresource_filter_metrics_observer_unittest.cc:260:
false /* was_lofi_response */,
On 2017/04/20 17:36:18, bengr wrote:
> Why no test coverage for was_lofi_response == true?

Irrelevant and unused for this observer.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:74:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
On 2017/04/20 17:36:18, bengr wrote:
> two spaces.

Acknowledged.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/tab_restore_page_load_metrics_observer_unittest.cc:78:
false /* data_reduction_proxy_used*/, false /*  was_lofi_response */,
On 2017/04/20 17:36:18, bengr wrote:
> why no tests for was_lofi_response == true?

See other response.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_delegate.cc:151:
DataReductionProxyChromeSettingsFactory::GetForBrowserContext(
On 2017/04/20 01:02:33, bengr wrote:
> I'm assuming this never gets run from incognito?

It should not; it would be an interesting crash to investigate. Note that this
is just moved up from below.

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
File chrome/browser/previews/previews_infobar_tab_helper.h (right):

https://codereview.chromium.org/2833523002/diff/100001/chrome/browser/preview...
chrome/browser/previews/previews_infobar_tab_helper.h:34: const
base::Optional<uint64_t>& committed_data_saver_page_id() {
On 2017/04/20 17:36:18, bengr wrote:
> #include <stdint.h>

Acknowledged.

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:77:
const base::Optional<uint64_t>& page_id() const { return page_id_; }
On 2017/04/20 17:36:18, bengr wrote:
> #include <stdint.h>

Done.

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:58:
EXPECT_EQ(std::string(), data->session_key());
On 2017/04/20 17:36:19, bengr wrote:
> #include <string>

Done.

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:397:
const net::HttpResponseHeaders* original_response_headers,
On 2017/04/20 01:02:33, bengr wrote:
> Is this guaranteed to be non-null?

I can't find any guarantees. The fact that it is a const* makes me want to null
check it. I added an early return instead of multiple checks.

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/browser/data_reduction_proxy_pingback_client.h:52:
// Adds an opt out for |tab_identifier_key| for a data saver |page_id|.
On 2017/04/20 17:36:19, bengr wrote:
> What's an opt out? Define somewhere.

Done.

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
File
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h
(right):

https://codereview.chromium.org/2833523002/diff/100001/components/data_reduct...
components/data_reduction_proxy/core/common/data_reduction_proxy_page_load_timing.h:61:
// True if app background forced tracking to stop for the page load.
On 2017/04/20 17:36:19, bengr wrote:
> I can't quite parse this comment.

Done.

[RyanSturm, 2017-04-20 20:25:58]

Description was changed from

==========
Adding opt out and previews type information to DRP pingback

This CL adds the previews type that was shown for a page load and also
reports when a page load was an opt out (clicked show original). Due to
backgrounding behavior, when app background occurs or there is not a
previews shown a third opt out state of unknown is reported. LoFi page
state is determined if any subresources were LoFi images. Lite page uses
the main resource to determine preview state.

BUG=713236
==========

to

==========
Adding opt out and previews type information to DRP pingback

This CL adds the previews type that was shown for a page load and also
reports when a page load was an opt out (clicked show original). Due to
backgrounding behavior, when app background occurs or there is not a
previews shown a third opt out state of unknown is reported. LoFi page
state is determined if any subresources were LoFi images. Lite page uses
the main resource to determine preview state.

BUG=713236
==========

[RyanSturm, 2017-04-20 20:25:59]

ryansturm@chromium.org changed reviewers:
+ jpfeiff@chromium.org

[commit-bot: I haz the power, 2017-04-20 20:26:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-04-20 20:30:32]

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (left):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:205:
ExtraRequestInfo(const ExtraRequestInfo& other);
csharrison@, I removed this, but I can add it back and leave it as = default; I
just need to add a copy constructor for DataReductionProxyData, but it seemed
like there wasn't a good reason to have a copy constructor as far as I could
tell.

[commit-bot: I haz the power, 2017-04-20 20:31:42]

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

[commit-bot: I haz the power, 2017-04-20 20:31:43]

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Charlie Harrison, 2017-04-20 20:52:50]

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:850: if (data)
Can you comment when this would be null? If this is non-null if a large % of
requests then the Deep Copy is concerning to me, due to the GURL / string copy.

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.cc (right):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.cc:66:
data_reduction_proxy_data(std::move(data_reduction_proxy_data)) {}
#include <utility> for std::move

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (left):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:205:
ExtraRequestInfo(const ExtraRequestInfo& other);
On 2017/04/20 20:30:32, Ryan Sturm wrote:
> csharrison@, I removed this, but I can add it back and leave it as = default;
I
> just need to add a copy constructor for DataReductionProxyData, but it seemed
> like there wasn't a good reason to have a copy constructor as far as I could
> tell.

Fine by me. I think we only use it for for loops :P

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:204:
std::unique_ptr<data_reduction_proxy::DataReductionProxyData>
#include <memory>

[RyanSturm, 2017-04-20 21:00:08]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:00:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jpfeiff, 2017-04-20 21:14:43]

On 2017/04/20 20:25:46, Ryan Sturm wrote:
> Addressed all comments (a lot were obsolete due to changing keys)
> 
> jpfeiff: PTAL at pageload_metrics.proto
pageload_metrics.proto LGTM

[RyanSturm, 2017-04-20 21:24:51]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:25:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-04-20 21:27:14]

Patchset #9 (id:160001) has been deleted

[RyanSturm, 2017-04-20 21:29:22]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:29:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[RyanSturm, 2017-04-20 21:32:04]

Patchset #9 (id:180001) has been deleted

[RyanSturm, 2017-04-20 21:33:01]

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:850: if (data)
On 2017/04/20 20:52:50, Charlie Harrison wrote:
> Can you comment when this would be null? If this is non-null if a large % of
> requests then the Deep Copy is concerning to me, due to the GURL / string
copy.

I just cut this way down by only adding session key and the URL for main frame
requests in the network_delegate code. It should be ~20-30% of main frame
requests on android (every DRP main frame request) and a low percent on other
platforms.

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.cc (right):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.cc:66:
data_reduction_proxy_data(std::move(data_reduction_proxy_data)) {}
On 2017/04/20 20:52:50, Charlie Harrison wrote:
> #include <utility> for std::move

Done.

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
File chrome/browser/page_load_metrics/page_load_metrics_observer.h (right):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/page_load_metrics_observer.h:204:
std::unique_ptr<data_reduction_proxy::DataReductionProxyData>
On 2017/04/20 20:52:50, Charlie Harrison wrote:
> #include <memory>

Done.

[RyanSturm, 2017-04-20 21:34:46]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-20 21:35:25]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Harrison, 2017-04-20 22:00:52]

page_load_metrics and c/b/l LGTM, but I didn't look at anything else

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/120001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:850: if (data)
On 2017/04/20 21:33:00, Ryan Sturm wrote:
> On 2017/04/20 20:52:50, Charlie Harrison wrote:
> > Can you comment when this would be null? If this is non-null if a large % of
> > requests then the Deep Copy is concerning to me, due to the GURL / string
> copy.
> 
> I just cut this way down by only adding session key and the URL for main frame
> requests in the network_delegate code. It should be ~20-30% of main frame
> requests on android (every DRP main frame request) and a low percent on other
> platforms.

Ok that's fine by me.

[commit-bot: I haz the power, 2017-04-20 23:09:40]

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

[commit-bot: I haz the power, 2017-04-20 23:09:41]

Dry run: This issue passed the CQ dry run.

[bengr, 2017-04-20 23:19:36]

lgtm

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
data_reduction_proxy_data = data->DeepCopy();
It's too bad you have to pass along the session key, which isn't used by this
code.

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:421:
0 /* original_network_content_length */, nullptr},
Perhaps comment everywhere that this is d_r_p_data.

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:177:
SendPingback(timing, info, true /* app_background_occured */);
Spelling.

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:186:
SendPingback(timing, info, false /* app_background_occured */);
Spelling.

[RyanSturm, 2017-04-21 16:24:52]

The CQ bit was checked by ryansturm@chromium.org to run a CQ dry run

[RyanSturm, 2017-04-21 16:25:04]

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/...
File chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc (right):

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/loader/...
chrome/browser/loader/chrome_resource_dispatcher_host_delegate.cc:831:
data_reduction_proxy_data = data->DeepCopy();
On 2017/04/20 23:19:35, bengr wrote:
> It's too bad you have to pass along the session key, which isn't used by this
> code.

It's only for main frame requests, but still unfortunate. The main frame request
is also copied elsewhere (but all fields are used for those copies).

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc
(right):

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/core_page_load_metrics_observer_unittest.cc:421:
0 /* original_network_content_length */, nullptr},
On 2017/04/20 23:19:36, bengr wrote:
> Perhaps comment everywhere that this is d_r_p_data.

Done.

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
File
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc
(right):

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:177:
SendPingback(timing, info, true /* app_background_occured */);
On 2017/04/20 23:19:36, bengr wrote:
> Spelling.

Done.

https://codereview.chromium.org/2833523002/diff/200001/chrome/browser/page_lo...
chrome/browser/page_load_metrics/observers/data_reduction_proxy_metrics_observer.cc:186:
SendPingback(timing, info, false /* app_background_occured */);
On 2017/04/20 23:19:36, bengr wrote:
> Spelling.

Done.

[commit-bot: I haz the power, 2017-04-21 16:25:18]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 17:13:01]

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

[commit-bot: I haz the power, 2017-04-21 17:13:02]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-04-21 17:17:57]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-04-21 17:17:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
jpfeiff@chromium.org, bengr@chromium.org, csharrison@chromium.org
Link to the patchset: https://codereview.chromium.org/2833523002/#ps220001
(title: "bengr comments")

[commit-bot: I haz the power, 2017-04-21 17:18:25]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 17:25:39]

CQ is committing da patch.

Bot data: {"patchset_id": 220001, "attempt_start_ts": 1492795077681520,
"parent_rev": "71d80a8e613740b5661720ec1be8aa4b007d80ce", "commit_rev":
"6ffce94e929ce46e50e364b726e07e2b4773c82b"}

[commit-bot: I haz the power, 2017-04-21 17:25:51]

Description was changed from

==========
Adding opt out and previews type information to DRP pingback

This CL adds the previews type that was shown for a page load and also
reports when a page load was an opt out (clicked show original). Due to
backgrounding behavior, when app background occurs or there is not a
previews shown a third opt out state of unknown is reported. LoFi page
state is determined if any subresources were LoFi images. Lite page uses
the main resource to determine preview state.

BUG=713236
==========

to

==========
Adding opt out and previews type information to DRP pingback

This CL adds the previews type that was shown for a page load and also
reports when a page load was an opt out (clicked show original). Due to
backgrounding behavior, when app background occurs or there is not a
previews shown a third opt out state of unknown is reported. LoFi page
state is determined if any subresources were LoFi images. Lite page uses
the main resource to determine preview state.

BUG=713236

Review-Url: https://codereview.chromium.org/2833523002
Cr-Commit-Position: refs/heads/master@{#466376}
Committed:
https://chromium.googlesource.com/chromium/src/+/6ffce94e929ce46e50e364b726e0...
==========

[commit-bot: I haz the power, 2017-04-21 17:25:53]

Committed patchset #10 (id:220001) as
https://chromium.googlesource.com/chromium/src/+/6ffce94e929ce46e50e364b726e0...