Post-commit fixes for "stale-while-revalidate..."

Post-commit fixes for "stale-while-revalidate..."

I accidentally committed http://crrev.com/455623003 before getting LGTM from
mmenke@. This CL is for additional fixes that he requested.

BUG=348877
TEST=net_unittests

Committed: https://crrev.com/4373698a495a4949e673f885bfc9ed2da0afb1d3
Cr-Commit-Position: refs/heads/master@{#302056}

[Adam Rice, 2014-10-20 09:32:19]

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

[mmenke, 2014-10-20 14:41:09]

LGTM

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:1024: DCHECK(lifetimes.staleness ==
TimeDelta());
Also, seems safest to use braces when the body of an if uses a macro.

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:1024: DCHECK(lifetimes.staleness ==
TimeDelta());
WHile you're here, should probably use DCHECK_EQ here, and in the two calls
below.

[Adam Rice, 2014-10-21 09:26:14]

PTAL.

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:1024: DCHECK(lifetimes.staleness ==
TimeDelta());
On 2014/10/20 14:41:08, mmenke wrote:
> Also, seems safest to use braces when the body of an if uses a macro.

Done.

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:1024: DCHECK(lifetimes.staleness ==
TimeDelta());
On 2014/10/20 14:41:08, mmenke wrote:
> WHile you're here, should probably use DCHECK_EQ here, and in the two calls
> below.

I did it, but it got a little bit out of control. Let me know what you think.

[mmenke, 2014-10-21 14:20:09]

I suggest moving the base/ changes into another CL - seems a bit far removed
from the original purpose of this one.

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/665023002/diff/40001/net/http/http_response_h...
net/http/http_response_headers.cc:1024: DCHECK(lifetimes.staleness ==
TimeDelta());
On 2014/10/21 09:26:14, Adam Rice wrote:
> On 2014/10/20 14:41:08, mmenke wrote:
> > WHile you're here, should probably use DCHECK_EQ here, and in the two calls
> > below.
> 
> I did it, but it got a little bit out of control. Let me know what you think.

I hadn't realized TimeDeltas did not support display - thought they did, just
for this purpose.  Very cool to add support for that.

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.cc
File base/time/time_logging.cc (right):

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.c...
base/time/time_logging.cc:22: Time::Exploded e;
nit:  Google style guide strong discourages using uncommon abbreviations in
variable names.  Maybe just exploded, though that's not exactly the clearest
name.

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.h
File base/time/time_logging.h (right):

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.h...
base/time/time_logging.h:11: // "base/i18n/time_formatting.h" instead.
Optional:  I think it's more common to put file-level comments below the
includes.  People are trained to ignore comments at the start of file - I know
that I'd probably never see this.  Same goes for time_logging.cc.

[mmenke, 2014-10-21 14:22:00]

https://codereview.chromium.org/665023002/diff/60001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/665023002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:23: #include "base/time/time_logging.h"
I wonder about requiring this include - I don't think I've seen anything else
that does this, and I suspect when most people get compile errors on time
comparisons, they'll just switch to ==, instead of looking for a file.

[mmenke, 2014-10-21 17:15:20]

On 2014/10/21 14:22:00, mmenke wrote:
>
https://codereview.chromium.org/665023002/diff/60001/net/http/http_response_h...
> File net/http/http_response_headers.cc (right):
> 
>
https://codereview.chromium.org/665023002/diff/60001/net/http/http_response_h...
> net/http/http_response_headers.cc:23: #include "base/time/time_logging.h"
> I wonder about requiring this include - I don't think I've seen anything else
> that does this, and I suspect when most people get compile errors on time
> comparisons, they'll just switch to ==, instead of looking for a file.

Oh, and could you fix the description?  I think you're missing a "committed"
before the URL.

[mmenke, 2014-10-22 18:44:42]

LGTM

[Adam Rice, 2014-10-23 01:13:02]

I moved the base/ changes to https://codereview.chromium.org/669083002/ and the
DCHECK_EQ() statements that require them to
https://codereview.chromium.org/670303003/

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.cc
File base/time/time_logging.cc (right):

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.c...
base/time/time_logging.cc:22: Time::Exploded e;
On 2014/10/21 14:20:09, mmenke wrote:
> nit:  Google style guide strong discourages using uncommon abbreviations in
> variable names.  Maybe just exploded, though that's not exactly the clearest
> name.

Done.

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.h
File base/time/time_logging.h (right):

https://codereview.chromium.org/665023002/diff/60001/base/time/time_logging.h...
base/time/time_logging.h:11: // "base/i18n/time_formatting.h" instead.
On 2014/10/21 14:20:09, mmenke wrote:
> Optional:  I think it's more common to put file-level comments below the
> includes.  People are trained to ignore comments at the start of file - I know
> that I'd probably never see this.  Same goes for time_logging.cc.

True. I merged the functionality into time.h anyway.

https://codereview.chromium.org/665023002/diff/60001/net/http/http_response_h...
File net/http/http_response_headers.cc (right):

https://codereview.chromium.org/665023002/diff/60001/net/http/http_response_h...
net/http/http_response_headers.cc:23: #include "base/time/time_logging.h"
On 2014/10/21 14:22:00, mmenke wrote:
> I wonder about requiring this include - I don't think I've seen anything else
> that does this, and I suspect when most people get compile errors on time
> comparisons, they'll just switch to ==, instead of looking for a file.

True. I got rid of the separate include file.

[Adam Rice, 2014-10-23 01:14:25]

ricea@chromium.org changed reviewers:
+ shishir@chromium.org

[Adam Rice, 2014-10-23 01:14:25]

+shishir for chrome/browser/predictors/ OWNERS.

[Adam Rice, 2014-10-24 07:30:20]

On 2014/10/23 01:14:25, Adam Rice wrote:
> +shishir for chrome/browser/predictors/ OWNERS.

ping shishir.

[Adam Rice, 2014-10-28 03:36:35]

ricea@chromium.org changed reviewers:
+ dominich@chromium.org
- shishir@chromium.org

[Adam Rice, 2014-10-28 03:36:36]

-shishir, +dominich for chrome/browser/predictors OWNERS.

[Adam Rice, 2014-10-30 06:09:21]

ping dominich

[Adam Rice, 2014-10-30 06:20:47]

ricea@chromium.org changed reviewers:
+ thestig@chromium.org
- dominich@chromium.org

[Adam Rice, 2014-10-30 06:20:48]

-dominich, +thestig for review of chrome/browser/predictors/

[Lei Zhang, 2014-10-30 06:39:02]

lgtm

https://codereview.chromium.org/665023002/diff/80001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/665023002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:315: bool IsStaleWhileRevalidateEnabled(const
CommandLine& command_line) {
nit: base::CommandLine

https://codereview.chromium.org/665023002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:318: if
(command_line.HasSwitch(switches::kEnableStaleWhileRevalidate))
You can do this first and then call FindFullName() afterwards if needed.

[Adam Rice, 2014-10-30 07:28:58]

https://codereview.chromium.org/665023002/diff/80001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

https://codereview.chromium.org/665023002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:315: bool IsStaleWhileRevalidateEnabled(const
CommandLine& command_line) {
On 2014/10/30 06:39:02, Lei Zhang wrote:
> nit: base::CommandLine

Done.

https://codereview.chromium.org/665023002/diff/80001/chrome/browser/io_thread...
chrome/browser/io_thread.cc:318: if
(command_line.HasSwitch(switches::kEnableStaleWhileRevalidate))
On 2014/10/30 06:39:02, Lei Zhang wrote:
> You can do this first and then call FindFullName() afterwards if needed.

Done.

[Adam Rice, 2014-10-30 07:30:34]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2014-10-30 07:31:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665023002/120001

[commit-bot: I haz the power, 2014-10-30 08:31:44]

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

[commit-bot: I haz the power, 2014-10-30 08:31:45]

Try jobs failed on following builders:
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)

[Adam Rice, 2014-10-30 08:50:28]

The CQ bit was checked by ricea@chromium.org

[commit-bot: I haz the power, 2014-10-30 08:51:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/665023002/120001

[commit-bot: I haz the power, 2014-10-30 09:15:58]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2014-10-30 09:16:36]

Patchset 7 (id:??) landed as
https://crrev.com/4373698a495a4949e673f885bfc9ed2da0afb1d3
Cr-Commit-Position: refs/heads/master@{#302056}