Remove stale-while-revalidate from net

Remove stale-while-revalidate from net

The stale-while-revalidate experiment is being turned down for the
moment. This current implementation is removed due to complexity of
interaction with other existing efforts. See
https://bugs.chromium.org/p/chromium/issues/detail?id=561609#c10.

BUG=348877, 700568
Review-Url: https://codereview.chromium.org/2763393002
Cr-Commit-Position: refs/heads/master@{#459857}
Committed: https://chromium.googlesource.com/chromium/src/+/75221e53b71cc5eb37cbfb957124025259aa5dab

[scottmg, 2017-03-22 17:13:33]

Description was changed from

==========
Remove stale-while-revalidate from net and blink

BUG=348877,520613,700568
==========

to

==========
Remove stale-while-revalidate from net

The stale-while-revalidate experiment is being turned down for the
moment. This current implementation is removed due to complexity of
interaction with other existing efforts. See
https://bugs.chromium.org/p/chromium/issues/detail?id=561609#c10.

BUG=348877,700568
==========

[scottmg, 2017-03-22 17:13:33]

scottmg@chromium.org changed reviewers:
+ mmenke@chromium.org

[scottmg, 2017-03-22 17:13:37]

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

[commit-bot: I haz the power, 2017-03-22 17:13:50]

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-03-22 17:16:40]

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

[commit-bot: I haz the power, 2017-03-22 17:16:41]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)

[scottmg, 2017-03-22 17:55:35]

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

[scottmg, 2017-03-22 17:56:24]

Patchset #4 (id:60001) has been deleted

[commit-bot: I haz the power, 2017-03-22 17:57:17]

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

[scottmg, 2017-03-22 17:58:12]

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

[commit-bot: I haz the power, 2017-03-22 17:59:06]

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

[scottmg, 2017-03-22 18:52:27]

Hi Matt, this was a bit intricate to unwind in parts. If you think there's a
better way to approach removing this, let me know.

[mmenke, 2017-03-22 18:55:01]

mmenke@chromium.org changed reviewers:
+ jkarlin@chromium.org

[mmenke, 2017-03-22 18:55:01]

[+jkarlin]:  Josh, did you review the CLs that added this?  If not, I'm happy to
review this CL, just thought you might know the code better.

[commit-bot: I haz the power, 2017-03-22 20:14:50]

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

[commit-bot: I haz the power, 2017-03-22 20:14:51]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2017-03-23 11:25:54]

On 2017/03/22 18:55:01, mmenke wrote:
> [+jkarlin]:  Josh, did you review the CLs that added this?  If not, I'm happy
to
> review this CL, just thought you might know the code better.

Nope, I didn't review the original. Please go ahead and review.

[mmenke, 2017-03-23 15:31:48]

On 2017/03/23 11:25:54, jkarlin wrote:
> On 2017/03/22 18:55:01, mmenke wrote:
> > [+jkarlin]:  Josh, did you review the CLs that added this?  If not, I'm
happy
> to
> > review this CL, just thought you might know the code better.
> 
> Nope, I didn't review the original. Please go ahead and review.

Thanks!  I'll try to get to this today, but may end up being tomorrow (Expect to
just sign off on it, but want to be careful, and not an expert on this code).

[mmenke, 2017-03-23 15:31:58]

mmenke@chromium.org changed reviewers:
- jkarlin@chromium.org

[mmenke, 2017-03-23 21:39:36]

Looks to me like you did a pretty thorough job of ripping it out, just a couple
minor comments (Not signing off now just because I'm paranoid about inverting
the bool checks I suggest you inline)

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:29: #include "base/values.h"
Tiny nit:  Is this still used in this file?  It was added for SWR, don't see any
current usages (Even before this CL, actually)

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:77: };
This enum (And the related histogram) look to have been added as part of the SWR
experiment.

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
net/http/http_response_headers.cc:932: return true;
optional:  Maybe just replace these last 4 lines with:

return lifetime <= GetCurrentAge(request_time, response_time, current_time);

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
net/http/http_response_headers.cc:969: bool must_revalidate =
HasHeaderValue("cache-control", "must-revalidate");
Can get rid of the must_revalidate local, and inline the HasHeaderValue call
below.

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
net/http/http_response_headers_unittest.cc:46: class
HttpResponseHeadersCacheControlTest : public HttpResponseHeadersTest {
Test fixture no longer used.

[scottmg, 2017-03-24 22:45:59]

scottmg@chromium.org changed reviewers:
+ isherman@chromium.org

[scottmg, 2017-03-24 22:46:00]

Thanks! +isherman for histograms.xml

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_cache_tra...
File net/http/http_cache_transaction.cc (right):

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:29: #include "base/values.h"
On 2017/03/23 21:39:35, mmenke wrote:
> Tiny nit:  Is this still used in this file?  It was added for SWR, don't see
any
> current usages (Even before this CL, actually)

Done.

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_cache_tra...
net/http/http_cache_transaction.cc:77: };
On 2017/03/23 21:39:35, mmenke wrote:
> This enum (And the related histogram) look to have been added as part of the
SWR
> experiment.

Aha, good catch. Done.

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
net/http/http_response_headers.cc:932: return true;
On 2017/03/23 21:39:35, mmenke wrote:
> optional:  Maybe just replace these last 4 lines with:
> 
> return lifetime <= GetCurrentAge(request_time, response_time, current_time);

Done. (I was a bit nervous about messing up on a bool, so I was trying to be a
bit conservative.)

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
net/http/http_response_headers.cc:969: bool must_revalidate =
HasHeaderValue("cache-control", "must-revalidate");
On 2017/03/23 21:39:35, mmenke wrote:
> Can get rid of the must_revalidate local, and inline the HasHeaderValue call
> below.

Done.

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
net/http/http_response_headers_unittest.cc:46: class
HttpResponseHeadersCacheControlTest : public HttpResponseHeadersTest {
On 2017/03/23 21:39:35, mmenke wrote:
> Test fixture no longer used.

Sorry, I don't understand this one. This class is used around line 2050 if
that's what you mean.

[scottmg, 2017-03-24 22:58:06]

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

[commit-bot: I haz the power, 2017-03-24 22:58:56]

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

[Ilya Sherman, 2017-03-24 23:18:58]

histograms.xml lgtm

[commit-bot: I haz the power, 2017-03-25 00:19:08]

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

[commit-bot: I haz the power, 2017-03-25 00:19:10]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-03-27 14:21:38]

LGTM (Just looked at the code where you responded to my comments - assume the
other changes were due to a merge)

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
File net/http/http_response_headers_unittest.cc (right):

https://codereview.chromium.org/2763393002/diff/80001/net/http/http_response_...
net/http/http_response_headers_unittest.cc:46: class
HttpResponseHeadersCacheControlTest : public HttpResponseHeadersTest {
On 2017/03/24 22:46:00, scottmg wrote:
> On 2017/03/23 21:39:35, mmenke wrote:
> > Test fixture no longer used.
> 
> Sorry, I don't understand this one. This class is used around line 2050 if
> that's what you mean.

Ahh, missed those.  The fixture was added for SWR, and I couldn't seem to find
any other uses of this fixture.

[scottmg, 2017-03-27 16:54:59]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2017-03-27 16:55:46]

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-03-27 18:41:15]

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

[commit-bot: I haz the power, 2017-03-27 18:41:16]

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

[mmenke, 2017-03-27 18:58:06]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2017-03-27 18:58:59]

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-03-27 19:50:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490641086695140,
"parent_rev": "0ce2435b3843ad695ff354cd3383bbfd354b2fac", "commit_rev":
"75221e53b71cc5eb37cbfb957124025259aa5dab"}

[commit-bot: I haz the power, 2017-03-27 19:51:12]

Description was changed from

==========
Remove stale-while-revalidate from net

The stale-while-revalidate experiment is being turned down for the
moment. This current implementation is removed due to complexity of
interaction with other existing efforts. See
https://bugs.chromium.org/p/chromium/issues/detail?id=561609#c10.

BUG=348877,700568
==========

to

==========
Remove stale-while-revalidate from net

The stale-while-revalidate experiment is being turned down for the
moment. This current implementation is removed due to complexity of
interaction with other existing efforts. See
https://bugs.chromium.org/p/chromium/issues/detail?id=561609#c10.

BUG=348877,700568

Review-Url: https://codereview.chromium.org/2763393002
Cr-Commit-Position: refs/heads/master@{#459857}
Committed:
https://chromium.googlesource.com/chromium/src/+/75221e53b71cc5eb37cbfb957124...
==========

[commit-bot: I haz the power, 2017-03-27 19:51:14]

Committed patchset #5 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/75221e53b71cc5eb37cbfb957124...