This switches timeline agent to use monotonically increasing time, so timeline records are not affected by clock adjustments. Units remain the same, i.e. fractional milliseconds. We may also consider using times since record start.
Created attachment 131217 [details] Patch
Comment on attachment 131217 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131217&action=review > Source/WebCore/inspector/InspectorTimelineAgent.h:160 > + static double timestamp(); > + static double timestamp(double microseconds) { return microseconds * 1000.0; } I am not super excited at this overloading. timestamp() and timestamp(double) are doing different things. Maybe we can rename timestamp(double) to timestampToMilliseconds(double).
Comment on attachment 131217 [details] Patch I guess Kentaro means r-.
Created attachment 131219 [details] Patch
Comment on attachment 131219 [details] Patch Looks OK!
Comment on attachment 131219 [details] Patch Kentaro, could you please avoid reviewing (r+ -ing) inspector changes without knowing the potential implications. This very case requires us negotiating with our clients on partially breaking the timeline contract prior to landing.
Created attachment 131220 [details] Patch
(In reply to comment #6) > (From update of attachment 131219 [details]) knowing the potential implications. This very case requires us negotiating with our clients on partially breaking the timeline contract prior to landing. I'm now using re-basing monotonically increasing ticks so that they match current time as of record start -- so these timestamps may be compared to time provided by other agents (e.g. network), as long as there were no time adjustments or suspension. We probably need to switch to monotonically increasing time in network as well.
Comment on attachment 131220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131220&action=review Looks good with a nit. Could you please add a test? > Source/WebCore/inspector/InspectorTimelineAgent.h:160 > + double timestampFromMicroseconds(double microseconds) { return (microseconds + m_timestampOffset) * 1000.0; } Why is it implemented in header?
View in context: https://bugs.webkit.org/attachment.cgi?id=131220&action=review > Source/WebCore/bindings/v8/ScriptGCEvent.cpp:93 > + s_startTime = WTF::monotonicallyIncreasingTime(); This variable should be per isolate. Could you file a bug on this?
Created attachment 131227 [details] Patch
Committed r110394: <http://trac.webkit.org/changeset/110394>
Comment on attachment 131227 [details] Patch From bug 79401: Ideally this would have used DocumentLoadTiming::convertMonotonicTimeToDocumentTime to have the same base timestamp as resources.
(In reply to comment #8) > (In reply to comment #6) > > (From update of attachment 131219 [details] [details]) > knowing the potential implications. This very case requires us negotiating with our clients on partially breaking the timeline contract prior to landing. > > I'm now using re-basing monotonically increasing ticks so that they match current time as of record start -- so these timestamps may be compared to time provided by other agents (e.g. network), as long as there were no time adjustments or suspension. We probably need to switch to monotonically increasing time in network as well. Network already uses monotonically increasing time and uses DocumentLoadTiming::convertMonotonicTimeToDocumentTime.
Also it would have been nice to search the Inspector bug list for "monotonically" before filing a new bug, or you would have found bug 79401.
> Network already uses monotonically increasing time and uses DocumentLoadTiming::convertMonotonicTimeToDocumentTime. I was just about to comment on this in bug 79401. There must be some confustion there: $ grep -c monotonically InspectorResourceAgent.cpp 0 $ grep -c currentTime InspectorResourceAgent.cpp 9 > "monotonically" before filing a new bug, or you would have found bug 79401. Fair enough (this came as an afterthought, and this is exactly how I found it).
(In reply to comment #16) > > Network already uses monotonically increasing time and uses DocumentLoadTiming::convertMonotonicTimeToDocumentTime. > > I was just about to comment on this in bug 79401. There must be some confustion there: > > $ grep -c monotonically InspectorResourceAgent.cpp > 0 > $ grep -c currentTime InspectorResourceAgent.cpp > 9 You're right. I was seeing the code in InspectorResourceAgent::buildObjectForTiming and got the idea it already used it through out. Looks like it is mixed!