Allow httpproxy to return 304 instead of 200 on conditional requests.

Allow httpproxy to return 304 instead of 200 on conditional requests.

BUG=#76

[gabadie, 2016-05-27 15:58:11]

Hey Ned,

This CL allow to generate 304 response on conditional requests. PTAL.

[nednguyen, 2016-05-29 21:42:45]

lg2me oveall but can you add test coverage?

https://codereview.appspot.com/294640043/diff/1/replay.py
File replay.py (right):

https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
replay.py:545: '304 response.')
Why do we need a flag for this 304 response? I think it's reasonable and can
just be the standard behavior

[pasko, 2016-05-30 11:32:25]

https://codereview.appspot.com/294640043/diff/1/httpproxy.py
File httpproxy.py (right):

https://codereview.appspot.com/294640043/diff/1/httpproxy.py#newcode249
httpproxy.py:249: response = httparchive.create_response(
as discussed offline, this may not produce a typical 304 response. I think a
test for this in httpproxy_test.py would be sufficient to satisfy the testing
requirement that Ned expressed.

https://codereview.appspot.com/294640043/diff/1/replay.py
File replay.py (right):

https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
replay.py:545: '304 response.')
On 2016/05/29 21:42:45, nednguyen wrote:
> Why do we need a flag for this 304 response? I think it's reasonable and can
> just be the standard behavior

Changing this behavior will affect benchmarking results (page cycler and
potentially others). It is important to keep benchmarks reproducible and new
results comparable to what was produced a while ago.

Even without changes like this benchmarking is full of pitfalls and unpleasant
surprises rendering many results useless.

We can certainly deprecate and remove the old mode after we get agreement from
all stakeholders: benchmark owners, WebPageTest, tools/android/loading and
others.

[nednguyen, 2016-05-30 14:54:14]

+ Kouhei who is the owner of page_cycler suites.
+Patrick: owner of WPT.

Thoughts?

[kouhei, 2016-05-31 00:45:20]

On 2016/05/30 11:32:25, pasko wrote:
> https://codereview.appspot.com/294640043/diff/1/httpproxy.py
> File httpproxy.py (right):
> 
> https://codereview.appspot.com/294640043/diff/1/httpproxy.py#newcode249
> httpproxy.py:249: response = httparchive.create_response(
> as discussed offline, this may not produce a typical 304 response. I think a
> test for this in httpproxy_test.py would be sufficient to satisfy the testing
> requirement that Ned expressed.
> 
> https://codereview.appspot.com/294640043/diff/1/replay.py
> File replay.py (right):
> 
> https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
> replay.py:545: '304 response.')
> On 2016/05/29 21:42:45, nednguyen wrote:
> > Why do we need a flag for this 304 response? I think it's reasonable and can
> > just be the standard behavior
> 
> Changing this behavior will affect benchmarking results (page cycler and
> potentially others). It is important to keep benchmarks reproducible and new
> results comparable to what was produced a while ago.
> 
> Even without changes like this benchmarking is full of pitfalls and unpleasant
> surprises rendering many results useless.
> 
> We can certainly deprecate and remove the old mode after we get agreement from
> all stakeholders: benchmark owners, WebPageTest, tools/android/loading and
> others.

Both PCv1/v2 supports disk cache control. I'm in favor of 304 support.

[gabadie, 2016-05-31 11:51:32]

Thanks! Uploaded a new revision adding unittest.

Ned: PTAL.

https://codereview.appspot.com/294640043/diff/1/httpproxy.py
File httpproxy.py (right):

https://codereview.appspot.com/294640043/diff/1/httpproxy.py#newcode249
httpproxy.py:249: response = httparchive.create_response(
On 2016/05/30 11:32:25, pasko wrote:
> as discussed offline, this may not produce a typical 304 response. I think a
> test for this in httpproxy_test.py would be sufficient to satisfy the testing
> requirement that Ned expressed.

Done.

https://codereview.appspot.com/294640043/diff/1/replay.py
File replay.py (right):

https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
replay.py:545: '304 response.')
On 2016/05/30 11:32:25, pasko wrote:
> On 2016/05/29 21:42:45, nednguyen wrote:
> > Why do we need a flag for this 304 response? I think it's reasonable and can
> > just be the standard behavior
> 
> Changing this behavior will affect benchmarking results (page cycler and
> potentially others). It is important to keep benchmarks reproducible and new
> results comparable to what was produced a while ago.
> 
> Even without changes like this benchmarking is full of pitfalls and unpleasant
> surprises rendering many results useless.
> 
> We can certainly deprecate and remove the old mode after we get agreement from
> all stakeholders: benchmark owners, WebPageTest, tools/android/loading and
> others.

Having this debate here to me sounds pre-mature because it seams like we are
requesting opinions only from Chromium or Google projects using WPR. This CL do
not change default behavior of WPR, and making it by default would just be a
trivial CL. Therefore, let's not block this CL by having this debate of
generating 304 by default independent, because this CL purpose's is only to
fulfill a missing feature of WPR that pasko@ and I require.

[pmeenan1, 2016-05-31 14:08:47]

On 2016/05/31 11:51:32, gabadie wrote:
> Thanks! Uploaded a new revision adding unittest.
> 
> Ned: PTAL.
> 
> https://codereview.appspot.com/294640043/diff/1/httpproxy.py
> File httpproxy.py (right):
> 
> https://codereview.appspot.com/294640043/diff/1/httpproxy.py#newcode249
> httpproxy.py:249: response = httparchive.create_response(
> On 2016/05/30 11:32:25, pasko wrote:
> > as discussed offline, this may not produce a typical 304 response. I think a
> > test for this in httpproxy_test.py would be sufficient to satisfy the
testing
> > requirement that Ned expressed.
> 
> Done.
> 
> https://codereview.appspot.com/294640043/diff/1/replay.py
> File replay.py (right):
> 
> https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
> replay.py:545: '304 response.')
> On 2016/05/30 11:32:25, pasko wrote:
> > On 2016/05/29 21:42:45, nednguyen wrote:
> > > Why do we need a flag for this 304 response? I think it's reasonable and
can
> > > just be the standard behavior
> > 
> > Changing this behavior will affect benchmarking results (page cycler and
> > potentially others). It is important to keep benchmarks reproducible and new
> > results comparable to what was produced a while ago.
> > 
> > Even without changes like this benchmarking is full of pitfalls and
unpleasant
> > surprises rendering many results useless.
> > 
> > We can certainly deprecate and remove the old mode after we get agreement
from
> > all stakeholders: benchmark owners, WebPageTest, tools/android/loading and
> > others.
> 
> Having this debate here to me sounds pre-mature because it seams like we are
> requesting opinions only from Chromium or Google projects using WPR. This CL
do
> not change default behavior of WPR, and making it by default would just be a
> trivial CL. Therefore, let's not block this CL by having this debate of
> generating 304 by default independent, because this CL purpose's is only to
> fulfill a missing feature of WPR that pasko@ and I require.

WebPageTest will work fine with either behavior though the allowing of 304's
would be better for repeat view testing (most use cases where WPT uses WPR are
only looking at first view so it doesn't come up often).

[nednguyen, 2016-05-31 17:19:42]

https://codereview.appspot.com/294640043/diff/1/replay.py
File replay.py (right):

https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
replay.py:545: '304 response.')
On 2016/05/31 11:51:32, gabadie wrote:
> On 2016/05/30 11:32:25, pasko wrote:
> > On 2016/05/29 21:42:45, nednguyen wrote:
> > > Why do we need a flag for this 304 response? I think it's reasonable and
can
> > > just be the standard behavior
> > 
> > Changing this behavior will affect benchmarking results (page cycler and
> > potentially others). It is important to keep benchmarks reproducible and new
> > results comparable to what was produced a while ago.
> > 
> > Even without changes like this benchmarking is full of pitfalls and
unpleasant
> > surprises rendering many results useless.
> > 
> > We can certainly deprecate and remove the old mode after we get agreement
from
> > all stakeholders: benchmark owners, WebPageTest, tools/android/loading and
> > others.
> 
> Having this debate here to me sounds pre-mature because it seams like we are
> requesting opinions only from Chromium or Google projects using WPR. This CL
do
> not change default behavior of WPR, and making it by default would just be a
> trivial CL. Therefore, let's not block this CL by having this debate of
> generating 304 by default independent, because this CL purpose's is only to
> fulfill a missing feature of WPR that pasko@ and I require.

With comments from Kouhei@ & Patric, I am pretty sure that this is a good
behavior change to make by default. Adding another flag increase the API surface
& flags are notoriously known hard to deprecate. Please remove the extra flag &
I will stamp this change.

[pasko, 2016-05-31 17:35:18]

On 2016/05/31 17:19:42, nednguyen wrote:
> https://codereview.appspot.com/294640043/diff/1/replay.py
> File replay.py (right):
> 
> https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
> replay.py:545: '304 response.')
> On 2016/05/31 11:51:32, gabadie wrote:
> > On 2016/05/30 11:32:25, pasko wrote:
> > > On 2016/05/29 21:42:45, nednguyen wrote:
> > > > Why do we need a flag for this 304 response? I think it's reasonable and
> can
> > > > just be the standard behavior
> > > 
> > > Changing this behavior will affect benchmarking results (page cycler and
> > > potentially others). It is important to keep benchmarks reproducible and
new
> > > results comparable to what was produced a while ago.
> > > 
> > > Even without changes like this benchmarking is full of pitfalls and
> unpleasant
> > > surprises rendering many results useless.
> > > 
> > > We can certainly deprecate and remove the old mode after we get agreement
> from
> > > all stakeholders: benchmark owners, WebPageTest, tools/android/loading and
> > > others.
> > 
> > Having this debate here to me sounds pre-mature because it seams like we are
> > requesting opinions only from Chromium or Google projects using WPR. This CL
> do
> > not change default behavior of WPR, and making it by default would just be a
> > trivial CL. Therefore, let's not block this CL by having this debate of
> > generating 304 by default independent, because this CL purpose's is only to
> > fulfill a missing feature of WPR that pasko@ and I require.
> 
> With comments from Kouhei@ & Patric, I am pretty sure that this is a good
> behavior change to make by default. Adding another flag increase the API
surface
> & flags are notoriously known hard to deprecate. Please remove the extra flag
&
> I will stamp this change.

I would prefer to be extra cautious here. We suffered before from changes in
WPR, Telemetry, Chrome, Android, etc. Let's make sure all affected parties know
why their numbers pop up and down before landing this. If there is no flag, how
would folks determine whether this mode affects them?

What forums should we send the heads-up to? My list is: net-dev@, loading-dev@,
chromium-dev@.

[nednguyen, 2016-05-31 17:38:21]

On 2016/05/31 17:35:18, pasko wrote:
> On 2016/05/31 17:19:42, nednguyen wrote:
> > https://codereview.appspot.com/294640043/diff/1/replay.py
> > File replay.py (right):
> > 
> > https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
> > replay.py:545: '304 response.')
> > On 2016/05/31 11:51:32, gabadie wrote:
> > > On 2016/05/30 11:32:25, pasko wrote:
> > > > On 2016/05/29 21:42:45, nednguyen wrote:
> > > > > Why do we need a flag for this 304 response? I think it's reasonable
and
> > can
> > > > > just be the standard behavior
> > > > 
> > > > Changing this behavior will affect benchmarking results (page cycler and
> > > > potentially others). It is important to keep benchmarks reproducible and
> new
> > > > results comparable to what was produced a while ago.
> > > > 
> > > > Even without changes like this benchmarking is full of pitfalls and
> > unpleasant
> > > > surprises rendering many results useless.
> > > > 
> > > > We can certainly deprecate and remove the old mode after we get
agreement
> > from
> > > > all stakeholders: benchmark owners, WebPageTest, tools/android/loading
and
> > > > others.
> > > 
> > > Having this debate here to me sounds pre-mature because it seams like we
are
> > > requesting opinions only from Chromium or Google projects using WPR. This
CL
> > do
> > > not change default behavior of WPR, and making it by default would just be
a
> > > trivial CL. Therefore, let's not block this CL by having this debate of
> > > generating 304 by default independent, because this CL purpose's is only
to
> > > fulfill a missing feature of WPR that pasko@ and I require.
> > 
> > With comments from Kouhei@ & Patric, I am pretty sure that this is a good
> > behavior change to make by default. Adding another flag increase the API
> surface
> > & flags are notoriously known hard to deprecate. Please remove the extra
flag
> &
> > I will stamp this change.
> 
> I would prefer to be extra cautious here. We suffered before from changes in
> WPR, Telemetry, Chrome, Android, etc. Let's make sure all affected parties
know
> why their numbers pop up and down before landing this. If there is no flag,
how
> would folks determine whether this mode affects them?
> 
> What forums should we send the heads-up to? My list is: net-dev@,
loading-dev@,
> chromium-dev@.

That list seems good to me. Also cc cbruni@ & nikolaos@ from v8 team

[gabadie, 2016-06-01 11:29:19]

On 2016/05/31 17:19:42, nednguyen wrote:
> https://codereview.appspot.com/294640043/diff/1/replay.py
> File replay.py (right):
> 
> https://codereview.appspot.com/294640043/diff/1/replay.py#newcode545
> replay.py:545: '304 response.')
> On 2016/05/31 11:51:32, gabadie wrote:
> > On 2016/05/30 11:32:25, pasko wrote:
> > > On 2016/05/29 21:42:45, nednguyen wrote:
> > > > Why do we need a flag for this 304 response? I think it's reasonable and
> can
> > > > just be the standard behavior
> > > 
> > > Changing this behavior will affect benchmarking results (page cycler and
> > > potentially others). It is important to keep benchmarks reproducible and
new
> > > results comparable to what was produced a while ago.
> > > 
> > > Even without changes like this benchmarking is full of pitfalls and
> unpleasant
> > > surprises rendering many results useless.
> > > 
> > > We can certainly deprecate and remove the old mode after we get agreement
> from
> > > all stakeholders: benchmark owners, WebPageTest, tools/android/loading and
> > > others.
> > 
> > Having this debate here to me sounds pre-mature because it seams like we are
> > requesting opinions only from Chromium or Google projects using WPR. This CL
> do
> > not change default behavior of WPR, and making it by default would just be a
> > trivial CL. Therefore, let's not block this CL by having this debate of
> > generating 304 by default independent, because this CL purpose's is only to
> > fulfill a missing feature of WPR that pasko@ and I require.
> 
> With comments from Kouhei@ & Patric, I am pretty sure that this is a good
> behavior change to make by default. Adding another flag increase the API
surface
> & flags are notoriously known hard to deprecate. Please remove the extra flag
&
> I will stamp this change.

I would be happier with a command line invocation failure because of argparse
not knowing a flag (because has been removed) rather than an under the hoot
silent change. But OK.

[nednguyen, 2016-06-01 17:57:02]

lgtm

Let me know when you think this is ready to land & I will land this

https://codereview.appspot.com/294640043/diff/40001/httpproxy_test.py
File httpproxy_test.py (right):

https://codereview.appspot.com/294640043/diff/40001/httpproxy_test.py#newcode50
httpproxy_test.py:50: del request
nit: "del request  # unused"

[gabadie, 2016-06-02 07:26:32]

Thanks Ned. The latest revision is the one.

https://codereview.appspot.com/294640043/diff/40001/httpproxy_test.py
File httpproxy_test.py (right):

https://codereview.appspot.com/294640043/diff/40001/httpproxy_test.py#newcode50
httpproxy_test.py:50: del request
On 2016/06/01 17:57:02, nednguyen wrote:
> nit: "del request  # unused"

Done.

[pasko, 2016-06-02 08:36:16]

lgtm with an extra assert in tests

https://codereview.appspot.com/294640043/diff/60001/httpproxy_test.py
File httpproxy_test.py (right):

https://codereview.appspot.com/294640043/diff/60001/httpproxy_test.py#newcode235
httpproxy_test.py:235: self.assertEqual(304, response.status)
we should also assert on *not* getting the body with the 304

[gabadie, 2016-06-02 13:04:59]

Uploaded a new revision addressing Pasko's whish.

Ned: you can go ahead. :)

Thanks!

https://codereview.appspot.com/294640043/diff/60001/httpproxy_test.py
File httpproxy_test.py (right):

https://codereview.appspot.com/294640043/diff/60001/httpproxy_test.py#newcode235
httpproxy_test.py:235: self.assertEqual(304, response.status)
On 2016/06/02 08:36:16, pasko wrote:
> we should also assert on *not* getting the body with the 304

Done.

[nednguyen, 2016-06-02 15:50:27]

On 2016/06/02 13:04:59, gabadie wrote:
> Uploaded a new revision addressing Pasko's whish.
> 
> Ned: you can go ahead. :)
> 
> Thanks!
> 
> https://codereview.appspot.com/294640043/diff/60001/httpproxy_test.py
> File httpproxy_test.py (right):
> 
>
https://codereview.appspot.com/294640043/diff/60001/httpproxy_test.py#newcode235
> httpproxy_test.py:235: self.assertEqual(304, response.status)
> On 2016/06/02 08:36:16, pasko wrote:
> > we should also assert on *not* getting the body with the 304
> 
> Done.

Landed in
https://github.com/chromium/web-page-replay/commit/aaee1e086c35d02cb9c70f4f1f...