Bug 119958 - Replace currentTime() with monotonicallyIncreasingTime() in WebCore
Summary: Replace currentTime() with monotonicallyIncreasingTime() in WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 119785
Blocks: 119761
  Show dependency treegraph
 
Reported: 2013-08-17 12:30 PDT by Arunprasad Rajkumar
Modified: 2013-08-27 13:46 PDT (History)
24 users (show)

See Also:


Attachments
Patch (38.37 KB, patch)
2013-08-17 13:38 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
List of places in WebCore where still currentTime() lives (21.35 KB, text/plain)
2013-08-18 10:44 PDT, Arunprasad Rajkumar
no flags Details
Patch (38.35 KB, patch)
2013-08-19 12:02 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (38.33 KB, patch)
2013-08-19 12:10 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (38.60 KB, patch)
2013-08-23 12:47 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff
Patch (38.68 KB, patch)
2013-08-27 12:12 PDT, Arunprasad Rajkumar
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Arunprasad Rajkumar 2013-08-17 12:30:04 PDT
This bug is a continuation of http://trac.webkit.org/changeset/154201 to replace currentTime() with monotonicallyIncreasingTime() in WebCore.
Comment 1 Arunprasad Rajkumar 2013-08-17 13:38:19 PDT
Created attachment 209006 [details]
Patch
Comment 2 Arunprasad Rajkumar 2013-08-18 10:44:49 PDT
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.
Comment 3 Darin Adler 2013-08-18 19:27:24 PDT
(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.
Comment 4 Arunprasad Rajkumar 2013-08-18 21:11:24 PDT
(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 :) )
Comment 5 Darin Adler 2013-08-18 23:10:45 PDT
(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.
Comment 6 Arunprasad Rajkumar 2013-08-18 23:36:40 PDT
(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.
Comment 7 Arunprasad Rajkumar 2013-08-19 11:07:01 PDT
(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.
Comment 8 Simon Fraser (smfr) 2013-08-19 11:49:27 PDT
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".
Comment 9 Arunprasad Rajkumar 2013-08-19 12:02:54 PDT
Created attachment 209104 [details]
Patch
Comment 10 Arunprasad Rajkumar 2013-08-19 12:10:57 PDT
Created attachment 209105 [details]
Patch
Comment 11 Arunprasad Rajkumar 2013-08-23 12:47:23 PDT
Created attachment 209493 [details]
Patch
Comment 12 Arunprasad Rajkumar 2013-08-23 12:48:18 PDT
(In reply to comment #11)
> Created an attachment (id=209493) [details]
> Patch

It is just a ChangeLog format correction as per rniwa mail.
Comment 13 Arunprasad Rajkumar 2013-08-27 12:12:26 PDT
Created attachment 209790 [details]
Patch
Comment 14 Alexey Proskuryakov 2013-08-27 12:13:53 PDT
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.
Comment 15 Alexey Proskuryakov 2013-08-27 13:22:10 PDT
Comment on attachment 209790 [details]
Patch

Mac WK1 EWS seems to be in a pretty bad shape now.
Comment 16 Eric Carlson 2013-08-27 13:24:57 PDT
(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.
Comment 17 WebKit Commit Bot 2013-08-27 13:46:21 PDT
Comment on attachment 209790 [details]
Patch

Clearing flags on attachment: 209790

Committed r154706: <http://trac.webkit.org/changeset/154706>
Comment 18 WebKit Commit Bot 2013-08-27 13:46:26 PDT
All reviewed patches have been landed.  Closing bug.