Use a monotonic clock (TimeTicks) to report network times to WebCore.

FYI, this was also landed with WebKit roll 102911:102988.

Use a monotonic clock (TimeTicks) to report network times to WebCore.

Navigation Timing (window.performance.timing) is now spec'd to use monotonically increasing time, so we need to follow suit. Most of this CL is just plumbing TimeTicks through. We already use TimeTicks to implement monotonicallyIncreasingTime() in WebCore.

On Windows, the clock doesn't tick uniformly between processes, so there is a layer in content that corrects for skew.

BUG=None
TEST=Adhoc (ran webtiming-* LayoutTests from Chrome) and content_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114736

[pfeldman, 2011-08-09 18:15:22]

http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bri...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bri...
webkit/glue/resource_loader_bridge.h:56: int64 base_ticks;
To clarify: you only need this to be able to compare it against finishTime when
you start sending it as ticks?

[James Simonsen, 2011-08-09 18:31:51]

http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bri...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bri...
webkit/glue/resource_loader_bridge.h:56: int64 base_ticks;
On 2011/08/09 18:15:22, pfeldman wrote:
> To clarify: you only need this to be able to compare it against finishTime
when
> you start sending it as ticks?

Oops. In my haste, I uploaded a slightly older branch. Maybe now it's clearer.

Let me try to explain my initial idea again.

1. WebCore records currentTime() and monotonicallyIncreasingTime() (aka
TimeTicks) when the loader is created. These are the reference* times.

2. Chrome's network stack records base_ticks. All of the times in
WebURLLoadTiming are relative to that, including a new responseEnd.

3. When a Javascript user wants to know when the end of the document is
received, we need to convert the monotonic time back to a wall time. The
calculation is:

referenceTime + (base_ticks - monotonicReferenceTime) + responseEnd

I'm proposing finishTime not be used at all for the above use case. I'd love to
convert inspector over to using responseEnd too.

The idea of a Document's reference time comes from the Navigation Timing spec
and is implemented in IE. Someone from Mozilla mentioned they also have a
similar concept.

[pfeldman, 2011-08-09 19:38:34]

> 1. WebCore records currentTime() and monotonicallyIncreasingTime() (aka
> TimeTicks) when the loader is created. These are the reference* times.
> 
> 2. Chrome's network stack records base_ticks. All of the times in
> WebURLLoadTiming are relative to that, including a new responseEnd.
> 

We are sending WebURLLoadTiming along with the response signal. By the time it
is issued, responseEnd is not yet known. So there is no chance WebURLLoadTiming
has new responseEnd field. Please tell me what I am missing here.

> 3. When a Javascript user wants to know when the end of the document is
> received, we need to convert the monotonic time back to a wall time. The
> calculation is:
> 
> referenceTime + (base_ticks - monotonicReferenceTime) + responseEnd
> 

Using "base_ticks - monotonicReferenceTime" to sync implies that tics are the
same in WebCore and embedder, across processes. Is it the case?

> I'm proposing finishTime not be used at all for the above use case. I'd love
to
> convert inspector over to using responseEnd too.
> 

I am still convinced that there is no responseEnd and the only way to pass
finish time into WebCore is by means of finishTime. As I mentioned earlier,
there is no way to get load timing struct from within didFinishLoading due to
its stateless notifier nature. Furthermore, due to the "logging" nature of the
network callbacks, you should provide information into WebCore incrementally, as
it becomes available. I.e. provide request / response timing with the response +
provide finish time along with didFinish message.

[James Simonsen, 2011-08-09 22:56:39]

On 2011/08/09 19:38:34, pfeldman wrote:
> > 1. WebCore records currentTime() and monotonicallyIncreasingTime() (aka
> > TimeTicks) when the loader is created. These are the reference* times.
> > 
> > 2. Chrome's network stack records base_ticks. All of the times in
> > WebURLLoadTiming are relative to that, including a new responseEnd.
> > 
> 
> We are sending WebURLLoadTiming along with the response signal. By the time it
> is issued, responseEnd is not yet known. So there is no chance
WebURLLoadTiming
> has new responseEnd field. Please tell me what I am missing here.

Hmm... Yup. Bummer. :(

> > 3. When a Javascript user wants to know when the end of the document is
> > received, we need to convert the monotonic time back to a wall time. The
> > calculation is:
> > 
> > referenceTime   (base_ticks - monotonicReferenceTime)   responseEnd
> > 
> 
> Using "base_ticks - monotonicReferenceTime" to sync implies that tics are the
> same in WebCore and embedder, across processes. Is it the case?

Yeah, that was my understanding. I did some digging to back it up. They all seem
to be based on system-wide clocks, not process specific clocks.

Win: http://drdobbs.com/184416651

Linux:
http://stackoverflow.com/questions/4943733/is-clock-monotonic-process-or-thre...

Mac: Couldn't find any really good references. This was the best:
http://doc.qt.nokia.com/latest/qelapsedtimer.html

> > I'm proposing finishTime not be used at all for the above use case. I'd love
> to
> > convert inspector over to using responseEnd too.
> > 
> 
> I am still convinced that there is no responseEnd and the only way to pass
> finish time into WebCore is by means of finishTime. As I mentioned earlier,
> there is no way to get load timing struct from within didFinishLoading due to
> its stateless notifier nature. Furthermore, due to the "logging" nature of the
> network callbacks, you should provide information into WebCore incrementally,
as
> it becomes available. I.e. provide request / response timing with the response
 
> provide finish time along with didFinish message.

We definitely need a monotonic time to make this work correctly. What do you
think the best way is to pass this down then? I could add an extra argument
everywhere that is 0 in the other ports. Or I could have some sort of port-wide
bool, like isFinishTimeMonotonic() and handle both cases until everyone switches
over. Either way, the long term plan would be to get rid of the wall clock, so
the two code paths would be "temporary."

[pfeldman, 2011-08-10 06:37:29]

Technically, finishTime is no problem. We can say that although not in the same
struct, finishTime should be provided in the same terms as ResourceLoadTiming
(i.e. as an integer offset to the base ticks). Chromium is the only port
providing those, we'll just make sure rest of the ports carefully pass 0 and not
currentTime. Probably renaming the parameter would help.

API issues aside, I agree that we need to have monotonic time, but above that we
need to have adequate time. WebTiming is all about perceived time, wall clock is
measuring perceived time. In contrast, ticks don't tick when CPU does not spin.
So they are monotonic, but not adequate.

To give you some data, we've seen all kinds of problems before we started
synchronizing ticks with wall clock every ten seconds
(http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/net/load_timing_...).
We are not afraid of it in the request phase (connection is usually established
within seconds). But using ticks for finishTime might be [theoretically]
dangerous. So migrating to the ticks overall is potentially compromising the
idea of the web timing. I am not sure which is more likely to happen: leap
second or CPU suspend, I tend to think that the latter.

From the consumer side (from my Calendar times), I can say that timing data is
always considered statistical. In either case it contains a lot of errors. It is
aggregated, outliers are thrown away, the data is analysed. I don't see
monotonic migration bringing anything good into picture.

Back to the code, other than that, I don't see a problem with your 'reference
time' solution.

[James Simonsen, 2011-08-11 00:44:37]

On 2011/08/10 06:37:29, pfeldman wrote:
> Technically, finishTime is no problem. We can say that although not in the
same
> struct, finishTime should be provided in the same terms as ResourceLoadTiming
> (i.e. as an integer offset to the base ticks). Chromium is the only port
> providing those, we'll just make sure rest of the ports carefully pass 0 and
not
> currentTime. Probably renaming the parameter would help.

Okay, I'll pursue this and upload a new patch.

> API issues aside, I agree that we need to have monotonic time, but above that
we
> need to have adequate time. WebTiming is all about perceived time, wall clock
is
> measuring perceived time. In contrast, ticks don't tick when CPU does not
spin.
> So they are monotonic, but not adequate.
> 
> To give you some data, we've seen all kinds of problems before we started
> synchronizing ticks with wall clock every ten seconds
>
(http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/net/load_timing_...).
> We are not afraid of it in the request phase (connection is usually
established
> within seconds). But using ticks for finishTime might be [theoretically]
> dangerous. So migrating to the ticks overall is potentially compromising the
> idea of the web timing. I am not sure which is more likely to happen: leap
> second or CPU suspend, I tend to think that the latter.
> 
> From the consumer side (from my Calendar times), I can say that timing data is
> always considered statistical. In either case it contains a lot of errors. It
is
> aggregated, outliers are thrown away, the data is analysed. I don't see
> monotonic migration bringing anything good into picture.

Actually, users are the ones that motivated this. We got several complaints that
pages were loading before they started and users couldn't handle the negative
load times at first. The negative times are a little embarrassing too.

So, we brought this issue up with the perf working group. Microsoft said they
already used a monotonic clock referenced from the document start. Mozilla said
they could use a similar thing too, so we decided to spec that. Now we're trying
to update Chrome to meet the spec:

https://dvcs.w3.org/hg/webperf/raw-file/tip/specs/NavigationTiming/Overview.h...

Maybe Chrome just needs a more robust monotonic clock than TimeTicks? I could
create a MonotonicClock class in Chrome. For the time being, it would just be a
pass-through to TimeTicks. But, we could later update it to handle the cases you
described, like if the computer goes to sleep.

Then I'll just update WebKit's monotonicallyIncreasingTime() and
LoadTimeObserver to use MonotonicClock. As that improves, the use cases will
benefit.

[pfeldman, 2011-08-11 06:19:13]

Sounds good. Back to the formula, do you still need to pass base_time in the
timing info struct along with base_ticks + sync clock in chrome for that? It
sounds like sending base_ticks will be sufficient.

[James Simonsen, 2011-08-12 04:52:33]

On 2011/08/11 06:19:13, pfeldman wrote:
> Sounds good. Back to the formula, do you still need to pass base_time in the
> timing info struct along with base_ticks + sync clock in chrome for that? It
> sounds like sending base_ticks will be sufficient.

Yep, that's sufficient for WebKit's ResourceLoadTiming. I was afraid to change
the meaning of fields, since I thought other ports may be using it.

I left base_time in WebURLLoadTiming for now though, because we have to compare
against the cache hit times. I think I've changed enough for this CL, so I'd
like to follow up on removing base_time in a separate CL.

Anyway, I'll upload a new patch here shortly with a crude implementation of
MonotonicTime. I'll post a better one once we finalize the WebKit change. We
might need to break out and discuss the MonotonicTime separately too.

[James Simonsen, 2011-08-25 05:11:00]

On 2011/08/10 06:37:29, pfeldman wrote:
> To give you some data, we've seen all kinds of problems before we started
> synchronizing ticks with wall clock every ten seconds

How bad were these problems?

I did a bunch of reading on the web about this. It looks like Windows is the
only platform that keeps track of TimeTicks while the machine is suspended.
Linux will eventually support it, but only after CLOCK_BOOTTIME is implemented
(Android already has something similar). I couldn't find any solution for Mac.

Based on this, I think we're better off using TimeTicks instead of introducing a
new type. It seems there's already a difference in behavior between platforms.
We may as well just pick one and try to converge on that. Any hacks that I
would've put in MonotonicTime are probably better placed in the
platform-specific TimeTicks implementations.

I'm not convinced that using elapsed realtime is best though. For graphics,
games, and animation, having TimeTicks sleep seems better. For network, I could
argue it either way. I'm tempted to say that having TimeTicks sleep is more
useful to a site-developer than having it tick while nothing is happening.

[pfeldman, 2011-08-25 07:19:25]

On Thu, Aug 25, 2011 at 9:11 AM, <simonjam@chromium.org> wrote:

> On 2011/08/10 06:37:29, pfeldman wrote:
>
>> To give you some data, we've seen all kinds of problems before we started
>> synchronizing ticks with wall clock every ten seconds
>>
>
> How bad were these problems?
>
>
After returning from sleep, subresources were reported to start loading
hours earlier.

I did a bunch of reading on the web about this. It looks like Windows is the
> only platform that keeps track of TimeTicks while the machine is suspended.
> Linux will eventually support it, but only after CLOCK_BOOTTIME is
> implemented
> (Android already has something similar). I couldn't find any solution for
> Mac.
>
> Based on this, I think we're better off using TimeTicks instead of
> introducing a
> new type. It seems there's already a difference in behavior between
> platforms.
> We may as well just pick one and try to converge on that. Any hacks that I
> would've put in MonotonicTime are probably better placed in the
> platform-specific TimeTicks implementations.
>
>
This is fine with me.


> I'm not convinced that using elapsed realtime is best though. For graphics,
> games, and animation, having TimeTicks sleep seems better. For network, I
> could
> argue it either way. I'm tempted to say that having TimeTicks sleep is more
> useful to a site-developer than having it tick while nothing is happening.
>
>
http://codereview.chromium.**org/7602023/<http://codereview.chromium.org/7602...
>

Last time I checked, WebTiming was about network. Has that changed? Are we
thinking of the more broad infrastructure for web-facing client-side
instrumentation?

-
Pavel

[James Simonsen, 2011-08-25 17:40:30]

On 2011/08/25 07:19:25, pfeldman wrote:
> On Thu, Aug 25, 2011 at 9:11 AM, <mailto:simonjam@chromium.org> wrote:
> 
> > On 2011/08/10 06:37:29, pfeldman wrote:
> >
> >> To give you some data, we've seen all kinds of problems before we started
> >> synchronizing ticks with wall clock every ten seconds
> >>
> >
> > How bad were these problems?
> >
> >
> After returning from sleep, subresources were reported to start loading
> hours earlier.

Do you remember which platform was worst? I can experiment with that and try to
get a fix in TimeTicks.

I'm somewhat hopeful that it won't be as bad of a problem here, since we're now
only using TimeTicks and not mixing TimeTicks with Time (once base_time is
removed, anyway).

> Last time I checked, WebTiming was about network. Has that changed? Are we
> thinking of the more broad infrastructure for web-facing client-side
> instrumentation?

Yeah, we have a lot of people that plan to use the same definition of time. Some
are going to add-on to the Web Timing's Performance Timeline too.

- RequestAnimationFrame's callback will report it with the animation frame.
- window.performance.now() will return it to JavaScript.
- User Timing will allow developers to record it in a timeline view for any
purpose.
- The GPU+WebGL guys are going to add new Web Timing specs for their interfaces.

All of these are going to follow the same definition: monotonically increasing
time since the root document navigation.

If you're interested, most of the discussion has been on public-web-perf. And
there's been an e-mail or two to whatwg as well.

[James Simonsen, 2011-08-25 22:01:05]

This is ready for review now. It's not a big change, but it's spread across many
directories, so I need bunch of LGTMs:

pfeldman - main review
darin - base, content/common, ipc, webkit
wtc - chrome/browser/net

If any of you are overloaded, let me know and I can find an alternate.

[pfeldman, 2011-08-26 15:46:15]

> Yeah, we have a lot of people that plan to use the same definition of time.
Some
> are going to add-on to the Web Timing's Performance Timeline too.
> 
> - RequestAnimationFrame's callback will report it with the animation frame.
> - window.performance.now() will return it to JavaScript.
> - User Timing will allow developers to record it in a timeline view for any
> purpose.
> - The GPU+WebGL guys are going to add new Web Timing specs for their
interfaces.
> 
> All of these are going to follow the same definition: monotonically increasing
> time since the root document navigation.
> 
> If you're interested, most of the discussion has been on public-web-perf. And
> there's been an e-mail or two to whatwg as well.

I wonder how much of this should be web-facing. I.e. whether
RequestAnimationFrame wants to send collected data to the server. How much of it
might serve the fingerprinting needs?

As owners of the DevTools / WebKit instrumentation, we receive similar requests.
In our case, developers want to visualize this data and persist / compare it
during the development, but not while running in the wild. We are using
InspectorInstrumentation and Timeline panel for that. If we decide to implement
any of these web-facing APIs for performance, we should do that on the inspector
basis, reusing existing terms. As a result, Timeline and window.performance will
operate the same terms, will be based on the same data.

[pfeldman, 2011-08-26 16:08:00]

Time -> Ticks and Inspector / WebTiming parts LGTM.

I am leaving for vacations tomorrow, so please check with vsevik@ upon landing.

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:632: client_->didFinishLoading(
What is your plan on landing this? Are you going to coordinate the two-sided
landing? It seems like landing either of upstream/downstream patches first will
break the inspector timing.

If this is a coordinated landing, should we convert from double to integer for
the finishLoading and requestTime values upstream?

[James Simonsen, 2011-08-26 18:53:57]

On 2011/08/26 15:46:15, pfeldman wrote:
> I wonder how much of this should be web-facing. I.e. whether
> RequestAnimationFrame wants to send collected data to the server. How much of
it
> might serve the fingerprinting needs?

It's true. Apple is worried about fingerprinting too. At least with Resource
Timing, security & privacy basically concluded you can't do any new
fingerprinting attacks, since you could already use the same technique with load
events. Exposing a few extra values didn't change the attack vector.

For RAF, I don't know if we've approached security & privacy yet. I will check
that with JamesR when he's back. I'll definitely make sure we do it for any
Performance Timeline work too. Next up is User Timing. I'll get that review
started.

> As owners of the DevTools / WebKit instrumentation, we receive similar
requests.
> In our case, developers want to visualize this data and persist / compare it
> during the development, but not while running in the wild. We are using
> InspectorInstrumentation and Timeline panel for that. If we decide to
implement
> any of these web-facing APIs for performance, we should do that on the
inspector
> basis, reusing existing terms. As a result, Timeline and window.performance
will
> operate the same terms, will be based on the same data.

These are good points.

I definitely agree that we should all be sharing the same infrastructure. Web
Timing is already reusing the data you plumbed through for Inspector. We might
want to circle back and refactor this in WebCore so we can share even more.

We should also loop in nduca@ about graphics debugging. They've already
implemented their own timeline in about:gpu. We might want to work towards
getting that in inspector.

To be honest, we haven't started spec'ing the graphics timeline yet. It's just
an idea right now. I don't know what use cases they have in mind. I know one was
debugging slow frame frames (hitches). Developers could ping back the detailed
timing for a slow frame and try to figure out why it was slow on a particular
machine. Again, ncuda@ would be a good starting point. We should probably have
this discussion in a different forum though.

[James Simonsen, 2011-08-26 18:57:18]

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:632: client_->didFinishLoading(
On 2011/08/26 16:08:01, pfeldman wrote:
> What is your plan on landing this? Are you going to coordinate the two-sided
> landing? It seems like landing either of upstream/downstream patches first
will
> break the inspector timing.

I plan to make a best-effort coordinated landing. Basically, this change doesn't
break any tests, it only breaks the reported values if someone is running
chrome. So, I was just going to watch for the webkit roll and land the chrome
change about the same time. Hopefully, only a developer or two will sync then
and they'll never notice it's off.

> If this is a coordinated landing, should we convert from double to integer for
> the finishLoading and requestTime values upstream?

Darin Adler wanted monotonicallyIncreasingTime() to be in doubles, so I think we
should have it match up with that.

[pfeldman, 2011-08-26 20:45:34]

> I plan to make a best-effort coordinated landing. Basically, this change
doesn't
> break any tests, it only breaks the reported values if someone is running
> chrome.

It is true that we don't have network timing tests upstream (there is a fake
stack that is not producing the timing data). But we do have a sanity test for
timing downstream. It is integration InProcessBrowserTest that opens devtools
window and captures network info:
http://codesearch.google.com/#OAMlx_jo-ck/src/chrome/browser/debugger/devtool...,
http://codesearch.google.com/#OAMlx_jo-ck/src/third_party/WebKit/Source/WebKi....

It is only measuring deltas, so it might pass upon the roll with no downstream
landing.

> So, I was just going to watch for the webkit roll and land the chrome
> change about the same time. Hopefully, only a developer or two will sync then
> and they'll never notice it's off.
> 

Sounds good.

> Darin Adler wanted monotonicallyIncreasingTime() to be in doubles, so I think
we
> should have it match up with that.

If Darin Adler wants it in doubles, let it be in doubles - it'll be easier for
me to migrate to it in the InspectorTimlineAgent.

[wtc, 2011-08-26 21:55:41]

I suggest that you make darin or brettw take a quick look at
this.  I believe there is a better way to serialize and
deserialize base::TimeTicks (my second comment below), and
we need to make sure all the deserialized TimeTicks values
have the same time origin (my first comment below).

http://codereview.chromium.org/7602023/diff/16001/base/time.h
File base/time.h (right):

http://codereview.chromium.org/7602023/diff/16001/base/time.h#newcode500
base/time.h:500: }
It is safe to deserialize this way only if all the int64
ticks values have the same "origin" (time 0), which is
probably true only if they come from the same process.

Also, this static method is equivalent to the protected
constructor on lines 555-556 below.  Why don't you just
make that constructor public?

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/resource_loader...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/resource_loader...
webkit/glue/resource_loader_bridge.h:60: base::Time base_time;
It is strange that we can use base::Time here but cannot
use base::TimeTicks here.

It seems that the proper solution is to add
struct ParamTraits<base::TimeTicks> to
ipc/ipc_message_utils.h, which will allow you to
define base_ticks as base::TimeTicks here.

[James Simonsen, 2011-08-26 22:34:29]

http://codereview.chromium.org/7602023/diff/16001/base/time.h
File base/time.h (right):

http://codereview.chromium.org/7602023/diff/16001/base/time.h#newcode500
base/time.h:500: }
On 2011/08/26 21:55:41, wtc wrote:
> It is safe to deserialize this way only if all the int64
> ticks values have the same "origin" (time 0), which is
> probably true only if they come from the same process.

Pavel and I discussed this earlier. All of the platforms use a system-wide clock
for TimeTicks. So, all processes use the same origin (roughly time since last
reboot).

> Also, this static method is equivalent to the protected
> constructor on lines 555-556 below.  Why don't you just
> make that constructor public?

I copied it from Time. I've now copied the comment that went with that one too.
I don't have an opinion either way though, so if you'd like TimeTicks to have a
different interface than Time, let me know.

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/resource_loader...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7602023/diff/16001/webkit/glue/resource_loader...
webkit/glue/resource_loader_bridge.h:60: base::Time base_time;
On 2011/08/26 21:55:41, wtc wrote:
> It is strange that we can use base::Time here but cannot
> use base::TimeTicks here.
> 
> It seems that the proper solution is to add
> struct ParamTraits<base::TimeTicks> to
> ipc/ipc_message_utils.h, which will allow you to
> define base_ticks as base::TimeTicks here.

Indeed. And I'd even already done the IPC work. :/

[wtc, 2011-08-26 23:39:15]

LGTM on chrome/browser/net.

http://codereview.chromium.org/7602023/diff/16001/base/time.h
File base/time.h (right):

http://codereview.chromium.org/7602023/diff/16001/base/time.h#newcode500
base/time.h:500: }
On 2011/08/26 22:34:29, James Simonsen wrote:
>
> Pavel and I discussed this earlier. All of the platforms use a system-wide
clock
> for TimeTicks. So, all processes use the same origin (roughly time since last
> reboot).

If you are relying on this fact, and are deserializing
TimeTicks values that come from different processes,
you need to document this property in the class TimeTicks.

> ... if you'd like TimeTicks to have a
> different interface than Time, let me know.

I see.  What you have is fine.

http://codereview.chromium.org/7602023/diff/22002/webkit/glue/resource_loader...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7602023/diff/22002/webkit/glue/resource_loader...
webkit/glue/resource_loader_bridge.h:56: // this base tick count.
If the values are offsets wrt base_ticks, the values should
ideally have the type base::TimeDelta.  But it may be a
risky change to make quickly.

http://codereview.chromium.org/7602023/diff/22002/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/22002/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:175:
static_cast<double>(base::Time::kMicrosecondsPerSecond));
Perhaps we should convert the base::TimeTicks to a
base::TimeDelta and then call its InSecondsF() method.

Same for lines 632-634.

[James Simonsen, 2011-08-29 21:07:17]

Thanks for the review Wan-Teh.

Darin, can you take a look now?

On 2011/08/26 23:39:15, wtc wrote:
> If you are relying on this fact, and are deserializing
> TimeTicks values that come from different processes,
> you need to document this property in the class TimeTicks.

Good point. Done.

http://codereview.chromium.org/7602023/diff/22002/webkit/glue/resource_loader...
> webkit/glue/resource_loader_bridge.h:56: // this base tick count.
> If the values are offsets wrt base_ticks, the values should
> ideally have the type base::TimeDelta.  But it may be a
> risky change to make quickly.

I tried this out, but wasn't too happy with the results. The biggest ugliness
was adding a conditional to set the values to -1 if the TimeDelta isn't set for
each of the values. My inclination is to just leave it the way it is.

[wtc, 2011-08-29 21:59:40]

simonjam: thanks for trying my suggestion.

Please make sure int32 has enough range (2^31 ticks) for
the various times you are interested in.  Since a tick is
one microsecond, 2^31 ticks is about two thousand seconds.
Is that long enough?

http://codereview.chromium.org/7602023/diff/26001/base/time.h
File base/time.h (right):

http://codereview.chromium.org/7602023/diff/26001/base/time.h#newcode468
base/time.h:468: // across processes.
Nit: this comment is a little unclear.  You can perhaps say
that time origin (time 0) should be the same across processes,
and give the system boot time as an example of time 0.

The reason I asked to have darin or brettw look at this is
to ensure that this is a requirement that we can commit to.

[darin (slow to review), 2011-08-29 23:42:56]

http://codereview.chromium.org/7602023/diff/26001/content/common/resource_mes...
File content/common/resource_messages.h (right):

http://codereview.chromium.org/7602023/diff/26001/content/common/resource_mes...
content/common/resource_messages.h:150: base::TimeTicks /* completion_time */)
Are you sure it makes sense to send TimeTicks over IPC?  Do all processes really
have the same tick source?

[James Simonsen, 2011-08-30 00:09:31]

http://codereview.chromium.org/7602023/diff/26001/content/common/resource_mes...
File content/common/resource_messages.h (right):

http://codereview.chromium.org/7602023/diff/26001/content/common/resource_mes...
content/common/resource_messages.h:150: base::TimeTicks /* completion_time */)
On 2011/08/29 23:42:56, darin wrote:
> Are you sure it makes sense to send TimeTicks over IPC?  Do all processes
really
> have the same tick source?

Yep. This has been a popular question in this code review. :)

All the platforms implement this with some sort of "system time" that's
implemented in the kernel and accessed with a system call. They all count time
since the OS booted.

Here are the some of my sources for each platform:

timeGetTime() - http://msdn.microsoft.com/en-us/library/dd743637(v=VS.85).aspx

clock_gettime() -
http://stackoverflow.com/questions/4943733/is-clock-monotonic-process-or-thre...

mach_absolute_time() -
http://google.com/codesearch#pFm0LxzAWvs/darwinsource/tarballs/apsl/xnu-792.t...

There is one discrepancy across platforms wrt suspending the machine. Mac and
Linux stop ticking while suspended. Windows continues to tick. We should
probably figure out how to normalize that across platforms, but it doesn't
actually interfere with anything here.

[darin (slow to review), 2011-08-30 06:13:40]

On Mon, Aug 29, 2011 at 5:09 PM, <simonjam@chromium.org> wrote:

>
> http://codereview.chromium.**org/7602023/diff/26001/**
>
content/common/resource_**messages.h<http://codereview.chromium.org/7602023/diff/26001/content/common/resource_messages.h>
> File content/common/resource_**messages.h (right):
>
> http://codereview.chromium.**org/7602023/diff/26001/**
>
content/common/resource_**messages.h#newcode150<http://codereview.chromium.org/7602023/diff/26001/content/common/resource_messages.h#newcode150>
> content/common/resource_**messages.h:150: base::TimeTicks /*
> completion_time */)
> On 2011/08/29 23:42:56, darin wrote:
>
>> Are you sure it makes sense to send TimeTicks over IPC?  Do all
>>
> processes really
>
>> have the same tick source?
>>
>
> Yep. This has been a popular question in this code review. :)
>
> All the platforms implement this with some sort of "system time" that's
> implemented in the kernel and accessed with a system call. They all
> count time since the OS booted.
>
>
Sorry to ask the same question.  I thought that Windows timeGetTime was
implemented by sampling the address space of the local process.  It was once
explained to me that timeGetTime just reads three numbers, and checks that
they are monotonically increasing relative to one another.  If not, then it
reads them again.  Then it takes the middle value as the current tick count.
 This makes timeGetTime super cheap to call because it does not involve a
system call.  Have you disassembled the implementation?

If I'm correct, then it seems like different processes could have different
ideas of what the tick counter is.

It is interesting that timeBeginPeriod effects the system globally.  I
believe that controls an interrupt from which those three magic numbers are
updated in each Windows process.

Here's some info that says a bit more about how it is implemented:
http://www.winvistatips.com/timegettime-versus-kequeryinterrupttime-t193534.html

I can't tell if is reading memory in the process' address space or if it is
reading some shared memory, which all processes see.  Still, I'm pretty sure
there is no locking, so different CPUs would see different time values.



> Here are the some of my sources for each platform:
>
> timeGetTime() -
>
http://msdn.microsoft.com/en-**us/library/dd743637(v=VS.85).**aspx<http://msd...
>
> clock_gettime() -
> http://stackoverflow.com/**questions/4943733/is-clock-**
>
monotonic-process-or-thread-**specific<http://stackoverflow.com/questions/4943733/is-clock-monotonic-process-or-thread-specific>
>
> mach_absolute_time() -
> http://google.com/codesearch#**pFm0LxzAWvs/darwinsource/**
> tarballs/apsl/xnu-792.tar.gz%**7CO_WJd_UyLFk/xnu-792/osfmk/**
>
i386/rtclock.c&type=cs&l=1418<http://google.com/codesearch#pFm0LxzAWvs/darwinsource/tarballs/apsl/xnu-792.tar.gz%7CO_WJd_UyLFk/xnu-792/osfmk/i386/rtclock.c&type=cs&l=1418>


I see a comment in the implementation of rtc_nanotime_read that says "this
is a per-cpu structure hence no locking."  I'm not quite sure what that is
referring to.

My guess is that assuming that each process sees the same tick counter is
incorrect.  It is at least incorrect on multi-core systems.

-Darin



>
>
> There is one discrepancy across platforms wrt suspending the machine.
> Mac and Linux stop ticking while suspended. Windows continues to tick.
> We should probably figure out how to normalize that across platforms,
> but it doesn't actually interfere with anything here.
>
>
>
http://codereview.chromium.**org/7602023/<http://codereview.chromium.org/7602...
>

[James Simonsen, 2011-08-31 18:35:48]

On 2011/08/30 06:13:40, darin wrote:
> Sorry to ask the same question.  I thought that Windows timeGetTime was
> implemented by sampling the address space of the local process.  It was once
> explained to me that timeGetTime just reads three numbers, and checks that
> they are monotonically increasing relative to one another.  If not, then it
> reads them again.  Then it takes the middle value as the current tick count.
>  This makes timeGetTime super cheap to call because it does not involve a
> system call.  Have you disassembled the implementation?
> 
> If I'm correct, then it seems like different processes could have different
> ideas of what the tick counter is.

Indeed. The docs weren't telling the whole story.

I've added an IPC test to demonstrate this. Mac and Linux both pass, even with
multiple processors. Windows fails with timeGetTime(). It passes with
HighResNow() or GetTickCount().

To solve the problem, I introduced a new time class: TimeMonotonic. On Windows,
it uses HighResNow() or GetTickCount(). Basically, I don't want to burden
existing TimeTicks users with the extra overhead and I want to make sure we're
type-safe when passing TimeMonotonic across processes. A new class seemed to be
the best solution.

[wtc, 2011-08-31 19:04:02]

TimeTicks is also supposed to be monotonic increasing.
What the new class provides is a monotonic clock with the
same time origin across processes.  I hope we can come up
with a short name that captures that property.

Alternatively, maybe we just need a new static method of
TimeTicks that returns a this new cross-process monotonic
clock.  But I am a little worried that using the same
TimeTicks class with two time origins may be confusing.

[darin (slow to review), 2011-08-31 19:29:16]

On Wed, Aug 31, 2011 at 11:35 AM, <simonjam@chromium.org> wrote:

> On 2011/08/30 06:13:40, darin wrote:
>
>> Sorry to ask the same question.  I thought that Windows timeGetTime was
>> implemented by sampling the address space of the local process.  It was
>> once
>> explained to me that timeGetTime just reads three numbers, and checks that
>> they are monotonically increasing relative to one another.  If not, then
>> it
>> reads them again.  Then it takes the middle value as the current tick
>> count.
>>  This makes timeGetTime super cheap to call because it does not involve a
>> system call.  Have you disassembled the implementation?
>>
>
>  If I'm correct, then it seems like different processes could have
>> different
>> ideas of what the tick counter is.
>>
>
> Indeed. The docs weren't telling the whole story.
>
> I've added an IPC test to demonstrate this. Mac and Linux both pass, even
> with
> multiple processors. Windows fails with timeGetTime(). It passes with
> HighResNow() or GetTickCount().
>
> To solve the problem, I introduced a new time class: TimeMonotonic. On
> Windows,
> it uses HighResNow() or GetTickCount(). Basically, I don't want to burden
> existing TimeTicks users with the extra overhead and I want to make sure
> we're
> type-safe when passing TimeMonotonic across processes. A new class seemed
> to be
> the best solution.
>
>
HighResNow() and GetTickCount() both suffer from problems.

HighResNow cannot be used in any production code because it is not
implemented properly
in some multi-processor machines.  Different cores see different values, or
something like
that, and so HighResNow is not strictly monotonic.  It is only useful for
profiling on machines
that do not have a busted implementation.

GetTickCount() suffers from having poor resolution (16 msec).  timeGetTime()
in contrast can
achieve 1 msec resolution, assuming timeBeginPeriod(1) was called by some
process on the
system.

-Darin



>
>
http://codereview.chromium.**org/7602023/<http://codereview.chromium.org/7602...
>

[darin (slow to review), 2011-08-31 19:30:11]

On Wed, Aug 31, 2011 at 12:29 PM, Darin Fisher <darin@chromium.org> wrote:

>
>
> On Wed, Aug 31, 2011 at 11:35 AM, <simonjam@chromium.org> wrote:
>
>> On 2011/08/30 06:13:40, darin wrote:
>>
>>> Sorry to ask the same question.  I thought that Windows timeGetTime was
>>> implemented by sampling the address space of the local process.  It was
>>> once
>>> explained to me that timeGetTime just reads three numbers, and checks
>>> that
>>> they are monotonically increasing relative to one another.  If not, then
>>> it
>>> reads them again.  Then it takes the middle value as the current tick
>>> count.
>>>  This makes timeGetTime super cheap to call because it does not involve a
>>> system call.  Have you disassembled the implementation?
>>>
>>
>>  If I'm correct, then it seems like different processes could have
>>> different
>>> ideas of what the tick counter is.
>>>
>>
>> Indeed. The docs weren't telling the whole story.
>>
>> I've added an IPC test to demonstrate this. Mac and Linux both pass, even
>> with
>> multiple processors. Windows fails with timeGetTime(). It passes with
>> HighResNow() or GetTickCount().
>>
>> To solve the problem, I introduced a new time class: TimeMonotonic. On
>> Windows,
>> it uses HighResNow() or GetTickCount(). Basically, I don't want to burden
>> existing TimeTicks users with the extra overhead and I want to make sure
>> we're
>> type-safe when passing TimeMonotonic across processes. A new class seemed
>> to be
>> the best solution.
>>
>>
> HighResNow() and GetTickCount() both suffer from problems.
>
> HighResNow cannot be used in any production code because it is not
> implemented properly
> in some multi-processor machines.  Different cores see different values, or
> something like
> that, and so HighResNow is not strictly monotonic.  It is only useful for
> profiling on machines
> that do not have a busted implementation.
>

HighResNow => QueryPerformanceCounter


>
> GetTickCount() suffers from having poor resolution (16 msec).
>  timeGetTime() in contrast can
> achieve 1 msec resolution, assuming timeBeginPeriod(1) was called by some
> process on the
> system.
>
> -Darin
>
>
>
>>
>>
http://codereview.chromium.**org/7602023/<http://codereview.chromium.org/7602...
>>
>
>

[James Simonsen, 2011-09-07 00:27:37]

Sorry for the slow response, I spent several more hours reading about this and
experimenting on Nat's Windows box in between vacation and sick days.

On 2011/08/31 19:30:11, darin wrote:
> > HighResNow() and GetTickCount() both suffer from problems.
> >
> > HighResNow cannot be used in any production code because it is not
> > implemented properly
> > in some multi-processor machines.  Different cores see different values, or
> > something like
> > that, and so HighResNow is not strictly monotonic.  It is only useful for
> > profiling on machines
> > that do not have a busted implementation.
> >
> 
> HighResNow => QueryPerformanceCounter

This is what Microsoft recommends people use though. [1] The HAL is supposed to
take care of these problems and seems to have improved with time. The only
references I could find to actual problems with QPC are from 2006. Everything
newer is just speculation about problems and references to the past.

We probably need to clamp it to ensure it's monotonic. MS already recommends
this in their discussion about timing in games. [1]

They also recommend pinning to a specific processor to mitigate HAL issues [2],
but that's probably not practical for us. I can't get a sense of how much of an
issue this actually is.

> >
> > GetTickCount() suffers from having poor resolution (16 msec).
> >  timeGetTime() in contrast can
> > achieve 1 msec resolution, assuming timeBeginPeriod(1) was called by some
> > process on the
> > system.

From my experimentation, this resolution is just an illusion. I was seeing
timeGetTime() advancing at different rates between the two processes in
unpredictable ways. Sometimes the difference was in milliseconds. Obviously this
isn't real, so the times reported can't be particularly accurate.
timeBeginPeriod(1) made the skew worse.

The unpredictable skew means the NTP calibration we'd talked about is not a
possibility.

Separately...

It turns out others on Chrome have similar requirements. Scott mentioned wanting
to be able to pass input event times from the browser to the renderer with
sub-millisecond resolution for game input. James and Nat have talked about
wanting to be able to time graphics calls at sub-millisecond resolution and
expose those to Javascript (for performance.now()/RAF and graphics extensions to
Performance Timeline).

So...

It sounds like either way we have to bite the bullet and use QPC. It's the
recommended technique, it has sub-millisecond resolution, and it's supposed to
work across processes. If it really is broken on some hardware, we'll get bug
reports and we can implement better fallback techniques. For now, we can use
GetTickCount(), which has bad resolution, but at least shouldn't break anything.
I think it's comparable to falling back to software rendering when hardware
doesn't work. With both fallbacks, the user gets a crappier experience.

Thoughts?

If all else fails, I can put some hacky skew correction in my code and continue
passing timeGetTime() across processes. It'll be good enough for my network
timing, but I think that's just punting on the actual problem. Eventually
someone else in Chrome will need QPC.

[1] http://msdn.microsoft.com/en-us/library/ee417693%28VS.85%29.aspx

[2] http://msdn.microsoft.com/en-us/library/ms644904(v=vs.85).aspx

[James Simonsen, 2011-10-01 02:17:20]

Now that the code yellow is over, I'd really like to finish this CL up. What's
the verdict on time? Should we try QPC on Windows? Or should I manually tweak my
TimeTicks to make them coherent across processes?

[darin (slow to review), 2011-10-01 04:31:56]

QPC is not reliable on Windows.  It is broken on some architectures.  It is
also expensive to query.  I don't think we should be using it for anything
but profiling in test code.

-Darin

On Fri, Sep 30, 2011 at 7:17 PM, <simonjam@chromium.org> wrote:

> Now that the code yellow is over, I'd really like to finish this CL up.
> What's
> the verdict on time? Should we try QPC on Windows? Or should I manually
> tweak my
> TimeTicks to make them coherent across processes?
>
>
http://codereview.chromium.**org/7602023/<http://codereview.chromium.org/7602...
>

[James Simonsen, 2011-10-04 21:08:04]

Okay, I rewrote it to fix skew in the renderer. Basically, we take timestamps on
both sides of the IPC when sending and receiving messages. Then the browser's
times are scaled to fit within the renderer's range, if they don't already fit.
Please review this.

As I said in my last big e-mail, I think most of the concerns about QPC are
historical. Maybe when I have more time I'll write a field trial to collect
actual data about the timers. At least on the machines at Google, QPC is way
more reliable than timeGetTime().

[darin (slow to review), 2011-10-04 21:57:53]

On Tue, Oct 4, 2011 at 2:08 PM, <simonjam@chromium.org> wrote:

> Okay, I rewrote it to fix skew in the renderer. Basically, we take
> timestamps on
> both sides of the IPC when sending and receiving messages. Then the
> browser's
> times are scaled to fit within the renderer's range, if they don't already
> fit.
> Please review this.
>

Sounds good.  Will review when I have the chance.  (If for some reason I
don't get to this soon, please ping ping ping.)



>
> As I said in my last big e-mail, I think most of the concerns about QPC are
> historical. Maybe when I have more time I'll write a field trial to collect
> actual data about the timers. At least on the machines at Google, QPC is
> way
> more reliable than timeGetTime().
>

Chrome doesn't run, for other reasons, on the hardware where QPC is busted?
QPC is not slow (expensive to call) like it used to be?

-Darin



>
>
http://codereview.chromium.**org/7602023/<http://codereview.chromium.org/7602...
>

[James Simonsen, 2011-10-04 22:20:27]

On Tue, Oct 4, 2011 at 2:57 PM, Darin Fisher <darin@chromium.org> wrote:

> Chrome doesn't run, for other reasons, on the hardware where QPC is busted?
> QPC is not slow (expensive to call) like it used to be?
>

I'd have to dig up my sources again, but basically QPC doesn't go out to the
FSB on all systems. It seems it now prefers to use TSC register if that's
known good and then falls back to the FSB if not. The HAL takes care of all
of this.

One of the broken systems is the early AMD64 chips with power saving. These
may be the same ones that are crashing on illegal instructions with
ffmpeg. Another broken chip is the early AMD dual core. That's probably more
prevalent.

Either way, we can always have fallback options if we know we have a bad
chip. GetTickCount() is reliable. If we went with that, we'd either have to
bump the system resolution (bad for power) or suffer the resolution loss.

Anyway, concrete data would be decisive. I was thinking of some sort of
background thread that runs a quick test when Chrome is idle, then reports
via UMA. I'll put that on my TODO list.

James

[darin (slow to review), 2011-10-06 22:50:46]

I'd really like Jim to review your approach here.

[James Simonsen, 2011-10-11 21:02:59]

Ping.

Jim, Darin would like you to take a look at this. Can you do that when you get a
chance?

[jar (doing other things), 2011-10-12 01:07:38]

The transition to TimeTicks as to get a monotonic clock seems like a good
direction.

This CL has a lot more to it than that however.

I'm not clear as to why you've added a pile of extra arguments (generally two
extra 64bit time argument in numerous calls).  I'm vaguely guessing you're
trying to ship many items around to help normalize things, but I was surprised
to see two whole timer values added to the mix.  

You also do a pile of computation, with no explanation of what you're doing or
why, and that makes the code pretty obscure.  See comments below.

Thanks,

Jim

http://codereview.chromium.org/7602023/diff/40001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_localization_peer_unittest.cc (right):

http://codereview.chromium.org/7602023/diff/40001/chrome/common/extensions/ex...
chrome/common/extensions/extension_localization_peer_unittest.cc:147:
EXPECT_CALL(*original_peer_, OnReceivedResponse(_, _, _));
I don't believe that underscore is a good variable name.  Please switch to
meaningful names.

Are these macros?  What is going on here?

If this is some new naming convention in the test system, then please cite the
reference so I can read about it.

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:763: WebURLLoadTiming PopulateWebURLLoadTiming(
Could you add comments to explain what you are trying to achieve here?

I would expect a lengthy block comment.

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl_unittest.cc (right):

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl_unittest.cc:45: WebURLLoadTiming timing =
RunTest(0, 1, 4, 5, 2, 1);
At a minimum, please define constants rather than listing a series of integers. 

Interfaces like this (with a long series of ints) are generally terrible to
read, and easier to mess up.  Much better is an object with a set() method, or
at least a struct with named slots, so that you can see what you are passing in,
and sure that the order is meaningful.

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl_unittest.cc:50: TEST(PopulateWebURLLoadTiming,
DoubleEndedSkew) {
These test names are not enough to explain what each test is doing.  Please add
a comment explaining the intent of the test.

[James Simonsen, 2011-10-12 02:57:46]

Thanks Jim. You're correct, the extra arguments are to correct for TimeTicks
skewing between processes. I hope the new comment helps explain things.

http://codereview.chromium.org/7602023/diff/40001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_localization_peer_unittest.cc (right):

http://codereview.chromium.org/7602023/diff/40001/chrome/common/extensions/ex...
chrome/common/extensions/extension_localization_peer_unittest.cc:147:
EXPECT_CALL(*original_peer_, OnReceivedResponse(_, _, _));
On 2011/10/12 01:07:38, jar wrote:
> I don't believe that underscore is a good variable name.  Please switch to
> meaningful names.
> 
> Are these macros?  What is going on here?
> 
> If this is some new naming convention in the test system, then please cite the
> reference so I can read about it.

http://code.google.com/p/googlemock/ (or go/gmock internally)

_ is a wildcard matcher. It will match any argument passed in to that function.

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:763: WebURLLoadTiming PopulateWebURLLoadTiming(
On 2011/10/12 01:07:38, jar wrote:
> Could you add comments to explain what you are trying to achieve here?
> 
> I would expect a lengthy block comment.

Done.

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl_unittest.cc (right):

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl_unittest.cc:45: WebURLLoadTiming timing =
RunTest(0, 1, 4, 5, 2, 1);
On 2011/10/12 01:07:38, jar wrote:
> At a minimum, please define constants rather than listing a series of
integers. 
> 
> Interfaces like this (with a long series of ints) are generally terrible to
> read, and easier to mess up.  Much better is an object with a set() method, or
> at least a struct with named slots, so that you can see what you are passing
in,
> and sure that the order is meaningful.

Done.

http://codereview.chromium.org/7602023/diff/40001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl_unittest.cc:50: TEST(PopulateWebURLLoadTiming,
DoubleEndedSkew) {
On 2011/10/12 01:07:38, jar wrote:
> These test names are not enough to explain what each test is doing.  Please
add
> a comment explaining the intent of the test.

Done.

[jar (doing other things), 2011-10-15 02:42:13]

http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param...
File content/common/webkit_param_traits.cc (right):

http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param...
content/common/webkit_param_traits.cc:360: return
nit: per style guide: parethesize combined list, and align with leading paren
like:

return (some_long_condition &&
        another_condition);

http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param...
content/common/webkit_param_traits.cc:371: LogParam(p.mime_type, l);            
  l->append(", ");
nit: one statement per line please.

http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param...
content/common/webkit_param_traits.cc:393: ReadParam(m, iter, &p->name) &&
nit: return format.

http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param...
content/common/webkit_param_traits.cc:406: LogParam(p.desc, l);      
l->append(", ");
nit: one statement per line.

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:767: // practice, they usually differ by a few
ms or less, and never more than 10 ms.
Since a common resolution is 15ms. The drift at any instance is commonly 15ms no
matter what.  I don't see how they could be less (when they differ) unless we're
in a higher-res mode.

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:771: // - |initiation_time|: when this process
started handling the request.
Is this when you *tried* to contact the browser process?  Or is it when you (for
instance) got a click, and started to unload a previous page?

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:782: // before adjustment.
I'm still trying to understand the goal.  Is the goal to have times represented
in the renderer process as having been taken from the browser process
time-space?
What is the comparisons that "need to be consistent" so that I can judge the
value of what you're doing here?

What is the impact of rounding error?  What is the impact of error in the
numerator or denominator?

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:789: int64 lower_bound =
start_time.ToInternalValue();
nit: better might be 
int64 lower_bound = 
    std::min(start_time, initiation_time).ToInternalValue();

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:802: int64 denominator = (end_time -
start_time).ToInternalValue();
Hmm... with a common 15ms granularity, I would assume that it would indeed be
comon that one or both of numerator and denominator are zero.

That said, other common differences will be 15ms and 30ms (min to max).

We do run in higher res modes sometimes, but (I think) always vanilla 15ms while
on battery power (for example).

[James Simonsen, 2011-10-18 23:15:46]

http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param...
File content/common/webkit_param_traits.cc (right):

http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param...
content/common/webkit_param_traits.cc:360: return
On 2011/10/15 02:42:13, jar wrote:
> nit: per style guide: parethesize combined list, and align with leading paren
> like:
> 
> return (some_long_condition &&
>         another_condition);

These aren't my changes. I don't know where they came from. I don't see them in
my tree. Maybe it was a bad merge? Or the tool got confused?

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:767: // practice, they usually differ by a few
ms or less, and never more than 10 ms.
On 2011/10/15 02:42:13, jar wrote:
> Since a common resolution is 15ms. The drift at any instance is commonly 15ms
no
> matter what.  I don't see how they could be less (when they differ) unless
we're
> in a higher-res mode.

We use timeGetTime() on Windows, which attempts to interpolate time to a
sub-millisecond resolution. IMO, it's this interpolation that skews between
processes. Other timers, such as GetTickCount(), only have 15ms resolution and
don't skew. For more info, feel free to read the earlier messages on this code
review.

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:771: // - |initiation_time|: when this process
started handling the request.
On 2011/10/15 02:42:13, jar wrote:
> Is this when you *tried* to contact the browser process?  Or is it when you
(for
> instance) got a click, and started to unload a previous page?

The former. I've updated the comment.

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:782: // before adjustment.
On 2011/10/15 02:42:13, jar wrote:
> I'm still trying to understand the goal.  Is the goal to have times
represented
> in the renderer process as having been taken from the browser process
> time-space?

I think we're talking about the same thing, but I'll rephrase it myself to be
sure...

Network times are recorded in the browser process. They're reported in the
renderer process. We want to make sure that the reported times occur in the
correct order relative to other events on the renderer's timeline.

I've added a better explanation to the code too.

> What is the comparisons that "need to be consistent" so that I can judge the
> value of what you're doing here?

The main use case I'm addressing is Navigation Timing. This W3C spec's goal is
to expose more detailed timing information to site authors via analytics
scripts. It's exposed as window.performance.timing in Javascript. We've had it
in Chrome since ~M11.

The problem is that we've received many bug reports about anomalies. For
instance, a responseEnd occurring before a responseStart. Other browsers have
had similar problems, so the Navigation Timing spec now requires the use of a
monotonic clock.

I've improved the comments to reflect this.

For the record, my primary concern is reporting events monotonically. My
secondary concern is making a reasonable effort to report accurate timing
information. I've tried to do both here.

> 
> What is the impact of rounding error?  What is the impact of error in the
> numerator or denominator?

It's hard to say. We have two independent observers of time that disagree with
each other. It's impossible to say which is right or by how much. Given that, I
don't know how we can quantify error.

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:789: int64 lower_bound =
start_time.ToInternalValue();
On 2011/10/15 02:42:13, jar wrote:
> nit: better might be 
> int64 lower_bound = 
>     std::min(start_time, initiation_time).ToInternalValue();

That has a slightly different meaning.

The current code gives the network time the benefit of the doubt. If it's
already consistent with the renderer, leave it alone. If it's obviously
out-of-order, then we try to fix it.

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:802: int64 denominator = (end_time -
start_time).ToInternalValue();
On 2011/10/15 02:42:13, jar wrote:
> Hmm... with a common 15ms granularity, I would assume that it would indeed be
> comon that one or both of numerator and denominator are zero.
> 
> That said, other common differences will be 15ms and 30ms (min to max).
> 
> We do run in higher res modes sometimes, but (I think) always vanilla 15ms
while
> on battery power (for example).

As above, we do get higher resolution numbers.

Also, keep in mind that initiation_time to callback_time spans the entire HTTP
request & response. For most resources on the internet, that's likely to be many
tens or hundreds of milliseconds.

[James Simonsen, 2011-10-25 02:45:27]

Ping. Jim, could you take another look at my comments?

[James Simonsen, 2011-11-01 18:42:08]

On 2011/10/25 02:45:27, James Simonsen wrote:
> Ping. Jim, could you take another look at my comments?

Ping again.

[jar (doing other things), 2011-11-08 03:42:49]

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:767: // practice, they usually differ by a few
ms or less, and never more than 10 ms.
On 2011/10/18 23:15:47, James Simonsen wrote:
> On 2011/10/15 02:42:13, jar wrote:
> > Since a common resolution is 15ms. The drift at any instance is commonly
15ms
> no
> > matter what.  I don't see how they could be less (when they differ) unless
> we're
> > in a higher-res mode.
> 
> We use timeGetTime() on Windows, which attempts to interpolate time to a
> sub-millisecond resolution. IMO, it's this interpolation that skews between
> processes. Other timers, such as GetTickCount(), only have 15ms resolution and
> don't skew. For more info, feel free to read the earlier messages on this code
> review.

timeGetTime() returns a DWORD, which is in units of milliseconds.  How does that
"interpolate" to a sub-millisecond time??

[James Simonsen, 2011-11-09 01:54:58]

On 2011/11/08 03:42:49, jar wrote:
>
http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
> File webkit/glue/weburlloader_impl.cc (right):
> 
>
http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_im...
> webkit/glue/weburlloader_impl.cc:767: // practice, they usually differ by a
few
> ms or less, and never more than 10 ms.
> On 2011/10/18 23:15:47, James Simonsen wrote:
> > On 2011/10/15 02:42:13, jar wrote:
> > > Since a common resolution is 15ms. The drift at any instance is commonly
> 15ms
> > no
> > > matter what.  I don't see how they could be less (when they differ) unless
> > we're
> > > in a higher-res mode.
> > 
> > We use timeGetTime() on Windows, which attempts to interpolate time to a
> > sub-millisecond resolution. IMO, it's this interpolation that skews between
> > processes. Other timers, such as GetTickCount(), only have 15ms resolution
and
> > don't skew. For more info, feel free to read the earlier messages on this
code
> > review.
> 
> timeGetTime() returns a DWORD, which is in units of milliseconds.  How does
that
> "interpolate" to a sub-millisecond time??

Yeah, you're right. I'm mixing them up again. It's been too long since I read up
on all of these timers. :(

Let's start over with just my observations on a Z600 running Windows:

My IPC test from patch #8 above does this:

1. Process #1 records the current timestamp as A.
2. Process #1 sends an IPC to process #2 requesting the time.
3. Process #2 records the current timestamp as B.
4. Process #2 sends B back to process #1.
5. Process #1 records the current timestamp as C.

The test ASSERTs that A <= B <= C. I also printed out B - (A+C)/2, which ideally
should be 0. Here are my results with each timer:

1. GetTickCount(): Passes. Difference is always 0 except when ticks increments
mid-iteration.

2. timeGetTime(): Fails pretty consistently with 1000 iterations. Difference is
in the range [-7ms, +7ms] and is constant for several dozen iterations, then
jumps to a new value.

3. QueryPerformanceCounter(): Passes. Difference is some small number < 1ms.

Given that we're not sure we can use #3 because it may break on some number of
machines in the wild, our options are #1 and #2. #1 is considered too poor
resolution. So, we're left with #2, which
means we have to compensate for the skew. This is what my CL does.

[James Simonsen, 2011-11-17 05:35:33]

Jim,

At your in-person request, I've used strong typing while converting between time
scales. Please take another look.

[jar (doing other things), 2011-11-19 03:42:32]

Thanks for the changes!!!

With some combination of your simpler code, and my looking at this a few
times... I think I understand what you're doing... and want to argue for a very
similar converter.  I think the key is that the common case is a
process-to-process skew.  As a result, your Convert() should handle that simple
(common) case perfectly.  In other cases, it should just do what it can.

See comments and suggestions below.

WDYT?

http://codereview.chromium.org/7602023/diff/77001/chrome/browser/net/load_tim...
File chrome/browser/net/load_timing_observer.cc (right):

http://codereview.chromium.org/7602023/diff/77001/chrome/browser/net/load_tim...
chrome/browser/net/load_timing_observer.cc:156: record.timing.base_time =
TimeTicksToTime(time);
It seems a bit strange to save this in to formats... both Time and TimeTicks.

http://codereview.chromium.org/7602023/diff/77001/chrome/common/extensions/ex...
File chrome/common/extensions/extension_localization_peer.h (right):

http://codereview.chromium.org/7602023/diff/77001/chrome/common/extensions/ex...
chrome/common/extensions/extension_localization_peer.h:51: const
base::TimeTicks& completion_time);
Generally I like to see TimeTicks used more (it is not local time dependent),
but there are some places that need the local adjusted time.  I'll be curious to
see where this falls.

http://codereview.chromium.org/7602023/diff/77001/chrome/renderer/security_fi...
File chrome/renderer/security_filter_peer.h (right):

http://codereview.chromium.org/7602023/diff/77001/chrome/renderer/security_fi...
chrome/renderer/security_filter_peer.h:50: const base::TimeTicks&
completion_time);
This might be a good place to try out that OVERRIDE keyword, and help prevent
errors.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:351: sync_load_response->net_end_time =
response_time;
Is this meant to be the default, and hence end_time == start_time unless we hear
more?  I'm a bit more used to setting it to TimeTicks() (null) when it is not
yet initialized, so that we can detect that state.

Why is this different?

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:645: loader_, completion_time.ToInternalValue()
/
Given you divide by MicrosecondsPerSecond, shouldn't this be ToMicroseconds()

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:687: OnReceivedResponse(info, response_time,
response_time);
Here again I'm confused about both start_time and end_time being set as
identical.  What am I missing?

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:785: // adjustment.
Given that "drift" is the most likely (and understandable), I'd expect your
translation to perform perfectly if only drift takes place.  I'd expect that
when a shift can accomplish everything, then the net numbers should both be
shifted together, and land nicely between (or touching) the ipc time endpoints.

 Hence I'd expect you to first slide both net times (via shifts) so that they
are both between the ipc times if possible.  If not possible, then I'd expect
both end-point net times to be simply adjusted to match the end-point ipc times
exactly, giving you the desired ordering.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:818: return WebKitMilliseconds(upper_bound);
I'm really wary that this is moving one samples in the browser space around, but
not moving the other sample in the same "shifted" way.  This seems very much at
odds with the belief that the clocks mostly skew (but don't race fast and slow
from microsecond to microsecond).

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:887: }
I think you should use a nice affine map, including a scaling factor, and a
shifting constant.  That means you'll need to also have a member:

int64 offset_;

I think you should first make sure the pair of times have the same center (i.e.,
assume equal ipc transmission latency at for both directions).  You should then
check to see if the target range (gap between lower_bound and upper_bound) is
smaller than the domain gap (between net_start_time and net_end_time).  If they
are too far apart, then you need to scale them down.  In the best (common) case,
you'll get away with just using the offset.

For instance, the Convert() function should probably look like:

covert(int64 value) {
  return offset_ + numerator_ * value / denominator_;
}

The above constructor might then be close to:

int64 target_range = upper_bound - lower_bound;
int64 source_range = net_end_time - net_start_time;
if (source_range < target_range) {
  // We fit!  Just shift the midpoints to match.
  numerator_ = 1;
  denominator_ = 1;  // We get away with no scaling.
  offset_ = ((upper_bound + lower_bound) - (net_end_time + net_start_time)) / 2;
  return;
}
// Set up scaling factors, and then deduce shift.
numerator_ = target_range_;
denominator_ = source_range_;  // We probably need a +1 here.
// Find out what we need to shift by to make this really work.
offset_ = 0;  // Temporary value, to make converter work.
offset_ = lower_bound - Convert(net_start_time);
DCHECK_GE(upper_bound, Convert(net_end_time));
}

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:921: ipc_response_time, net_end_time);
This appears to broaden the interval between lower_boand and upper_bound,
translating a clock-skew into additional network-delay. 

I think you should just go ahead and use ipc_request_time as the lower bound,
and ipc_response_time as the upper bound, and map everything into that interval
with a little affine map.

[James Simonsen, 2011-11-23 07:06:14]

Thanks Jim. I like this a lot better. Please take one last look at it.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:351: sync_load_response->net_end_time =
response_time;
On 2011/11/19 03:42:32, jar wrote:
> Is this meant to be the default, and hence end_time == start_time unless we
hear
> more?  I'm a bit more used to setting it to TimeTicks() (null) when it is not
> yet initialized, so that we can detect that state.
> 
> Why is this different?

This is the default. start_time == end_time and that won't change.

These are data: URLs that we're dealing with. That means there's nothing to
fetch from the network. The body of the URL contains the resource, and we just
have to decode it.

If you look at the implementation for GetInfoFromDataURL(), it sets the request
time and response time to the same Time::Now() value. I'm just doing the same
here for TimeTicks.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:645: loader_, completion_time.ToInternalValue()
/
On 2011/11/19 03:42:32, jar wrote:
> Given you divide by MicrosecondsPerSecond, shouldn't this be ToMicroseconds()

Done.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:687: OnReceivedResponse(info, response_time,
response_time);
On 2011/11/19 03:42:32, jar wrote:
> Here again I'm confused about both start_time and end_time being set as
> identical.  What am I missing?

It's a data: url. There's no downloading.

Theoretically, we could time how long it takes us to parse the url. I'm not sure
it'd be worth it though, since the time is likely insignificant. Either way,
since it'd be a change in behavior, we should do it in a separate CL.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:785: // adjustment.
On 2011/11/19 03:42:32, jar wrote:
> Given that "drift" is the most likely (and understandable), I'd expect your
> translation to perform perfectly if only drift takes place.  I'd expect that
> when a shift can accomplish everything, then the net numbers should both be
> shifted together, and land nicely between (or touching) the ipc time
endpoints.
> 
>  Hence I'd expect you to first slide both net times (via shifts) so that they
> are both between the ipc times if possible.  If not possible, then I'd expect
> both end-point net times to be simply adjusted to match the end-point ipc
times
> exactly, giving you the desired ordering.  

Agreed. This is much better.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:818: return WebKitMilliseconds(upper_bound);
On 2011/11/19 03:42:32, jar wrote:
> I'm really wary that this is moving one samples in the browser space around,
but
> not moving the other sample in the same "shifted" way.  This seems very much
at
> odds with the belief that the clocks mostly skew (but don't race fast and slow
> from microsecond to microsecond).

Agreed. The data backs that up too. It's typically just an offset between
processes, except when one side sees a clock tick, but the other doesn't.

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:887: }
On 2011/11/19 03:42:32, jar wrote:
> I think you should use a nice affine map, including a scaling factor, and a
> shifting constant.  That means you'll need to also have a member:
> 
> int64 offset_;
> 
> I think you should first make sure the pair of times have the same center
(i.e.,
> assume equal ipc transmission latency at for both directions).  You should
then
> check to see if the target range (gap between lower_bound and upper_bound) is
> smaller than the domain gap (between net_start_time and net_end_time).  If
they
> are too far apart, then you need to scale them down.  In the best (common)
case,
> you'll get away with just using the offset.
> 
> For instance, the Convert() function should probably look like:
> 
> covert(int64 value) {
>   return offset_ + numerator_ * value / denominator_;
> }
> 
> The above constructor might then be close to:
> 
> int64 target_range = upper_bound - lower_bound;
> int64 source_range = net_end_time - net_start_time;
> if (source_range < target_range) {
>   // We fit!  Just shift the midpoints to match.
>   numerator_ = 1;
>   denominator_ = 1;  // We get away with no scaling.
>   offset_ = ((upper_bound + lower_bound) - (net_end_time + net_start_time)) /
2;
>   return;
> }
> // Set up scaling factors, and then deduce shift.
> numerator_ = target_range_;
> denominator_ = source_range_;  // We probably need a +1 here.
> // Find out what we need to shift by to make this really work.
> offset_ = 0;  // Temporary value, to make converter work.
> offset_ = lower_bound - Convert(net_start_time);
> DCHECK_GE(upper_bound, Convert(net_end_time));
> }

Done with a few minor tweaks:
- Offset should not be applied to deltas.
- source_range == 0 is handled safely.
- No +1. I'm not sure why you suggested that. Can you clarify?

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:921: ipc_response_time, net_end_time);
On 2011/11/19 03:42:32, jar wrote:
> This appears to broaden the interval between lower_boand and upper_bound,
> translating a clock-skew into additional network-delay. 
> 
> I think you should just go ahead and use ipc_request_time as the lower bound,
> and ipc_response_time as the upper bound, and map everything into that
interval
> with a little affine map.

Done.

[jar (doing other things), 2011-11-23 17:25:04]

I'd suggest you add one tiny unit test, as described below (just 'cause I'm
paranoid), but then LGTM.

Thanks!

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/77001/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:887: }
re: Why did I mention a +1.

This integer math stuff is hard to get right.  I was wary that when I divided,
I'd end up rounding in bad (wrong) direction... and at the time I thought you
might need a little extra wiggle room when you scaled.

To test to be sure this is all working, I'd suggest you test to see that this
stuff works correctly for source intervals:

[0, k]
having to map to intervals:
[0, m]

and try all combinations of values of k and m in, perchance, the range of 0 to
100.  In each test case the DCHECK_GE will ensure that all is as expected.

I'm probably being paranoid... and not doing my math carefully enough... but
having been burned by integer math before... I'd suggest you validate with a
quick run through such examples in your unit test.

Thinking about it now... I'm pretty convinced the math is right, and the
rounding will do the right thing... but the simple unit test wouldn't hurt...
and would make it very clear what the intent was (you might even duplicate the
DCHECK() with an EXPECT() in your test).

On 2011/11/23 07:06:15, James Simonsen wrote:
> On 2011/11/19 03:42:32, jar wrote:
> > I think you should use a nice affine map, including a scaling factor, and a
> > shifting constant.  That means you'll need to also have a member:
> > 
> > int64 offset_;
> > 
> > I think you should first make sure the pair of times have the same center
> (i.e.,
> > assume equal ipc transmission latency at for both directions).  You should
> then
> > check to see if the target range (gap between lower_bound and upper_bound)
is
> > smaller than the domain gap (between net_start_time and net_end_time).  If
> they
> > are too far apart, then you need to scale them down.  In the best (common)
> case,
> > you'll get away with just using the offset.
> > 
> > For instance, the Convert() function should probably look like:
> > 
> > covert(int64 value) {
> >   return offset_ + numerator_ * value / denominator_;
> > }
> > 
> > The above constructor might then be close to:
> > 
> > int64 target_range = upper_bound - lower_bound;
> > int64 source_range = net_end_time - net_start_time;
> > if (source_range < target_range) {
> >   // We fit!  Just shift the midpoints to match.
> >   numerator_ = 1;
> >   denominator_ = 1;  // We get away with no scaling.
> >   offset_ = ((upper_bound + lower_bound) - (net_end_time + net_start_time))
/
> 2;
> >   return;
> > }
> > // Set up scaling factors, and then deduce shift.
> > numerator_ = target_range_;
> > denominator_ = source_range_;  // We probably need a +1 here.
> > // Find out what we need to shift by to make this really work.
> > offset_ = 0;  // Temporary value, to make converter work.
> > offset_ = lower_bound - Convert(net_start_time);
> > DCHECK_GE(upper_bound, Convert(net_end_time));
> > }
> 
> Done with a few minor tweaks:
> - Offset should not be applied to deltas.
> - source_range == 0 is handled safely.
> - No +1. I'm not sure why you suggested that. Can you clarify?

[James Simonsen, 2011-11-30 20:29:27]

On 2011/11/23 17:25:04, jar wrote:
> To test to be sure this is all working, I'd suggest you test to see that this
> stuff works correctly for source intervals:
> 
> [0, k]
> having to map to intervals:
> [0, m]
> 
> and try all combinations of values of k and m in, perchance, the range of 0 to
> 100.  In each test case the DCHECK_GE will ensure that all is as expected.

Done.

Darin: This is back to you for review now.

[James Simonsen, 2011-12-05 20:34:49]

On 2011/11/30 20:29:27, James Simonsen wrote:
> Darin: This is back to you for review now.

Darin: ping

[darin (slow to review), 2011-12-06 01:06:32]

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/resource_loader...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/resource_loader...
webkit/glue/resource_loader_bridge.h:309: const base::TimeTicks& net_start_time,
are you sure you wouldn't want to include these timestamps inside the
ResourceResponseInfo structure?

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:150: const TimeTicks& ipc_request_time,
This file should not know anything about IPC.  Perhaps some of this code should
move to the IPCResourceLoaderBridge level?  Or, maybe you can get by with
renaming these fields?

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:659: loader_, static_cast<double>(
Should TimeDelta have a InMicrosecondsF() method?

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:806: class WebKitDelta {
It might help to factor out this code into a separate file.  This all feels like
code that belongs in src/content/ instead of src/webkit/glue/ as this is all
about multi-process details.

[wtc, 2011-12-06 21:00:53]

Drive-by review comments on Patch Set 15:

I only reviewed the changes in src/net.

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.cc:416: response_info_.request_time = Time::Now();

Since URLRequest has a Start() method, setting the
start_time_ member in the constructor rather than in the
Start() method is confusing.  Perhaps the member should be
named create_time_ instead?

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.h:53: class TimeTicks;

Since the URLRequest class contains a base::TimeTicks member,
it seems that a forward declaration of base::TimeTicks is
not enough.  I suspect that "base/time.h" is already included
by one of the headers above.

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.h:464: }

I highly recommend that you declare the new start_time()
method here, right next to the closely related and easily
confused request_time() method.

It may make sense to name the method start_time_ticks()
to give a hint of the return type.

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.h:820: // When this request was created.

Move or copy this comment to the start_time() method.

[James Simonsen, 2011-12-10 00:21:47]

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request.cc
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.cc:416: response_info_.request_time = Time::Now();
On 2011/12/06 21:00:53, wtc wrote:
> 
> Since URLRequest has a Start() method, setting the
> start_time_ member in the constructor rather than in the
> Start() method is confusing.  Perhaps the member should be
> named create_time_ instead?

Done.

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.h:53: class TimeTicks;
On 2011/12/06 21:00:53, wtc wrote:
> 
> Since the URLRequest class contains a base::TimeTicks member,
> it seems that a forward declaration of base::TimeTicks is
> not enough.  I suspect that "base/time.h" is already included
> by one of the headers above.

Done.

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.h:464: }
On 2011/12/06 21:00:53, wtc wrote:
> 
> I highly recommend that you declare the new start_time()
> method here, right next to the closely related and easily
> confused request_time() method.
> 
> It may make sense to name the method start_time_ticks()
> to give a hint of the return type.

Done.

http://codereview.chromium.org/7602023/diff/89006/net/url_request/url_request...
net/url_request/url_request.h:820: // When this request was created.
On 2011/12/06 21:00:53, wtc wrote:
> 
> Move or copy this comment to the start_time() method.

Done.

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/resource_loader...
File webkit/glue/resource_loader_bridge.h (right):

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/resource_loader...
webkit/glue/resource_loader_bridge.h:309: const base::TimeTicks& net_start_time,
On 2011/12/06 01:06:32, darin wrote:
> are you sure you wouldn't want to include these timestamps inside the
> ResourceResponseInfo structure?

Done.

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/weburlloader_im...
File webkit/glue/weburlloader_impl.cc (right):

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:150: const TimeTicks& ipc_request_time,
On 2011/12/06 01:06:32, darin wrote:
> This file should not know anything about IPC.  Perhaps some of this code
should
> move to the IPCResourceLoaderBridge level?  Or, maybe you can get by with
> renaming these fields?

Yeah, good point. Thanks. I went with the former. It makes a lot more sense and
simplified the code a lot.

http://codereview.chromium.org/7602023/diff/89006/webkit/glue/weburlloader_im...
webkit/glue/weburlloader_impl.cc:659: loader_, static_cast<double>(
On 2011/12/06 01:06:32, darin wrote:
> Should TimeDelta have a InMicrosecondsF() method?

Not sure what I was thinking. This should be in seconds, since that's all WebKit
uses. Fixed now.

[wtc, 2011-12-10 01:02:54]

Drive-by review comments on Patch Set 17:

src/net looks good to me.

http://codereview.chromium.org/7602023/diff/105001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7602023/diff/105001/net/url_request/url_reques...
net/url_request/url_request.h:55: }  // namespace base

If you include "base/time.h", please remove the forward
declarations of Time and TimeTicks.

[darin (slow to review), 2011-12-12 22:25:46]

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
File content/common/inter_process_time_ticks_converter.h (right):

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
content/common/inter_process_time_ticks_converter.h:57: LocalTimeTicks
ConvertMilliseconds(const RemoteTimeTicks& remote_ms);
nit: What do you think about renaming this ToLocalTimeTicks?  That way you are
emphasizing the type names involved, and it should make the conversion a little
more apparent at the callsite.

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
content/common/inter_process_time_ticks_converter.h:61: LocalTimeDelta
ConvertDelta(const RemoteTimeDelta& remote_delta);
nit: ToLocalTimeDelta?

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
content/common/inter_process_time_ticks_converter.h:66: static LocalTimeTicks
ClampTimeInRange(const RemoteTimeTicks& remote_value,
This function feels like a small, orthogonal appendage.  Maybe it could / should
just be a static helper function in the file where it is used or the code should
be inlined where needed?  However, perhaps the downside to doing so would be
that you could no longer unit test it.  Hmm...

If it stays here, then it might be nice to also give it a name like
ToLocalTimeTicks, but add some modifier to convey the fact that it is a "less
good" conversion.

http://codereview.chromium.org/7602023/diff/105001/content/common/resource_di...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/7602023/diff/105001/content/common/resource_di...
content/common/resource_dispatcher.cc:601:
ResourceDispatcher::ConvertBrowserResponseToRendererResponse(
nit: this seems like a good use case for an out-param.  the ResourceResponseInfo
copy constructor generates a fair bit of code.  you could also get away with
just calling this function ToResourceResponseInfo, which is a bit shorter and
perhaps more precise.

http://codereview.chromium.org/7602023/diff/105001/content/common/resource_di...
content/common/resource_dispatcher.cc:644:
ResourceDispatcher::ConvertBrowserCompletionToRendererCompletion(
nit: maybe just shorten this function named down to ToRendererCompletionTime? 
(I think it at least helps to include the word "time" in the name.)

[James Simonsen, 2011-12-12 23:35:33]

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
File content/common/inter_process_time_ticks_converter.h (right):

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
content/common/inter_process_time_ticks_converter.h:57: LocalTimeTicks
ConvertMilliseconds(const RemoteTimeTicks& remote_ms);
On 2011/12/12 22:25:46, darin wrote:
> nit: What do you think about renaming this ToLocalTimeTicks?  That way you are
> emphasizing the type names involved, and it should make the conversion a
little
> more apparent at the callsite.

Agreed. Done.

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
content/common/inter_process_time_ticks_converter.h:61: LocalTimeDelta
ConvertDelta(const RemoteTimeDelta& remote_delta);
On 2011/12/12 22:25:46, darin wrote:
> nit: ToLocalTimeDelta?

Done.

http://codereview.chromium.org/7602023/diff/105001/content/common/inter_proce...
content/common/inter_process_time_ticks_converter.h:66: static LocalTimeTicks
ClampTimeInRange(const RemoteTimeTicks& remote_value,
On 2011/12/12 22:25:46, darin wrote:
> This function feels like a small, orthogonal appendage.  Maybe it could /
should
> just be a static helper function in the file where it is used or the code
should
> be inlined where needed?  However, perhaps the downside to doing so would be
> that you could no longer unit test it.  Hmm...
> 
> If it stays here, then it might be nice to also give it a name like
> ToLocalTimeTicks, but add some modifier to convey the fact that it is a "less
> good" conversion.

It's too simple to worry about unit testing and I want to discourage others from
using it. So, I moved it into ResourceDispatcher::ToRendererCompletionTime.

http://codereview.chromium.org/7602023/diff/105001/content/common/resource_di...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/7602023/diff/105001/content/common/resource_di...
content/common/resource_dispatcher.cc:601:
ResourceDispatcher::ConvertBrowserResponseToRendererResponse(
On 2011/12/12 22:25:46, darin wrote:
> nit: this seems like a good use case for an out-param.  the
ResourceResponseInfo
> copy constructor generates a fair bit of code.  you could also get away with
> just calling this function ToResourceResponseInfo, which is a bit shorter and
> perhaps more precise.

Done.

http://codereview.chromium.org/7602023/diff/105001/content/common/resource_di...
content/common/resource_dispatcher.cc:644:
ResourceDispatcher::ConvertBrowserCompletionToRendererCompletion(
On 2011/12/12 22:25:46, darin wrote:
> nit: maybe just shorten this function named down to ToRendererCompletionTime? 
> (I think it at least helps to include the word "time" in the name.)

Done.

http://codereview.chromium.org/7602023/diff/105001/net/url_request/url_request.h
File net/url_request/url_request.h (right):

http://codereview.chromium.org/7602023/diff/105001/net/url_request/url_reques...
net/url_request/url_request.h:55: }  // namespace base
On 2011/12/10 01:02:54, wtc wrote:
> 
> If you include "base/time.h", please remove the forward
> declarations of Time and TimeTicks.

Done.

Yeah, I saw that right after I uploaded the patch and decided it wasn't worth
uploading another one just to fix that. Oh well.

[darin (slow to review), 2011-12-12 23:56:55]

You probably need to re-upload the patch.  Many of the diffs are showing as
broken for some reason.

http://codereview.chromium.org/7602023/diff/109002/content/common/inter_proce...
File content/common/inter_process_time_ticks_converter_unittest.cc (right):

http://codereview.chromium.org/7602023/diff/109002/content/common/inter_proce...
content/common/inter_process_time_ticks_converter_unittest.cc:206: }  //
namespace webkit_glue
nit: s/webkit_glue/content/

[James Simonsen, 2011-12-13 00:03:56]

Sorry. Didn't notice the AppEngine error message on the console. The patch
should be fixed now.

http://codereview.chromium.org/7602023/diff/109002/content/common/inter_proce...
File content/common/inter_process_time_ticks_converter_unittest.cc (right):

http://codereview.chromium.org/7602023/diff/109002/content/common/inter_proce...
content/common/inter_process_time_ticks_converter_unittest.cc:206: }  //
namespace webkit_glue
On 2011/12/12 23:56:55, darin wrote:
> nit: s/webkit_glue/content/

Done.

[darin (slow to review), 2011-12-13 00:43:25]

LGTM

http://codereview.chromium.org/7602023/diff/113030/content/common/resource_di...
File content/common/resource_dispatcher.cc (right):

http://codereview.chromium.org/7602023/diff/113030/content/common/resource_di...
content/common/resource_dispatcher.cc:648: // TimeTicks::Now() returned to
WebKit. Is it worth trying to cache that?
That would be pretty easy to cache.

[commit-bot: I haz the power, 2011-12-14 23:50:05]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/7602023/120001

[commit-bot: I haz the power, 2011-12-14 23:50:20]

Presubmit check for 7602023-120001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/public/common/webkit_param_traits.cc,content/public/common/resource_response.h

Presubmit checks took 3.4s to calculate.

[commit-bot: I haz the power, 2011-12-14 23:55:24]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/simonjam@chromium.org/7602023/120001

[commit-bot: I haz the power, 2011-12-14 23:55:40]

Presubmit check for 7602023-120001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
content/public/common/webkit_param_traits.cc,content/public/common/resource_response.h

Presubmit checks took 4.0s to calculate.

[James Simonsen, 2011-12-15 00:01:46]

Avi or Joi, can I get approval for content/public/common?

[Jói, 2011-12-15 07:06:17]

LGTM for content/public/