Bug 84912 - High res times should start at 0
: High res times should start at 0
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Platform
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: James Simonsen
http://dvcs.w3.org/hg/webperf/raw-fil...
:
Depends on: 98223
Blocks: 66683 66684 88278
  Show dependency treegraph
 
Reported: 2012-04-25 17:19 PDT by James Simonsen
Modified: 2012-10-10 20:06 PDT (History)
5 users (show)

See Also:


Attachments
Patch (16.07 KB, patch)
2012-10-10 13:34 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (17.49 KB, patch)
2012-10-10 15:46 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch (17.49 KB, patch)
2012-10-10 16:31 PDT, James Simonsen
no flags Details | Formatted Diff | Diff
Patch for landing (18.09 KB, patch)
2012-10-10 18:27 PDT, James Simonsen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Simonsen 2012-04-25 17:19:38 PDT
The spec has changed to define 0 as the value of navigationStart for the document. All times returned by window.performance.webkitNow() should be relative to that.

convertMonotonicTimeToDocumentTime() should automatically subtract off the value of navigationStart.

Note that doing so will break the time values returned by window.performance.timing. These values are supposed to use the monotonic clock, but not rebase their values to 0. To fix this, perhaps we could add a legacy accessor that doesn't subtract out navigationStart. This should be clearly marked as legacy, because all future uses are expected to use convertMonotonicTimeToDocumentTime().
Comment 1 James Simonsen 2012-10-10 13:34:31 PDT
Created attachment 168065 [details]
Patch
Comment 2 Build Bot 2012-10-10 14:17:00 PDT
Comment on attachment 168065 [details]
Patch

Attachment 168065 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14252319
Comment 3 James Simonsen 2012-10-10 15:46:36 PDT
Created attachment 168081 [details]
Patch
Comment 4 James Simonsen 2012-10-10 15:58:33 PDT
I think this is ready for review. This should get us in line with the spec for performance.now() without breaking Navigation Timing. Next, I'll import the W3 test suite for now() and work on unprefixing that.
Comment 5 Build Bot 2012-10-10 16:10:02 PDT
Comment on attachment 168081 [details]
Patch

Attachment 168081 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14259241
Comment 6 James Simonsen 2012-10-10 16:31:18 PDT
Created attachment 168090 [details]
Patch
Comment 7 Tony Gentilcore 2012-10-10 18:09:32 PDT
Comment on attachment 168090 [details]
Patch

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

> Source/WebCore/loader/DocumentLoadTiming.cpp:57
> +double DocumentLoadTiming::convertMonotonicTimeToLegacyDocumentTime(double monotonicTime) const

I'd consider s/Legacy/WallBased/ with a comment that new users should use zero-based. That name is more synergistic with ZeroBasedDocumentTime and better describes what the method does.

> Source/WebCore/loader/DocumentLoadTiming.h:41
> +    double convertMonotonicTimeToZeroBasedDocumentTime(double) const;

You could consider dropping the term "convert" from these methods. The name will be just as descriptive but a little more terse.
Comment 8 James Simonsen 2012-10-10 18:27:28 PDT
Created attachment 168112 [details]
Patch for landing
Comment 9 James Simonsen 2012-10-10 18:27:48 PDT
(In reply to comment #7)
> (From update of attachment 168090 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=168090&action=review
> 
> > Source/WebCore/loader/DocumentLoadTiming.cpp:57
> > +double DocumentLoadTiming::convertMonotonicTimeToLegacyDocumentTime(double monotonicTime) const
> 
> I'd consider s/Legacy/WallBased/ with a comment that new users should use zero-based. That name is more synergistic with ZeroBasedDocumentTime and better describes what the method does.

Went with PseudoWallTime

> 
> > Source/WebCore/loader/DocumentLoadTiming.h:41
> > +    double convertMonotonicTimeToZeroBasedDocumentTime(double) const;
> 
> You could consider dropping the term "convert" from these methods. The name will be just as descriptive but a little more terse.

Done.
Comment 10 WebKit Review Bot 2012-10-10 20:06:23 PDT
Comment on attachment 168112 [details]
Patch for landing

Clearing flags on attachment: 168112

Committed r131001: <http://trac.webkit.org/changeset/131001>
Comment 11 WebKit Review Bot 2012-10-10 20:06:27 PDT
All reviewed patches have been landed.  Closing bug.