Adding a data saver page id to requests and reports

Adding a data saver page id to requests and reports

This adds a page id determined from GlobalRequestID to main frame data
saver requests as well as PageLoad Metrics pingback reports.

BUG=701579
Review-Url: https://codereview.chromium.org/2803813003
Cr-Commit-Position: refs/heads/master@{#463470}
Committed: https://chromium.googlesource.com/chromium/src/+/dba8a17e64a4d6bc35e941b2b2a46003c25c5505

[RyanSturm, 2017-04-05 21:37:08]

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

[commit-bot: I haz the power, 2017-04-05 21:38:06]

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-05 22:12:05]

ryansturm@chromium.org changed reviewers:
+ bengr@chromium.org, buettner@chromium.org, jpfeiff@chromium.org

[RyanSturm, 2017-04-05 22:12:05]

bengr, buettner, jpfeiff: PTAL. The follow-up CL will add opt out (and maybe
previews type).

[commit-bot: I haz the power, 2017-04-05 22:51:32]

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

[commit-bot: I haz the power, 2017-04-05 22:51:32]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-04-05 23:14:32]

On 2017/04/05 22:12:05, Ryan Sturm wrote:
> bengr, buettner, jpfeiff: PTAL. The follow-up CL will add opt out (and maybe
> previews type).

As a note, this can't land until the server supports the "pid" directive.

[bengr, 2017-04-07 16:27:25]

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:19:
DataReductionProxyData::~DataReductionProxyData() {}
What goes wrong if you don't override this destructor?

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:65: //
The data saver ID that uniquely identifies the page load. Only present on
Be clearer in the comment, e.g., something like:
// An identifier that is guaranteed to be unique to each page load during a
browsing session.


Also, what happens if we try to set this on a subresource request?

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:106: //
The data saver ID that uniquely identifies the page load. Only present on
Update this comment to match your changes to line 65.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:57:
uint64_t page_id = 1;
#include <stdint.h>

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:230:
if (data) {
What happens if the renderer cancels the main frame request and issues a new
one? Does the id need to propagate in that case as well? And if not, why?

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:305:
if (request->load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) {
We need to move away from LOAD_MAIN_FRAME_DEPRECATED. Please file a bug.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:572:
int response_body_size = 140;
Where did 140 come from?

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:607:
std::string mock_write = prefix_headers + suffix_headers;
Why did you split this up in the prefix and suffix? Why not use StringPrintf?

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1455:
Init(true /* use_secure_proxy */, false /* enable_brotli_globally */);
Add a comment to make it clear (or add test coverage) that brotli or an insecure
proxy won't affect the result. Likewise for the following tests.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:176:
header_value += ", " + FormatOption(kPageIdOption, page_id_buffer);
Probably overkill, but it might be easier long term to create a list of values
and join them.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:316:
return ++current_page_id_;
Add a comment here that a caller should not depend on the ordering.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:116:
void ResetPageId();
This would seem less brittle if a Flywheel session object owned the page id.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:164:
// The page identifier that is sent and incremented whenever data saver sends
I wouldn't describe the mechanism by which IDs are generated here. I.e., get rid
of the word "incremented."

[RyanSturm, 2017-04-07 21:22:22]

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

[commit-bot: I haz the power, 2017-04-07 21:22:54]

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-07 21:34:19]

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

[commit-bot: I haz the power, 2017-04-07 21:34:19]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, build
hasn't started yet, builder probably lacks capacity)

[RyanSturm, 2017-04-07 21:42:26]

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

[commit-bot: I haz the power, 2017-04-07 21:43:15]

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-07 22:15:04]

Thanks for the feedback. I opened a few bugs.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:19:
DataReductionProxyData::~DataReductionProxyData() {}
On 2017/04/07 16:27:24, bengr wrote:
> What goes wrong if you don't override this destructor?

Complex object  chromium compiler warning.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:65: //
The data saver ID that uniquely identifies the page load. Only present on
On 2017/04/07 16:27:24, bengr wrote:
> Be clearer in the comment, e.g., something like:
> // An identifier that is guaranteed to be unique to each page load during a
> browsing session.
Done.
> 
> 
> Also, what happens if we try to set this on a subresource request?

I have a plan, but it is much more complex than this CL. It involves storing one
of these objects (copied from the main frame complete request) on
ChromeDataUseAscriber for the page load and adding the id if the session hasn't
changed. etc.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:106: //
The data saver ID that uniquely identifies the page load. Only present on
On 2017/04/07 16:27:24, bengr wrote:
> Update this comment to match your changes to line 65.

Done.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data_unittest.cc:57:
uint64_t page_id = 1;
On 2017/04/07 16:27:24, bengr wrote:
> #include <stdint.h>

Done.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:230:
if (data) {
On 2017/04/07 16:27:24, bengr wrote:
> What happens if the renderer cancels the main frame request and issues a new
> one? Does the id need to propagate in that case as well? And if not, why?

No. Unfortunately, any renderer redirects (javascript) etc., aren't tracked, but
that is the way it works for tab observers I believe as well (new callbacks for
that navigation).

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:305:
if (request->load_flags() & net::LOAD_MAIN_FRAME_DEPRECATED) {
On 2017/04/07 16:27:24, bengr wrote:
> We need to move away from LOAD_MAIN_FRAME_DEPRECATED. Please file a bug.

crbug.com/709621

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:572:
int response_body_size = 140;
On 2017/04/07 16:27:24, bengr wrote:
> Where did 140 come from? 

Copying. Just needed a size.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:607:
std::string mock_write = prefix_headers + suffix_headers;
On 2017/04/07 16:27:24, bengr wrote:
> Why did you split this up in the prefix and suffix? Why not use StringPrintf?

I just used "+" shorthand. I think it's more readable (albeit probably slightly
less efficient depending on compiler, but this is test code).

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate_unittest.cc:1455:
Init(true /* use_secure_proxy */, false /* enable_brotli_globally */);
On 2017/04/07 16:27:24, bengr wrote:
> Add a comment to make it clear (or add test coverage) that brotli or an
insecure
> proxy won't affect the result. Likewise for the following tests.

Done.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:176:
header_value += ", " + FormatOption(kPageIdOption, page_id_buffer);
On 2017/04/07 16:27:24, bengr wrote:
> Probably overkill, but it might be easier long term to create a list of values
> and join them.

Yea. I think this is a little better although ugly. It joins all the persistent
headers and stores that string to be copied as a whole then appends this. I'm
not going to open a bug because I think we need to rework all of this with the
other session bug.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.cc:316:
return ++current_page_id_;
On 2017/04/07 16:27:24, bengr wrote:
> Add a comment here that a caller should not depend on the ordering.

Done.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:116:
void ResetPageId();
On 2017/04/07 16:27:24, bengr wrote:
> This would seem less brittle if a Flywheel session object owned the page id.

Filed a bug and assigned it to me.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:164:
// The page identifier that is sent and incremented whenever data saver sends
On 2017/04/07 16:27:25, bengr wrote:
> I wouldn't describe the mechanism by which IDs are generated here. I.e., get
rid
> of the word "incremented."

Done.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:316:
void DataReductionProxyNetworkDelegate::OnBeforeRedirectInternal(
Apparently the net full stack tests don't hit this path properly. I added a
seperate direct test.

[commit-bot: I haz the power, 2017-04-07 22:30:59]

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

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

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-04-10 16:27:03]

bengr: PTAL

[bengr, 2017-04-10 20:40:09]

lgtm, with nits on comments. Please address before submitting.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.cc:19:
DataReductionProxyData::~DataReductionProxyData() {}
On 2017/04/07 22:15:03, Ryan Sturm wrote:
> On 2017/04/07 16:27:24, bengr wrote:
> > What goes wrong if you don't override this destructor?
> 
> Complex object  chromium compiler warning.

Acknowledged.

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2803813003/diff/1/components/data_reduction_p...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:230:
if (data) {
On 2017/04/07 22:15:03, Ryan Sturm wrote:
> On 2017/04/07 16:27:24, bengr wrote:
> > What happens if the renderer cancels the main frame request and issues a new
> > one? Does the id need to propagate in that case as well? And if not, why?
> 
> No. Unfortunately, any renderer redirects (javascript) etc., aren't tracked,
but
> that is the way it works for tab observers I believe as well (new callbacks
for
> that navigation).

Consider adding a comment that makes my observation explicit. I.e., that
redirects or cancellations/reissues don't carry over IDs.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:66: //
browsing session. Only present on main frame requests.
Is it a browsing session or a flywheel session? sorry.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:107: //
browsing session. Only present on main frame requests.
Ditto.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:316:
void DataReductionProxyNetworkDelegate::OnBeforeRedirectInternal(
On 2017/04/07 22:15:03, Ryan Sturm wrote:
> Apparently the net full stack tests don't hit this path properly. I added a
> seperate direct test.

Acknowledged.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:116:
// TODO(ryansturm): create a session object to store this and other session
create -> Create
other session -> other data reduction proxy session

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:166:
// The page identifier was last generated for data saver proxy server.
was -> that was

[RyanSturm, 2017-04-10 23:38:06]

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

[commit-bot: I haz the power, 2017-04-10 23:39:03]

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-11 00:31:49]

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

[commit-bot: I haz the power, 2017-04-11 00:31:51]

Dry run: This issue passed the CQ dry run.

[RyanSturm, 2017-04-11 00:33:48]

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
File components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h
(right):

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:66: //
browsing session. Only present on main frame requests.
On 2017/04/10 20:40:08, bengr wrote:
> Is it a browsing session or a flywheel session? sorry.

Done.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_data.h:107: //
browsing session. Only present on main frame requests.
On 2017/04/10 20:40:08, bengr wrote:
> Ditto.

Done.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc
(right):

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_network_delegate.cc:316:
void DataReductionProxyNetworkDelegate::OnBeforeRedirectInternal(
On 2017/04/10 20:40:08, bengr wrote:
> On 2017/04/07 22:15:03, Ryan Sturm wrote:
> > Apparently the net full stack tests don't hit this path properly. I added a
> > seperate direct test.
> 
> Acknowledged.

Done.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
File
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h
(right):

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:116:
// TODO(ryansturm): create a session object to store this and other session
On 2017/04/10 20:40:09, bengr wrote:
> create -> Create
> other session -> other data reduction proxy session

Done.

https://codereview.chromium.org/2803813003/diff/40001/components/data_reducti...
components/data_reduction_proxy/core/browser/data_reduction_proxy_request_options.h:166:
// The page identifier was last generated for data saver proxy server.
On 2017/04/10 20:40:09, bengr wrote:
> was -> that was

Done.

[RyanSturm, 2017-04-11 00:33:57]

The CQ bit was checked by ryansturm@chromium.org

[RyanSturm, 2017-04-11 00:33:58]

The patchset sent to the CQ was uploaded after l-g-t-m from bengr@chromium.org
Link to the patchset: https://codereview.chromium.org/2803813003/#ps60001
(title: "bengr nits")

[commit-bot: I haz the power, 2017-04-11 00:34:54]

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-11 00:44:03]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1491870837882210,
"parent_rev": "6b0f538afa82922e2abb7880418fb75320275b26", "commit_rev":
"dba8a17e64a4d6bc35e941b2b2a46003c25c5505"}

[commit-bot: I haz the power, 2017-04-11 00:44:54]

Description was changed from

==========
Adding a data saver page id to requests and reports

This adds a page id determined from GlobalRequestID to main frame data
saver requests as well as PageLoad Metrics pingback reports.

BUG=701579
==========

to

==========
Adding a data saver page id to requests and reports

This adds a page id determined from GlobalRequestID to main frame data
saver requests as well as PageLoad Metrics pingback reports.

BUG=701579

Review-Url: https://codereview.chromium.org/2803813003
Cr-Commit-Position: refs/heads/master@{#463470}
Committed:
https://chromium.googlesource.com/chromium/src/+/dba8a17e64a4d6bc35e941b2b2a4...
==========

[commit-bot: I haz the power, 2017-04-11 00:44:56]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/dba8a17e64a4d6bc35e941b2b2a4...