save-data header value gets appended on page refreshes

save-data header value gets appended on page refreshes

When data-saver is enabled, Chrome sets save-data header value to 'on'.
When the webpage is refreshed, the sub resource requests get the
save-data value as 'on,on'.

BUG=591594
Committed: https://crrev.com/8c957da3f5a7b7e3eb37483a57ebbf323f997f1f
Cr-Commit-Position: refs/heads/master@{#380568}

[Raj, 2016-03-10 00:24:37]

rajendrant@chromium.org changed reviewers:
+ japhet@chromium.org, sclittle@chromium.org

[Raj, 2016-03-10 00:24:37]

japhet: PTAL FrameFetchContext.cpp
sclittle: PTAL data_saver_browsertest.cc

[Nate Chapin, 2016-03-10 00:33:14]

Code change is fine, but can you test this in FrameFetchContextTest.cpp instead?

[Raj, 2016-03-10 05:35:20]

Added test in FrameFetchContextTest.cpp
Wondering if page reload type of test is possible there.
PTAL

[Nate Chapin, 2016-03-10 19:04:03]

On 2016/03/10 05:35:20, Raj wrote:
> Added test in FrameFetchContextTest.cpp
> Wondering if page reload type of test is possible there.
> PTAL

Your test could manually create a 2nd FrameFetchContext and call
addAdditionalRequestHeaders() on that one. But there's nothing magical about the
fact that the page reloaded, the bug is when revalidating an existing Resource
regardless of the reason for the revalidation. If the resource had
Cache-Control: headers such that we sent a revalidation request from the same
page, you'd get the same bug.

[Nate Chapin, 2016-03-10 19:04:12]

lgtm

[sclittle, 2016-03-10 21:57:28]

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
File chrome/browser/data_saver/data_saver_browsertest.cc (right):

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:68: auto save_data_header =
request.headers.find("save-data");
nit: For clarity, could you rename |save_data_header| so that it's clear that
it's an iterator? e.g. save_data_header_it or something maybe. Right now it
looks like it would be the actual header string.

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:73: }
Should you also check that if |expected_save_data_header_| is empty, there's no
save-data header?

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:81:
IN_PROC_BROWSER_TEST_F(DataSaverWithServerBrowserTest,
Should there also be a test that the save-data header is not present if Data
Saver is turned off before reloading the page?

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:90: // Reload the webpage
and expect all the subresources will get the corect
nit: spelling of "correct"

[Raj, 2016-03-10 23:30:39]

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
File chrome/browser/data_saver/data_saver_browsertest.cc (right):

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:68: auto save_data_header =
request.headers.find("save-data");
On 2016/03/10 21:57:27, sclittle wrote:
> nit: For clarity, could you rename |save_data_header| so that it's clear that
> it's an iterator? e.g. save_data_header_it or something maybe. Right now it
> looks like it would be the actual header string.

Done.

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:73: }
On 2016/03/10 21:57:27, sclittle wrote:
> Should you also check that if |expected_save_data_header_| is empty, there's
no
> save-data header?

Done.

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:81:
IN_PROC_BROWSER_TEST_F(DataSaverWithServerBrowserTest,
On 2016/03/10 21:57:27, sclittle wrote:
> Should there also be a test that the save-data header is not present if Data
> Saver is turned off before reloading the page?

Done.

https://codereview.chromium.org/1783653002/diff/20001/chrome/browser/data_sav...
chrome/browser/data_saver/data_saver_browsertest.cc:90: // Reload the webpage
and expect all the subresources will get the corect
On 2016/03/10 21:57:27, sclittle wrote:
> nit: spelling of "correct"

Done.

[sclittle, 2016-03-10 23:34:20]

lgtm

[Raj, 2016-03-10 23:36:47]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-03-10 23:36:47]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org
Link to the patchset: https://codereview.chromium.org/1783653002/#ps40001
(title: "Addressed sclittle comments")

[commit-bot: I haz the power, 2016-03-10 23:37:38]

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

[commit-bot: I haz the power, 2016-03-11 00:16:05]

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

[commit-bot: I haz the power, 2016-03-11 00:16:06]

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

[Raj, 2016-03-11 06:18:30]

The CQ bit was checked by rajendrant@chromium.org

[Raj, 2016-03-11 06:18:31]

The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org,
sclittle@chromium.org
Link to the patchset: https://codereview.chromium.org/1783653002/#ps60001
(title: "Rebased")

[commit-bot: I haz the power, 2016-03-11 06:18:53]

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

[commit-bot: I haz the power, 2016-03-11 07:49:22]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-03-11 07:51:36]

Description was changed from

==========
save-data header value gets appended on page refreshes

When data-saver is enabled, Chrome sets save-data header value to 'on'.
When the webpage is refreshed, the sub resource requests get the
save-data value as 'on,on'.

BUG=591594
==========

to

==========
save-data header value gets appended on page refreshes

When data-saver is enabled, Chrome sets save-data header value to 'on'.
When the webpage is refreshed, the sub resource requests get the
save-data value as 'on,on'.

BUG=591594

Committed: https://crrev.com/8c957da3f5a7b7e3eb37483a57ebbf323f997f1f
Cr-Commit-Position: refs/heads/master@{#380568}
==========

[commit-bot: I haz the power, 2016-03-11 07:51:37]

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