Preserve original int64 time stamp in ResourceResponse.

Preserve original int64 time stamp in ResourceResponse.

This is part 1/4 of a clean up to switch to int64 time stamps when caching metadata. The background to this is that converting int64 time stamps to double and back to int64 is lossy and causes false time stamp mismatches.

BUG=453298
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190551

[Yang, 2015-02-18 09:16:00]

yangguo@chromium.org changed reviewers:
+ mkwst@chromium.org

[Yang, 2015-02-18 09:46:15]

Please take a look. This is part of a series of CLs between blink and chromium.

Part 1/4: https://codereview.chromium.org/938643002/
Part 2/4: https://codereview.chromium.org/935963002/
Part 3/4: https://codereview.chromium.org/940443002/
Part 4/4: https://codereview.chromium.org/935983002/

https://codereview.chromium.org/938643002/diff/40001/public/platform/WebURLRe...
File public/platform/WebURLResponse.h (right):

https://codereview.chromium.org/938643002/diff/40001/public/platform/WebURLRe...
public/platform/WebURLResponse.h:99: BLINK_PLATFORM_EXPORT void
setOriginalResponseTime(long long);
We use long long and int64 interchangeably when exporting, for example in
blink::WebHTTPBody::setIdentifier.

[Mike West, 2015-02-18 14:34:05]

I think I agree with https://codereview.chromium.org/887973003#msg12; we
shouldn't be using a double here. It's a bit confusing that we vend int64 in
some places, and double in others for a value that's ostensibly the same thing.

This series of patches is an improvement in terms of accuracy, but it's not
clear to me that it leaves Blink in a better state. How much work would it be to
remove m_responseTime entirely, and convert all its accessors to use int64?

[Yang, 2015-02-18 15:00:12]

On 2015/02/18 14:34:05, Mike West wrote:
> I think I agree with https://codereview.chromium.org/887973003#msg12; we
> shouldn't be using a double here. It's a bit confusing that we vend int64 in
> some places, and double in others for a value that's ostensibly the same
thing.
> 
> This series of patches is an improvement in terms of accuracy, but it's not
> clear to me that it leaves Blink in a better state. How much work would it be
to
> remove m_responseTime entirely, and convert all its accessors to use int64?

Given that blink's currentTime returns a double and is used widely, and base::*
seems generally not to be used in blink, introducing base::Time seems a lot of
work. I'm very hesitant to tie a simple bugfix to such a large refactoring.

[Yang, 2015-02-18 15:03:23]

On 2015/02/18 15:00:12, Yang wrote:
> On 2015/02/18 14:34:05, Mike West wrote:
> > I think I agree with https://codereview.chromium.org/887973003#msg12; we
> > shouldn't be using a double here. It's a bit confusing that we vend int64 in
> > some places, and double in others for a value that's ostensibly the same
> thing.
> > 
> > This series of patches is an improvement in terms of accuracy, but it's not
> > clear to me that it leaves Blink in a better state. How much work would it
be
> to
> > remove m_responseTime entirely, and convert all its accessors to use int64?
> 
> Given that blink's currentTime returns a double and is used widely, and
base::*
> seems generally not to be used in blink, introducing base::Time seems a lot of
> work. I'm very hesitant to tie a simple bugfix to such a large refactoring.

What I'm saying is that to be consequent we would need to replace all timestamps
in blink with base::Time instead of double, everywhere, which is out of the
scope of this bug fix.

[Mike West, 2015-02-18 15:38:21]

On 2015/02/18 at 15:03:23, yangguo wrote:
> On 2015/02/18 15:00:12, Yang wrote:
> > On 2015/02/18 14:34:05, Mike West wrote:
> > > I think I agree with https://codereview.chromium.org/887973003#msg12; we
> > > shouldn't be using a double here. It's a bit confusing that we vend int64
in
> > > some places, and double in others for a value that's ostensibly the same
> > thing.
> > > 
> > > This series of patches is an improvement in terms of accuracy, but it's
not
> > > clear to me that it leaves Blink in a better state. How much work would it
be
> > to
> > > remove m_responseTime entirely, and convert all its accessors to use
int64?
> > 
> > Given that blink's currentTime returns a double and is used widely, and
base::*
> > seems generally not to be used in blink, introducing base::Time seems a lot
of
> > work. I'm very hesitant to tie a simple bugfix to such a large refactoring.

Given that `responseTime()` is called something like 3 times in Blink, and
you're already replacing two of them, I think you can replace the other one as
well. `setResponseTime()` is called once, in Chromium, you could kill it as
well.

Basically, I agree with your premise that this would be inconsequent. You're
_already_ being inconsequent by storing the int64 and using it for comparisons.
Let's make this class consistent within itself, file a bug to make the rest of
Blink consistent with it, and go one from there. :)

> What I'm saying is that to be consequent we would need to replace all
timestamps in blink with base::Time instead of double, everywhere, which is out
of the scope of this bug fix.

Yes. We should certainly file a bug to migrate to `base::Time` after we merge
repositories. There's no way we'll be able to do that before then.

[Yang, 2015-02-19 08:36:56]

On 2015/02/18 15:38:21, Mike West wrote:
> On 2015/02/18 at 15:03:23, yangguo wrote:
> > On 2015/02/18 15:00:12, Yang wrote:
> > > On 2015/02/18 14:34:05, Mike West wrote:
> > > > I think I agree with https://codereview.chromium.org/887973003#msg12; we
> > > > shouldn't be using a double here. It's a bit confusing that we vend
int64
> in
> > > > some places, and double in others for a value that's ostensibly the same
> > > thing.
> > > > 
> > > > This series of patches is an improvement in terms of accuracy, but it's
> not
> > > > clear to me that it leaves Blink in a better state. How much work would
it
> be
> > > to
> > > > remove m_responseTime entirely, and convert all its accessors to use
> int64?
> > > 
> > > Given that blink's currentTime returns a double and is used widely, and
> base::*
> > > seems generally not to be used in blink, introducing base::Time seems a
lot
> of
> > > work. I'm very hesitant to tie a simple bugfix to such a large
refactoring.
> 
> Given that `responseTime()` is called something like 3 times in Blink, and
> you're already replacing two of them, I think you can replace the other one as
> well. `setResponseTime()` is called once, in Chromium, you could kill it as
> well.
> 
> Basically, I agree with your premise that this would be inconsequent. You're
> _already_ being inconsequent by storing the int64 and using it for
comparisons.
> Let's make this class consistent within itself, file a bug to make the rest of
> Blink consistent with it, and go one from there. :)
> 
> > What I'm saying is that to be consequent we would need to replace all
> timestamps in blink with base::Time instead of double, everywhere, which is
out
> of the scope of this bug fix.
> 
> Yes. We should certainly file a bug to migrate to `base::Time` after we merge
> repositories. There's no way we'll be able to do that before then.

Understood. I wasn't aware that responseTime is not used anywhere else. I'm
still introducing the int64 time stamp in addition to the old double time stamp.
I will remove the latter in part 3 of this series of CLs though, so that
afterwards we will have fully moved to int64 for responseTime in
ResourceResponse.

[Mike West, 2015-02-19 09:35:29]

LGTM, thanks!

[Yang, 2015-02-19 09:50:40]

New patchsets have been uploaded after l-g-t-m from mkwst@chromium.org

[Yang, 2015-02-20 06:19:43]

The CQ bit was checked by yangguo@chromium.org

[commit-bot: I haz the power, 2015-02-20 06:20:26]

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

[commit-bot: I haz the power, 2015-02-20 11:07:50]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=190551