Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(207)

Issue 7602023: Use a monotonic clock (TimeTicks) to report network times to WebCore. (Closed)

Created:
9 years, 4 months ago by James Simonsen
Modified:
9 years ago
CC:
chromium-reviews, joi+watch-content_chromium.org, jam, cbentzel+watch_chromium.org, darin-cc_chromium.org, tonyg
Visibility:
Public.

Description

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

Patch Set 1 #

Total comments: 2

Patch Set 2 : Added response_end #

Patch Set 3 : Added MonotonicTime #

Patch Set 4 : Back to TimeTicks #

Patch Set 5 : Fix tests #

Total comments: 7

Patch Set 6 : Pass TimeTicks in ResourceLoadTimingInfo #

Total comments: 2

Patch Set 7 : Added comment #

Total comments: 3

Patch Set 8 : Add IPC test #

Patch Set 9 : Add TimeMonotonic #

Patch Set 10 : Fix skew in webkit_glue. #

Total comments: 8

Patch Set 11 : Add explanation #

Total comments: 16

Patch Set 12 : Improved comments #

Patch Set 13 : Use strong typing #

Total comments: 18

Patch Set 14 : Apply offset to net times #

Patch Set 15 : Add random test #

Total comments: 15

Patch Set 16 : Move to content #

Patch Set 17 : Fix build #

Total comments: 12

Patch Set 18 : Rename functions #

Total comments: 2

Patch Set 19 : Try to upload the patch again #

Patch Set 20 : Fix namespace comment #

Total comments: 1

Patch Set 21 : Sync & Merge #

Unified diffs Side-by-side diffs Delta from patch set Stats (+601 lines, -71 lines) Patch
M chrome/browser/net/load_timing_observer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/extensions/extension_localization_peer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -3 lines 0 comments Download
M chrome/common/extensions/extension_localization_peer.cc View 1 2 3 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M chrome/common/extensions/extension_localization_peer_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 6 chunks +15 lines, -12 lines 0 comments Download
M chrome/renderer/security_filter_peer.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +12 lines, -9 lines 0 comments Download
M chrome/renderer/security_filter_peer.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +6 lines, -5 lines 0 comments Download
M content/browser/renderer_host/async_resource_handler.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 4 chunks +5 lines, -2 lines 0 comments Download
M content/browser/renderer_host/resource_dispatcher_host.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -2 lines 0 comments Download
A content/common/inter_process_time_ticks_converter.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +144 lines, -0 lines 0 comments Download
A content/common/inter_process_time_ticks_converter.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +58 lines, -0 lines 0 comments Download
A content/common/inter_process_time_ticks_converter_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 1 chunk +206 lines, -0 lines 0 comments Download
M content/common/resource_dispatcher.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 6 chunks +17 lines, -3 lines 0 comments Download
M content/common/resource_dispatcher.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 8 chunks +86 lines, -10 lines 0 comments Download
M content/common/resource_dispatcher_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +2 lines, -2 lines 0 comments Download
M content/common/resource_messages.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M content/content_common.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 1 chunk +1 line, -0 lines 0 comments Download
M content/public/common/resource_response.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +4 lines, -0 lines 0 comments Download
M content/public/common/webkit_param_traits.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +4 lines, -0 lines 0 comments Download
M content/test/render_view_fake_resources_test.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +1 line, -1 line 0 comments Download
M net/url_request/url_request.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 4 chunks +6 lines, -4 lines 0 comments Download
M net/url_request/url_request.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +2 lines, -1 line 0 comments Download
M webkit/glue/resource_loader_bridge.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 2 chunks +9 lines, -5 lines 0 comments Download
M webkit/glue/weburlloader_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 7 chunks +8 lines, -6 lines 0 comments Download
M webkit/tools/test_shell/simple_resource_loader_bridge.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 4 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 67 (0 generated)
pfeldman
http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bridge.h File webkit/glue/resource_loader_bridge.h (right): http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bridge.h#newcode56 webkit/glue/resource_loader_bridge.h:56: int64 base_ticks; To clarify: you only need this to ...
9 years, 4 months ago (2011-08-09 18:15:22 UTC) #1
James Simonsen
http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bridge.h File webkit/glue/resource_loader_bridge.h (right): http://codereview.chromium.org/7602023/diff/1/webkit/glue/resource_loader_bridge.h#newcode56 webkit/glue/resource_loader_bridge.h:56: int64 base_ticks; On 2011/08/09 18:15:22, pfeldman wrote: > To ...
9 years, 4 months ago (2011-08-09 18:31:51 UTC) #2
pfeldman
> 1. WebCore records currentTime() and monotonicallyIncreasingTime() (aka > TimeTicks) when the loader is created. ...
9 years, 4 months ago (2011-08-09 19:38:34 UTC) #3
pfeldman
9 years, 4 months ago (2011-08-09 19:38:57 UTC) #4
James Simonsen
On 2011/08/09 19:38:34, pfeldman wrote: > > 1. WebCore records currentTime() and monotonicallyIncreasingTime() (aka > ...
9 years, 4 months ago (2011-08-09 22:56:39 UTC) #5
pfeldman
Technically, finishTime is no problem. We can say that although not in the same struct, ...
9 years, 4 months ago (2011-08-10 06:37:29 UTC) #6
James Simonsen
On 2011/08/10 06:37:29, pfeldman wrote: > Technically, finishTime is no problem. We can say that ...
9 years, 4 months ago (2011-08-11 00:44:37 UTC) #7
pfeldman
Sounds good. Back to the formula, do you still need to pass base_time in the ...
9 years, 4 months ago (2011-08-11 06:19:13 UTC) #8
James Simonsen
On 2011/08/11 06:19:13, pfeldman wrote: > Sounds good. Back to the formula, do you still ...
9 years, 4 months ago (2011-08-12 04:52:33 UTC) #9
James Simonsen
On 2011/08/10 06:37:29, pfeldman wrote: > To give you some data, we've seen all kinds ...
9 years, 4 months ago (2011-08-25 05:11:00 UTC) #10
pfeldman
On Thu, Aug 25, 2011 at 9:11 AM, <simonjam@chromium.org> wrote: > On 2011/08/10 06:37:29, pfeldman ...
9 years, 4 months ago (2011-08-25 07:19:25 UTC) #11
James Simonsen
On 2011/08/25 07:19:25, pfeldman wrote: > On Thu, Aug 25, 2011 at 9:11 AM, <mailto:simonjam@chromium.org> ...
9 years, 4 months ago (2011-08-25 17:40:30 UTC) #12
James Simonsen
This is ready for review now. It's not a big change, but it's spread across ...
9 years, 4 months ago (2011-08-25 22:01:05 UTC) #13
pfeldman
> Yeah, we have a lot of people that plan to use the same definition ...
9 years, 4 months ago (2011-08-26 15:46:15 UTC) #14
pfeldman
Time -> Ticks and Inspector / WebTiming parts LGTM. I am leaving for vacations tomorrow, ...
9 years, 4 months ago (2011-08-26 16:08:00 UTC) #15
James Simonsen
On 2011/08/26 15:46:15, pfeldman wrote: > I wonder how much of this should be web-facing. ...
9 years, 4 months ago (2011-08-26 18:53:57 UTC) #16
James Simonsen
http://codereview.chromium.org/7602023/diff/16001/webkit/glue/weburlloader_impl.cc File webkit/glue/weburlloader_impl.cc (right): http://codereview.chromium.org/7602023/diff/16001/webkit/glue/weburlloader_impl.cc#newcode632 webkit/glue/weburlloader_impl.cc:632: client_->didFinishLoading( On 2011/08/26 16:08:01, pfeldman wrote: > What is ...
9 years, 4 months ago (2011-08-26 18:57:18 UTC) #17
pfeldman
> I plan to make a best-effort coordinated landing. Basically, this change doesn't > break ...
9 years, 4 months ago (2011-08-26 20:45:34 UTC) #18
wtc
I suggest that you make darin or brettw take a quick look at this. I ...
9 years, 4 months ago (2011-08-26 21:55:41 UTC) #19
James Simonsen
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 ...
9 years, 4 months ago (2011-08-26 22:34:29 UTC) #20
wtc
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 ...
9 years, 4 months ago (2011-08-26 23:39:15 UTC) #21
James Simonsen
Thanks for the review Wan-Teh. Darin, can you take a look now? On 2011/08/26 23:39:15, ...
9 years, 3 months ago (2011-08-29 21:07:17 UTC) #22
wtc
simonjam: thanks for trying my suggestion. Please make sure int32 has enough range (2^31 ticks) ...
9 years, 3 months ago (2011-08-29 21:59:40 UTC) #23
darin (slow to review)
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 content/common/resource_messages.h:150: base::TimeTicks /* completion_time */) Are you sure it makes ...
9 years, 3 months ago (2011-08-29 23:42:56 UTC) #24
James Simonsen
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 content/common/resource_messages.h:150: base::TimeTicks /* completion_time */) On 2011/08/29 23:42:56, darin wrote: ...
9 years, 3 months ago (2011-08-30 00:09:31 UTC) #25
darin (slow to review)
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> ...
9 years, 3 months ago (2011-08-30 06:13:40 UTC) #26
James Simonsen
On 2011/08/30 06:13:40, darin wrote: > Sorry to ask the same question. I thought that ...
9 years, 3 months ago (2011-08-31 18:35:48 UTC) #27
wtc
TimeTicks is also supposed to be monotonic increasing. What the new class provides is a ...
9 years, 3 months ago (2011-08-31 19:04:02 UTC) #28
darin (slow to review)
On Wed, Aug 31, 2011 at 11:35 AM, <simonjam@chromium.org> wrote: > On 2011/08/30 06:13:40, darin ...
9 years, 3 months ago (2011-08-31 19:29:16 UTC) #29
darin (slow to review)
On Wed, Aug 31, 2011 at 12:29 PM, Darin Fisher <darin@chromium.org> wrote: > > > ...
9 years, 3 months ago (2011-08-31 19:30:11 UTC) #30
James Simonsen
Sorry for the slow response, I spent several more hours reading about this and experimenting ...
9 years, 3 months ago (2011-09-07 00:27:37 UTC) #31
James Simonsen
Now that the code yellow is over, I'd really like to finish this CL up. ...
9 years, 2 months ago (2011-10-01 02:17:20 UTC) #32
darin (slow to review)
QPC is not reliable on Windows. It is broken on some architectures. It is also ...
9 years, 2 months ago (2011-10-01 04:31:56 UTC) #33
James Simonsen
Okay, I rewrote it to fix skew in the renderer. Basically, we take timestamps on ...
9 years, 2 months ago (2011-10-04 21:08:04 UTC) #34
darin (slow to review)
On Tue, Oct 4, 2011 at 2:08 PM, <simonjam@chromium.org> wrote: > Okay, I rewrote it ...
9 years, 2 months ago (2011-10-04 21:57:53 UTC) #35
James Simonsen
On Tue, Oct 4, 2011 at 2:57 PM, Darin Fisher <darin@chromium.org> wrote: > Chrome doesn't ...
9 years, 2 months ago (2011-10-04 22:20:27 UTC) #36
darin (slow to review)
I'd really like Jim to review your approach here.
9 years, 2 months ago (2011-10-06 22:50:46 UTC) #37
James Simonsen
Ping. Jim, Darin would like you to take a look at this. Can you do ...
9 years, 2 months ago (2011-10-11 21:02:59 UTC) #38
jar (doing other things)
The transition to TimeTicks as to get a monotonic clock seems like a good direction. ...
9 years, 2 months ago (2011-10-12 01:07:38 UTC) #39
James Simonsen
Thanks Jim. You're correct, the extra arguments are to correct for TimeTicks skewing between processes. ...
9 years, 2 months ago (2011-10-12 02:57:46 UTC) #40
jar (doing other things)
http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param_traits.cc File content/common/webkit_param_traits.cc (right): http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param_traits.cc#newcode360 content/common/webkit_param_traits.cc:360: return nit: per style guide: parethesize combined list, and ...
9 years, 2 months ago (2011-10-15 02:42:13 UTC) #41
James Simonsen
http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param_traits.cc File content/common/webkit_param_traits.cc (right): http://codereview.chromium.org/7602023/diff/55001/content/common/webkit_param_traits.cc#newcode360 content/common/webkit_param_traits.cc:360: return On 2011/10/15 02:42:13, jar wrote: > nit: per ...
9 years, 2 months ago (2011-10-18 23:15:46 UTC) #42
James Simonsen
Ping. Jim, could you take another look at my comments?
9 years, 2 months ago (2011-10-25 02:45:27 UTC) #43
James Simonsen
On 2011/10/25 02:45:27, James Simonsen wrote: > Ping. Jim, could you take another look at ...
9 years, 1 month ago (2011-11-01 18:42:08 UTC) #44
jar (doing other things)
http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_impl.cc File webkit/glue/weburlloader_impl.cc (right): http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_impl.cc#newcode767 webkit/glue/weburlloader_impl.cc:767: // practice, they usually differ by a few ms ...
9 years, 1 month ago (2011-11-08 03:42:49 UTC) #45
James Simonsen
On 2011/11/08 03:42:49, jar wrote: > http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_impl.cc > File webkit/glue/weburlloader_impl.cc (right): > > http://codereview.chromium.org/7602023/diff/55001/webkit/glue/weburlloader_impl.cc#newcode767 > ...
9 years, 1 month ago (2011-11-09 01:54:58 UTC) #46
James Simonsen
Jim, At your in-person request, I've used strong typing while converting between time scales. Please ...
9 years, 1 month ago (2011-11-17 05:35:33 UTC) #47
jar (doing other things)
Thanks for the changes!!! With some combination of your simpler code, and my looking at ...
9 years, 1 month ago (2011-11-19 03:42:32 UTC) #48
James Simonsen
Thanks Jim. I like this a lot better. Please take one last look at it. ...
9 years, 1 month ago (2011-11-23 07:06:14 UTC) #49
jar (doing other things)
I'd suggest you add one tiny unit test, as described below (just 'cause I'm paranoid), ...
9 years, 1 month ago (2011-11-23 17:25:04 UTC) #50
James Simonsen
On 2011/11/23 17:25:04, jar wrote: > To test to be sure this is all working, ...
9 years ago (2011-11-30 20:29:27 UTC) #51
James Simonsen
On 2011/11/30 20:29:27, James Simonsen wrote: > Darin: This is back to you for review ...
9 years ago (2011-12-05 20:34:49 UTC) #52
darin (slow to review)
http://codereview.chromium.org/7602023/diff/89006/webkit/glue/resource_loader_bridge.h File webkit/glue/resource_loader_bridge.h (right): http://codereview.chromium.org/7602023/diff/89006/webkit/glue/resource_loader_bridge.h#newcode309 webkit/glue/resource_loader_bridge.h:309: const base::TimeTicks& net_start_time, are you sure you wouldn't want ...
9 years ago (2011-12-06 01:06:32 UTC) #53
wtc
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 ...
9 years ago (2011-12-06 21:00:53 UTC) #54
James Simonsen
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.cc#newcode416 net/url_request/url_request.cc:416: response_info_.request_time = Time::Now(); On 2011/12/06 21:00:53, wtc wrote: > ...
9 years ago (2011-12-10 00:21:47 UTC) #55
wtc
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 ...
9 years ago (2011-12-10 01:02:54 UTC) #56
darin (slow to review)
http://codereview.chromium.org/7602023/diff/105001/content/common/inter_process_time_ticks_converter.h File content/common/inter_process_time_ticks_converter.h (right): http://codereview.chromium.org/7602023/diff/105001/content/common/inter_process_time_ticks_converter.h#newcode57 content/common/inter_process_time_ticks_converter.h:57: LocalTimeTicks ConvertMilliseconds(const RemoteTimeTicks& remote_ms); nit: What do you think ...
9 years ago (2011-12-12 22:25:46 UTC) #57
James Simonsen
http://codereview.chromium.org/7602023/diff/105001/content/common/inter_process_time_ticks_converter.h File content/common/inter_process_time_ticks_converter.h (right): http://codereview.chromium.org/7602023/diff/105001/content/common/inter_process_time_ticks_converter.h#newcode57 content/common/inter_process_time_ticks_converter.h:57: LocalTimeTicks ConvertMilliseconds(const RemoteTimeTicks& remote_ms); On 2011/12/12 22:25:46, darin wrote: ...
9 years ago (2011-12-12 23:35:33 UTC) #58
darin (slow to review)
You probably need to re-upload the patch. Many of the diffs are showing as broken ...
9 years ago (2011-12-12 23:56:55 UTC) #59
James Simonsen
Sorry. Didn't notice the AppEngine error message on the console. The patch should be fixed ...
9 years ago (2011-12-13 00:03:56 UTC) #60
darin (slow to review)
LGTM http://codereview.chromium.org/7602023/diff/113030/content/common/resource_dispatcher.cc File content/common/resource_dispatcher.cc (right): http://codereview.chromium.org/7602023/diff/113030/content/common/resource_dispatcher.cc#newcode648 content/common/resource_dispatcher.cc:648: // TimeTicks::Now() returned to WebKit. Is it worth ...
9 years ago (2011-12-13 00:43:25 UTC) #61
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/7602023/120001
9 years ago (2011-12-14 23:50:05 UTC) #62
commit-bot: I haz the power
Presubmit check for 7602023-120001 failed and returned exit status 1. Running presubmit commit checks ...
9 years ago (2011-12-14 23:50:20 UTC) #63
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/simonjam@chromium.org/7602023/120001
9 years ago (2011-12-14 23:55:24 UTC) #64
commit-bot: I haz the power
Presubmit check for 7602023-120001 failed and returned exit status 1. Running presubmit commit checks ...
9 years ago (2011-12-14 23:55:40 UTC) #65
James Simonsen
Avi or Joi, can I get approval for content/public/common?
9 years ago (2011-12-15 00:01:46 UTC) #66
Jói
9 years ago (2011-12-15 07:06:17 UTC) #67
LGTM for content/public/

Powered by Google App Engine
This is Rietveld 408576698