Closed Bug 539095 Opened 14 years ago Closed 12 years ago

Expose high-resolution timing API to web content

Categories

(Core :: DOM: Core & HTML, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: cjones, Assigned: Gijs)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

TBH I don't think the actual implementation of this should be hard; the API design is the difficult bit.  This API can only offer meaningful *intervals*, as opposed to |Date.now()|.  bz tells me that webkit proposed an API for this.

The dependency on bug 539093 is "weak" in that we don't /need/ high-resolution timers for all Tier Is to expose this API ... we /could/ fall back on NSPR millisecond-resolution timers.  But high-resolution timers for all Tier Is would certainly be nice to have before the content API lands.
Hmm ... https://lists.webkit.org/pipermail/webkit-dev/2008-October/005223.html seems to be what bz was referring to.  I actually thought bz was referring to a high-resolution *timing* API, rather than a high-resolution *timer callback* API.  Apologies.

Do we want both of these?  Just the timer callback API?
OK.  The timing API should be "easy", but the callback API is a lot harder because high-resolution wait intervals need to filter down into the places we wait, which IIRC are PR_Poll() and PR_WaitCondVar() (forget which is used on the main thread).  The latter will be covered by bug 507718, but the former ... sigh.
I thought they'd proposed both; I personally am more interested in the high-resolution timing api (which we could use for our performance tests, for example, to reduce noise) than in the high-resolution timers...
Google is failing to turn up a webkit timing proposal for me.  Should we just brainstorm a moz* extension?  Should this be a discussion for m.d.platform/m.d.t.dom/m.d.t.js-engine?

Offhand, I can think of a few possible APIs.

(1)
  double uptimeSecs() (Or Date.uptimeSecs()) --- Number of seconds since something browser-related started up (doesn't really matter what).  Simple and easy to implement.

(2)
  void startTiming(string what)
  double stopTiming(string what) --- Firebug-like API.  Could nest the |what| timings; I don't know what firebug does with |startTiming("foo"); startTiming("foo"); stopTiming("foo");|.

(3)
  class TimeTrial {
    TimeTrial(name="") --- For toString().
    void start()
    double stop() --- Return the number of seconds since the last |start()|, with appropriate error handling.
    double min()
    double max()
    double mean()
    double variance()
    ... --- Stats calculated from all start()/stop() calls.  Maybe getting too far into JS-user-level functionality.
 }

Thoughts?
I think we want to be able to tell when high-resolution timing is required, since IIRC there are power issues, at least on Windows XP, when you're maintaining a high-resolution timer.

How about a Timer object with just

readonly attribute double resolution;
readonly attribute double elapsedSeconds;
void start();
void stop();
void reset();

I think adding statistics is more functionality than we'd want at this layer.
(In reply to comment #6)
> I think we want to be able to tell when high-resolution timing is required,
> since IIRC there are power issues, at least on Windows XP, when you're
> maintaining a high-resolution timer.
> 

I don't know what "is required" would mean here, or how this interface would change if it's not required.  (Not my area of expertise, but IIRC QueryPerformanceCounters is implemented with rdtsc and shouldn't have power issues, should we use a QPC-based TimeStamp implementation.)

> How about a Timer object with just
> 
> readonly attribute double resolution;
> readonly attribute double elapsedSeconds;
> void start();
> void stop();
> void reset();
> 

Why do you prefer this to |uptimeSecs()|?  I'm not sure what value is added with this extra complexity of |Timer|.

> I think adding statistics is more functionality than we'd want at this layer.

Possibly.  But IMHO the value added by doing these trivial statistics outweighs the extra complexity of a stateful (in a JS-state sense) class for timing.
What I meant was that it might be useful for the implementation to know whether we are currently in an interval whose duration is being measured. Maybe it's not.

uptimeSecs() might be adequate. But there is the question of what the epoch should be. A Timer/TimeTrial (don't like the latter name) interface avoids that.
I don't know how to go about deciding on this API.

(In reply to comment #8)
> uptimeSecs() might be adequate. But there is the question of what the epoch
> should be. A Timer/TimeTrial (don't like the latter name) interface avoids
> that.

Yep, but I don't think the epoch is all that important; I had thought of grabbing a TimeStamp during NS_InitXPCOM() or soon thereafter.  I *do* think it's important that the epoch is the same for all processes in the multi-process world, but that's pretty easy to implement.

(In reply to comment #6)
> readonly attribute double resolution;
> readonly attribute double elapsedSeconds;
> void start();
> void stop();
> void reset();

What happens if start() is called multiple times before stop()?  What happens if stop() is called before start()?  What does reset() mean?  What is elapsedSeconds if accessed before start()/stop()?  What is elapsedSeconds if accessed after start() but before stop()?  (What do you think about naming this "Stopwatch" so as not to confuse "Timer" with timer callback APIs?)
(In reply to comment #9)
> (In reply to comment #6)
> > readonly attribute double resolution;
> > readonly attribute double elapsedSeconds;
> > void start();
> > void stop();
> > void reset();
> 
> What happens if start() is called multiple times before stop()?  What happens
> if stop() is called before start()?  What does reset() mean?  What is
> elapsedSeconds if accessed before start()/stop()?  What is elapsedSeconds if
> accessed after start() but before stop()?  (What do you think about naming this
> "Stopwatch" so as not to confuse "Timer" with timer callback APIs?)

Also, does elapsedSeconds accumulate each start()/stop() interval or only the latest?
(In reply to comment #9)
> What happens if start() is called multiple times before stop()?

Nothing.

> What happens if stop() is called before start()?

Nothing.

> What does reset() mean?

Sets elapsedSeconds to 0.

> What is elapsedSeconds if accessed before start()/stop()?

It's 0.

> What is elapsedSeconds if accessed after start() but before stop()?

It returns the current value.

> (What do you think about naming this
> "Stopwatch" so as not to confuse "Timer" with timer callback APIs?)

Sure.

> Also, does elapsedSeconds accumulate each start()/stop() interval or only the
> latest?

All intervals, otherwise reset() would not be needed. An alternative design would be to forget reset() and have start() reset elapsedSeconds to 0.
I agree that it's desirable to not offer a vaguely-defined "seconds since X" API, like uptime().  But also the Stopwatch interface feels a bit restrictive in that it can't be used to compute negative durations of time, as uptime() can.  So:

How about a vastly simplified version of the TimeStamp/TimeDuration API?

  class TimePoint {
    static TimePoint now();

    double secondsSince(TimePoint t); --- this - t
    double secondsUntil(TimePoint t); --- t - this
    double secondsSinceNow();         --- this - now()
    double secondsUntilNow();         --- now() - this
  }

(The name "TimePoint" is taken from the C++1x spec.)

Then a timing would look like

  t = TimePoint.now(); ... ; elapsed = t.secondsUntilNow();

or

  t = TimePoint.now(); ... ; elapsed = TimePoint.now().secondsSince(t);

This would be much nicer with overloaded arithmetic operators, but oh well.  I don't particularly like |plus()|, |minus()|, etc. methods but would be willing to concede them if they seem simpler.

Is this a reasonable compromise?
Hmm ... even simpler is

  class TimePoint {
    static TimePoint now();

    double secondsBefore(TimeStamp t);  --- t - this
    double secondsBeforeNow();          --- now() - this
  }

Nearly equally powerful, and hopefully makes the common case

  t = TimePoint.now(); ... ; elapsed = t.secondsBeforeNow();

clearer.
If this basic API (we may want to add to it later) suits all interested parties, I'd like to suggest a representation of TimePoint.  It's intended to fulfill these desiderata

  - timings of "short-term" events, e.g. JS function execution, have the highest precision possible
  - TimePoints can be sent to a server, read back on any client, and used in a meaningful way
  - timings of "long-term" events, e.g. "how long since gmail was last opened", have the highest precision that can be "guaranteed", but degrade gracefully if the absolute highest precision can't be maintained.  Should be no worse than Date.now() in the worst case

So I propose

  struct TimePoint {
    int64_t base;
    int64_t calib;
    int64_t point;
    int64_t scale;
  }

|base| is a point in the past, represented in milliseconds since the UNIX epoch.
|calib| is a clock-specific calibration point.
|point| is a time stamp only meaningful wrt to |base|, |calib|, and |scale|.  It has the same unit as |calib|.
|scale| is "unit-of-calib-and-point" per milliseconds.

Two different clocks using this representation.  First, Date.now: I want implementations to be able to trivially fall back on Date.now() timing if they don't have high-precision timers (this also makes the API easier to ramp up).

  base: milliseconds since the UNIX epoch (i.e. Date.now()) when TimePoint.now() is called
  calib: 0
  point: 0
  scale: 1

Also possible is

  base: 0
  calib: 0
  point: milliseconds since the UNIX epoch (i.e. Date.now()) when TimePoint.now() is called
  scale: 1

(Many other possibilities too.)

Possibility for POSIX CLOCK_MONOTONIC

  base: milliseconds since the UNIX epoch at some implementation-defined point in time.  For example, Gecko might grab |base| during XPCOM startup, or when TimePoint is first accessed.  (It doesn't really matter when, see below.)
  calib: nanoseconds reported by clock_gettime() at the "same" implementation-defined point as |base| (as close as possible).
  point: nanoseconds reported by clock_gettime() when |this| TimePoint was created
  scale: 1000000 (ns/ms)

Now, the importance of this representation is in computing |TimePoint.secondsBefore(t)|.  Here's an algorithm

  TimePoint.secondsBefore(TimePoint t):
    if this.scale == t.scale and this.base == t.base and this.calib == t.calib:
      // "fast", high-precision path
      return (t.point - this.point) / (scale / 1000)

    // "slow", low-precision path
    absThis = round(this.base + (this.point - this.calib) / this.scale)
    absT = round(t.base + (t.point - t.calib) / t.scale)
    return (absT - absThis) / 1000

If the calibration of the two TimePoints is the same, maximum precision is used.  But if the calibration is different, e.g. Date.now-backed TimePoints compared with CLOCK_MONOTONIC-backed TimePoints or CLOCK_MONOTONIC TimePoints with different calibrations, then the algorithm falls back on millisecond resolution.

We'd probably also want a TimePoint.maximumResolution readonly property too, as Rob proposed for Timer/Stopwatch.  One possible addition to the API might be allowing TimePoints to be created from Dates.

Thoughts?
Why not just reimplement Date this way, and provide some extra methods to get the high-precision data, instead of introducing TimePoint?
Aren't there compatibility problems with JSON'd Dates, e.g.?
I'm not sure what you mean. You mean JSON'ing a Date would lose the high-precision data? OK, but that seems minor and easy to work around.
Or maybe we could provide a PreciseDate object that is (effectively) a subclass of Date and that JSONs better.
(In reply to comment #18)
> I'm not sure what you mean. You mean JSON'ing a Date would lose the
> high-precision data? OK, but that seems minor and easy to work around.

I mean that the current JSON representation of Date will be different than TimePoint/PreciseDate, and I assume that that will break a lot of things.

I'm fine with PreciseDate.
One (potential) annoyance with implementing PreciseDate is that our TimeStamp code isn't accessible from JS.  (I see that Date is implemented in js/src).  It would make me sad to have to slog through the bog of nsIXPCScriptable crud to implement such a simple API.

How about moving the TimeStamp backends into js/time, wrapped using the C++1x time APIs?  We'd probably still want to keep TimeStamp/TimeDuration, but they could just wrap js/time instead.
Oops, meant to CC brendan and gal re: comment 21.
We've talked about having a library of code that can be used by JS and XPCOM.
Yeah, this would be in the same spirit as bug 507718.  In fact, some of the js/thread C++1x API wants the time APIs anyway.
How about adding it to the js/src/threads patch but move all that up to js/threads and we'll start doing the same with other overdeep subdirs of js/src.

I'll review the next rev of the patch for bug 507718 -- I promise!

/be
I'm having second thoughts now about the wisdom of trying to spec and find consensus on something as complicated as the PreciseDate of comment 15.  The harmony Date.prototype.nanoAge() proposal[1] looks OK, although it's a bit underspecified, and it would be pretty easy to implement with our soon-to-be-shared TimeStamp/std::time_point.

Unless anyone has strong feelings for PreciseDate, I think I'd prefer to take the path of less resistance with nanoAge().

[1] http://wiki.ecmascript.org/doku.php?id=proposals:date_and_time&s=nanoage#current_and_elapsed_times
Depends on: 673105
No longer depends on: 539093
(In reply to Chris Jones [:cjones] [:warhammer] from comment #27)
> I'm having second thoughts now about the wisdom of trying to spec and find
> consensus on something as complicated as the PreciseDate of comment 15.  The
> harmony Date.prototype.nanoAge() proposal[1] looks OK, although it's a bit
> underspecified, and it would be pretty easy to implement with our
> soon-to-be-shared TimeStamp/std::time_point.
> 
> Unless anyone has strong feelings for PreciseDate, I think I'd prefer to
> take the path of less resistance with nanoAge().

I don't like nanoAge() because it seems like it would require Date objects to get two different times on creation, which makes both use cases slower. i.e.
currently new Date() does gettimeofday(). Supporting nanoAge() would require it to do gettimeofday() and clock_gettime(MONOTONIC)/mach_absolute_time().

Further, hanging something like nanoAge() off of Date() also doesn't help with the confusion between dates and continuous time. C++11 and Java's JSR-310 both work to separate these concepts and I think Javascript should too.
The need for high-perf has come up recently in the context of HTML gaming.  The simplest API possible seems like it would be sufficient for gaming needs: a single function 'now' that returns a monotonically-increasing double.  There is already a webkit bug/whatwg-mailing proposing to put a 'now' method on 'window.performance' (https://bugs.webkit.org/show_bug.cgi?id=66684 / http://lists.whatwg.org/htdig.cgi/whatwg-whatwg.org/2011-August/032942.html).  I see we already have a 'window.performance' object so adding window.performance.now() seems straightforward.

Are there any objections?  I have no idea how this should interact with the standards process.  The webkit bug mentions discussing this on public-webperf...
Blocks: gecko-games
Here is the draft spec:
  http://dvcs.w3.org/hg/webperf/raw-file/tip/specs/HighResolutionTime/Overview.html
Now would be a good time to raise issues.
(In particular, I think the spec is going to last call phase soon.)
Anyone know where this spec is being discussed?  My issues

 - why return milliseconds?  If we're returning a double we can do seconds, which is the SI unit for time.  I guess millseconds is more "Webby" though.
 - Date.now() not listing unit was a mistake, IMHO.  Whatever the unit, interface should be explicit.
 - readonly attribute vs. function: |var x1 = Performance.nowMs; var x2 = Performance.nowMs; x1 != x2| makes me queasy.  Use a function, if only for consistency with Date.now().

> 4.5 Privacy and Security

There's a fingerprinting issue here, which is that web content can easily guess at the resolution of Performance.now*().  The resolution probably reveals information about the underlying HW that's not exposed through other interfaces.
The spec does not have the problems I talked about in comment 28 so I have no objections to it.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #32)
> Anyone know where this spec is being discussed?  

It's the web perf working group: "Please send comments to public-web-perf@w3.org (archived) with [HighResolutionTime] at the start of the subject line."

> My issues
> 
>  - why return milliseconds?  If we're returning a double we can do seconds,
> which is the SI unit for time.  I guess millseconds is more "Webby" though.

I raised that question by email. The response was that other existing APIs use milliseconds, e.g. setTimeout. I thought that was a reasonable argument.

>  - Date.now() not listing unit was a mistake, IMHO.  Whatever the unit,
> interface should be explicit.
>  - readonly attribute vs. function: |var x1 = Performance.nowMs; var x2 =
> Performance.nowMs; x1 != x2| makes me queasy.  Use a function, if only for
> consistency with Date.now().

Hmm, those seem like taste issues. I think I like the existing proposal better.

> > 4.5 Privacy and Security
> 
> There's a fingerprinting issue here, which is that web content can easily
> guess at the resolution of Performance.now*().  The resolution probably
> reveals information about the underlying HW that's not exposed through other
> interfaces.

I'm not sure what to do about that. The games people said they wanted to get the highest resolution available on the platform--seems like it would be pretty hard to hide the resolution if that requirement is satisfied.
(In reply to David Mandelin from comment #34)
> (In reply to Chris Jones [:cjones] [:warhammer] from comment #32)
> > Anyone know where this spec is being discussed?  
> 
> It's the web perf working group: "Please send comments to
> public-web-perf@w3.org (archived) with [HighResolutionTime] at the start of
> the subject line."
> 
> > My issues
> > 
> >  - why return milliseconds?  If we're returning a double we can do seconds,
> > which is the SI unit for time.  I guess millseconds is more "Webby" though.
> 
> I raised that question by email. The response was that other existing APIs
> use milliseconds, e.g. setTimeout. I thought that was a reasonable argument.
> 

Sure, whatever, webby ;).

> >  - Date.now() not listing unit was a mistake, IMHO.  Whatever the unit,
> > interface should be explicit.
> >  - readonly attribute vs. function: |var x1 = Performance.nowMs; var x2 =
> > Performance.nowMs; x1 != x2| makes me queasy.  Use a function, if only for
> > consistency with Date.now().
> 
> Hmm, those seem like taste issues. I think I like the existing proposal
> better.
> 

Yep.  Doesn't matter too much to me.  There are worse things.

> > > 4.5 Privacy and Security
> > 
> > There's a fingerprinting issue here, which is that web content can easily
> > guess at the resolution of Performance.now*().  The resolution probably
> > reveals information about the underlying HW that's not exposed through other
> > interfaces.
> 
> I'm not sure what to do about that. The games people said they wanted to get
> the highest resolution available on the platform--seems like it would be
> pretty hard to hide the resolution if that requirement is satisfied.

Privileged API is a solution.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #35)
> Privileged API is a solution.

An unattractive solution, especially if it isn't necessary.  I'm not a security expert, but when this issue is raised, the standard answer has been: information that can be revealed through high-resolution timers is usually also available through coarse grain timers by running more iterations.
Currently, Date.now() basically lets web content guess whether timer resolution is 15ms (WinXP) or 1ms (everywhere else).  That information isn't quite the same as the UA; I might run Firefox in a linux VM on a WinXP machine, and content would deduce a 15ms timer resolution, which exposes the fact that it's running in a VM.  (In all probability.)

Higher-resolution timing exposes more information about the underlying HW.  For example, on the computer I'm running this, the minimum measurable resolution is ~100ns.  That's not particularly likely to be the same on the machine you're reading this.

I'm not particularly worried about this, and there are probably other existing ways to measure some factors that correlate with timer resolution.  Running simple arithmetic in a tight loop is probably a pretty reliable way to measure processor speed, e.g.  Carefully written typed array code could probably be used to measure memory bandwidth and cache hierarchies.  (That'd be a fun project, actually.)

Just pointing out that higher-res timing might expose more fingerprinting bits.  Maybe not.
(In reply to Chris Jones [:cjones] [:warhammer] from comment #37)
Yes, that is usual progression of the argument.  http://lists.w3.org/Archives/Public/public-web-perf/2012Mar/0002.html shows they plan to follow-up with (w3c?) security folks; I'm not sure what the resolution was.

To wit, .now was originally a function, somehow became an attribute, and it sounds like will go back to a function \o/
http://lists.w3.org/Archives/Public/public-web-perf/2012Mar/
Draft moved to last call
  http://lists.w3.org/Archives/Public/public-web-perf/2012Mar/0023.html
Deadline for comments is 10 April 2012.
(In reply to Luke Wagner [:luke] from comment #39)
> Draft moved to last call
>   http://lists.w3.org/Archives/Public/public-web-perf/2012Mar/0023.html
> Deadline for comments is 10 April 2012.

Does that mean it's close enough that we can start implementing? And if so, how about creating a new bug for the implementation: this one has a lot of comments already they are almost all API discussion.

(Btw, I saw that they did change it back to a method in the current draft.)
Attached patch Attempt at patch (obsolete) — Splinter Review
So, here's a try for a patch. Some questions:

0) Does this make sense? It's been a long time since I hacked native Fx stuff. Especially the double outparam gave me some headaches. The current approach seems to be fine, though. Related: do we need the typedef or should I just define double and forget about it? (not entirely sure why the existing DOMTimeMilliSec thing exists in the first place)

1) Tests - where do they go and what should they test? Should I just add some mochitests that test the rv monotonically increases and is positive? Would it be possible to 'really' test the monotonicity by actually changing the system clock (probably not...)?

2) AIUI, Timestamp falls back to being ms-resolution on platforms without support for more high-res timers. This breaks with the spec which requires at least 0.1ms resolution. Is my understanding correct, and if so do we care and/or want the 'fallback' to work differently (not provide the API, throw, ...)?
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Attachment #618174 - Flags: review?(bugs)
(In reply to Gijs Kruitbosch from comment #41)
> So, here's a try for a patch. Some questions:

Oh, and 3): I'm just assuming that since the API draft is in Last Call, implementing now is a reasonably safe bet... The period for questions is already over (April 10th), does anyone know if massive revisions are still in the works?
Comment on attachment 618174 [details] [diff] [review]
Attempt at patch

>+++ b/dom/interfaces/base/nsIDOMPerformanceTiming.idl
>@@ -59,8 +59,10 @@ interface nsIDOMPerformanceTiming : nsIS
Update the uuid.

nsIDOMPerformanceTiming should be quickstubbed
(add it to dom_quickstubs.qsconf).
Probably also nsIDOMPerformance.

I haven't followed the spec changes. Could someone else review this?
Perhaps cjones or bz or dmandelin?
Attachment #618174 - Flags: review?(bugs) → review?(jones.chris.g)
Comment on attachment 618174 [details] [diff] [review]
Attempt at patch

I'm not a good person to review this because I'm not familiar with the interface and I haven't been following the spec discussion.  -->bz who probably knows where to kick this (if it's not him).
Attachment #618174 - Flags: review?(jones.chris.g) → review?(bzbarsky)
(In reply to Gijs Kruitbosch from comment #42)
> (In reply to Gijs Kruitbosch from comment #41)
> > So, here's a try for a patch. Some questions:
> 
> Oh, and 3): I'm just assuming that since the API draft is in Last Call,
> implementing now is a reasonably safe bet... The period for questions is
> already over (April 10th), does anyone know if massive revisions are still
> in the works?

I expect it to be pretty stable. I was briefly involved in the discussions about a month ago and nothing major has changed during that time.

Thanks a lot for the patch, by the way. Game devs really want this feature, plus we perf people have long wanted higher resolution timer for general usage.
Comment on attachment 618174 [details] [diff] [review]
Attempt at patch

Agreed with smaug on quickstubbing this.  Probably no point in doing a custom quickstub, but could you file a followup on moving these objects to the new DOM bindings asap?

The type of the property in the spec is DOMHighResTimeStamp, not DOMTimeMilliSecFractional.

The spec says the property is on performance, not on performance.timing.  So it needs to be on nsIDOMPerformance.

Testing this is hard (apart from trivial tests like the prop being on the right object and returning a time).  :(  I have no good ideas there.

I think the fallback behavior is fine.
Attachment #618174 - Flags: review?(bzbarsky) → review-
(In reply to Boris Zbarsky (:bz) from comment #46)
> Comment on attachment 618174 [details] [diff] [review]
> Attempt at patch
> 
> Agreed with smaug on quickstubbing this.  Probably no point in doing a
> custom quickstub, but could you file a followup on moving these objects to
> the new DOM bindings asap?

Filed bug 749101. Might need more detail (should it block bug 580070?), but I'm not really a domain expert. :-(

FWIW, if you can point me to a sample of how this is done (ie other existing stuff that's been moved to the new bindings), I might look at doing it. Seems so far these interfaces aren't particularly complicated so hopefully it won't be rocket science (knock on wood!).

> The type of the property in the spec is DOMHighResTimeStamp, not
> DOMTimeMilliSecFractional.

Ah, so these typedefs are (at least partially) there for spec compliance? I completely missed that point, so I made something up. Sorry! I figured it was just for making it easy to change the type of all the attributes in one go, if the spec ever changed.

Out of interest, (how) does the type defined matter? In my day job these days I'm just a web dev, so I get back a js Number and I stop caring (which is why I hadn't realized this mattered). Is it even possible to check the names of these types? Is it ('just') for non-js consumers?

> The spec says the property is on performance, not on performance.timing.  So
> it needs to be on nsIDOMPerformance.

Um, wow. I can't believe I missed that. I guess I just figured since it uses navigationStart, it would be on the same object.

> Testing this is hard (apart from trivial tests like the prop being on the
> right object and returning a time).  :(  I have no good ideas there.

OK. I guess I'll add the trivial ones in any case, I suppose they might still catch something silly happening when we move to the new DOM bindings...

(will try to get a new patch up tonight (European time))
Attached patch Patch v2 (w/ basic test) (obsolete) — Splinter Review
This should address the issues raised in comment 46 and comment 43.

Please check if/that my quickstubs usage is correct; I read http://blog.mozilla.org/jorendorff/2008/10/09/quick-stubs/ and read/skimmed the comments in the python script mentioned there (if someone cares: the link is dead!). Then I just added what seemed sensible enough.

The resulting build works and passes the new test. I'll submit it to try in a sec; the previous patch was submitted to try and got a green tree (but emails with 'warnings' about jetpack tests? Not entirely clear what that entails).

I'm not really happy with the code duplication between TimeStampToDOMHighRes and TimeStampToDOM, but I wasn't sure if it was desirable to have the latter call the former (and cast the result). If there's a better/prefered way to do this (create a getter for mNavigationStartTimeStamp?), let me know.

The test is pretty silly, but at least it tests something. I considered adding a test about the sub-ms resolution, but didn't as it would break on architectures without high-res timing info. I put it in general since creating a directory for it seemed unnecessary, and I couldn't see a better place (but maybe I missed it?).

Back when I did this more regularly a patch like this needed sr. Google isn't finding me up-to-date docs, does it still?
Attachment #618174 - Attachment is obsolete: true
Attachment #618700 - Flags: review?(bzbarsky)
> FWIW, if you can point me to a sample of how this is done (ie other existing
> stuff that's been moved to the new bindings)

So far, basically just XMLHttpRequest.  The interface part here should be simple; the hard part is that new bindings require the object to be cycle collected, so if these are not already that will have to be fixed.

I wouldn't worry about it short-term; I think we want to make some infrastructure changes to the new bindings first that will make this easier.

> Ah, so these typedefs are (at least partially) there for spec compliance?

Well, more like for making our IDL look like the spec's to reduce confusion.  The name is not exposed anywhere on the web, so doesn't affect compliance per se.
Comment on attachment 618700 [details] [diff] [review]
Patch v2 (w/ basic test)

I would make TimeStampToDOMHighRes inlined.  Also, get rid of the IsNull() check: no one should be passing null here.  TimeStampToDOM just needs that case so it can return 0 if people, say, ask for loadEnd before the load has ended. 

Also, the test seems like it will fail on fast hardware or on systems with a lower timing accuracy...  Probably better to just make both of the '>' checks into '>='.

r=me with that.  Thanks!
Attachment #618700 - Flags: review?(bzbarsky) → review+
Comment on attachment 618700 [details] [diff] [review]
Patch v2 (w/ basic test)


>+NS_IMETHODIMP
>+nsPerformance::Now(DOMHighResTimeStamp* aNow) {
{ should be in the next line


>+<html>
>+<head>
>+  <title>Test for High Resolution Timer</title>
>+  <script type="text/javascript" src="/MochiKit/MochiKit.js"></script>
>+  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
>+  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
>+</head>
>+<body>
>+  <script>
>+    ok(window.performance);
>+    ok(typeof window.performance.now == 'function');
>+    var n = window.performance.now();
>+    ok(n > 0);
>+    ok(window.performance.now() > n);

Would be good to test that the now() will actually increase.
Some simple setTimeout should work there.
Just sent this to try, including all the comment so far. Carrying over review.
Attachment #618813 - Flags: superreview?(bugs)
Attachment #618813 - Flags: review+
Comment on attachment 618813 [details] [diff] [review]
Patch sent to try

>+nsPerformance::Now(DOMHighResTimeStamp* aNow) {
{ should be in the next line
Attachment #618813 - Flags: superreview?(bugs) → superreview+
Try run for 95b7e1726e64 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=95b7e1726e64
Results (out of 83 total builds):
    success: 65
    warnings: 18
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/gijskruitbosch@gmail.com-95b7e1726e64
Comment on attachment 618813 [details] [diff] [review]
Patch sent to try

Pushed to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/21dcf77f7b18
(with nit from comment #53)

Try run: see comment #54
Attachment #618813 - Flags: checkin+
Attachment #618700 - Attachment is obsolete: true
Target Milestone: --- → mozilla15
http://hg.mozilla.org/mozilla-central/rev/21dcf77f7b18
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 749894
You need to log in before you can comment on or make changes to this bug.