Bug 119958 – Replace currentTime() with monotonicallyIncreasingTime() in WebCore

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Arunprasad Rajkumar]

This bug is a continuation of http://trac.webkit.org/changeset/154201 to replace currentTime() with monotonicallyIncreasingTime() in WebCore.

[Arunprasad Rajkumar]

Created attachment 209006 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 209037 [details]
List of places in WebCore where still currentTime() lives

List of places in WebCore where still currentTime() lives

1. Geolocation(Refer Geolocation.cpp#416)
2. Memory Cache
3. Events(where the DOM time stamp should be in epoch since 01/01/1970,00:00)
4. WorkerRunLoop(mutex needs absolute time - https://bugs.webkit.org/show_bug.cgi?id=114826).
5. Blackberry's platform/network - (we can address this in https://bugs.webkit.org/show_bug.cgi?id=114696).
6. Some of the Inspector Files(I think inspector front-end  don't have access to monotonic time)

Please correct me if my assumptions were wrong.

[Darin Adler]

(In reply to comment #2)
> 2. Memory Cache

This might require some thinking. We don’t want changes to the system clock to affect cache expiration.

> 6. Some of the Inspector Files(I think inspector front-end  don't have access to monotonic time)

That does not sound right. Since the inspector front end is not necessarily on the same computer, it won’t necessarily have access to the same current time either. We don’t intend to require that the two computers have synchronized clocks to make the inspector work.

[Arunprasad Rajkumar]

(In reply to comment #3)
> (In reply to comment #2)
> > 2. Memory Cache
> 
> This might require some thinking. We don’t want changes to the system clock to affect cache expiration.
> 
> > 6. Some of the Inspector Files(I think inspector front-end  don't have access to monotonic time)
> 
> That does not sound right. Since the inspector front end is not necessarily on the same computer, it won’t necessarily have access to the same current time either. We don’t intend to require that the two computers have synchronized clocks to make the inspector work.

Darin, I was about to say "The list of places where we can't change currentTime() to monotonicallyIncreasingTime()" :)

(That attachment includes list of places where currentTime() would live even after all patches gets landed, I didn't mentioned like we have to replace those stuffs :) )

[Darin Adler]

(In reply to comment #4)
> Darin, I was about to say "The list of places where we can't change currentTime() to monotonicallyIncreasingTime()" :)
> 
> (That attachment includes list of places where currentTime() would live even after all patches gets landed, I didn't mentioned like we have to replace those stuffs :) )

Yes, and my point is that given what I can see these two should be changed to use monotonicallyIncreasingTime. I don’t understand the arguments above for why they need to stay on currentTime.

[Arunprasad Rajkumar]

(In reply to comment #5)
> (In reply to comment #4)
> > Darin, I was about to say "The list of places where we can't change currentTime() to monotonicallyIncreasingTime()" :)
> > 
> > (That attachment includes list of places where currentTime() would live even after all patches gets landed, I didn't mentioned like we have to replace those stuffs :) )
> 
> Yes, and my point is that given what I can see these two should be changed to use monotonicallyIncreasingTime. I don’t understand the arguments above for why they need to stay on currentTime.

Sorry for the misinterpretation.

>>2. Memory Cache
Cached Resource Age calculation also includes HTTP response header field "date"(CachedResource::currentAge()).

>>6. Some of the Inspector Files(I think inspector front-end  don't have access to monotonic time)
Frankly I don't know much details about inspector, will check & let you know.

[Arunprasad Rajkumar]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #4)
> > > Darin, I was about to say "The list of places where we can't change currentTime() to monotonicallyIncreasingTime()" :)
> > > 
> > > (That attachment includes list of places where currentTime() would live even after all patches gets landed, I didn't mentioned like we have to replace those stuffs :) )
> > 
> > Yes, and my point is that given what I can see these two should be changed to use monotonicallyIncreasingTime. I don’t understand the arguments above for why they need to stay on currentTime.
> 
> Sorry for the misinterpretation.
> 
> >>2. Memory Cache
> Cached Resource Age calculation also includes HTTP response header field "date"(CachedResource::currentAge()).
> 
> >>6. Some of the Inspector Files(I think inspector front-end  don't have access to monotonic time)
> Frankly I don't know much details about inspector, will check & let you know.

Guys anyone pls review the attached patch? I'm checking the possibilities to change the same for MemoryCache & Inspector related stuffs & if possible will do it as a separate patch.

[Simon Fraser (smfr)]

Comment on attachment 209006 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=209006&action=review

> Source/WTF/wtf/CurrentTime.h:55
> +// NTP or manual adjustments , so it better suits for elapsed time measurement.

"it is better suited for".

[Arunprasad Rajkumar]

Created attachment 209104 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 209105 [details]
Patch

[Arunprasad Rajkumar]

Created attachment 209493 [details]
Patch

[Arunprasad Rajkumar]

(In reply to comment #11)
> Created an attachment (id=209493) [details]
> Patch

It is just a ChangeLog format correction as per rniwa mail.

[Arunprasad Rajkumar]

Created attachment 209790 [details]
Patch

[Alexey Proskuryakov]

Comment on attachment 209790 [details]
Patch

Looks good to me. I don't understand mediaTime changes well, but Eric and Jer can comment if these are wrong.

Waiting a little bit for EWS to start providing results.

[Alexey Proskuryakov]

Comment on attachment 209790 [details]
Patch

Mac WK1 EWS seems to be in a pretty bad shape now.

[Eric Carlson]

(In reply to comment #14)
> (From update of attachment 209790 [details])
> Looks good to me. I don't understand mediaTime changes well, but Eric and Jer can comment if these are wrong.
> 
The media changes look fine.

[WebKit Commit Bot]

Comment on attachment 209790 [details]
Patch

Clearing flags on attachment: 209790

Committed r154706: <http://trac.webkit.org/changeset/154706>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.