Andrea Marchesini [:baku] Assignee
	Description • 8 years ago

      No description provided.

	Andrea Marchesini [:baku] Assignee
	Comment 2 • 8 years ago

I'll add a test in a separate patch.

	Boris Zbarsky [:bzbarsky]
	Comment 3 • 8 years ago

Hmm.  This isn't quite right, I think.

As as simple example, consider the shared worker case.  In the shared worker case, mNowBaseTimeHighRes is set via PR_Now() at worker creation time.  So it's not using the monotonic clock at all.

What we really want here, I think, is a single process-wide mozilla::TimeStamp and the corresponding PR_Now() value.  And then we us that one snapshotted PR_Now() value for all the other TimeOrigin() determinations.  At least that's how the spec defines this.

timeOrigin should probbaly be set [Constant] in the IDL, right?

Once we get this checked in, we probably want a followup to stop using the parent's now base time in child workers, using the creation time instead like the spec says, and people can then use timeOrigin to convert.  Separate bug on that.

	Andrea Marchesini [:baku] Assignee
	Comment 6 • 8 years ago

This approach is based on mozilla::TimeStamp. There is 1 unique startupTimeStamp, shared with the child processes (so that the start time is in sync). Then I use that TimeStamp for calculating timeOrigin (part 2).
Maybe, 'startup' is the wrong word.

	Boris Zbarsky [:bzbarsky]
	Comment 8 • 8 years ago

> There is 1 unique startupTimeStamp, shared with the child processes (so that the start time is in sync).

Does sending a TimeStamp to a different process even make sense?  What ensures they're using the same monotonic clock?  Does all the static state set up on windows end up looking the same across processes?

In any case, this is still wrong, because you want to add the unix time of the start timestamp to the whole thing.  Otherwise you're leaking the browser startup time to the web.

	Boris Zbarsky [:bzbarsky]
	Comment 11 • 8 years ago

The part 1 diff is missing the new files....

	Boris Zbarsky [:bzbarsky]
	Comment 14 • 8 years ago

Comment on attachment 8811166 [details] [diff] [review]
part 1 - timeOrigin

> +static PerformanceService* gPerformanceService = nullptr;

Doesn't this mean that we'll keep destroying and recreating it, in general?  That doesn't look right.

>+PerformanceService::TimeOrigin(const TimeStamp& aCreationTimeStamp) const
>+  MOZ_ASSERT(aCreationTimeStamp > mCreationTimeStamp);

What guarantees this?  I don't think anything actually does.  We can have things getting created before they create their performance objects...

>+  PRTime mCreationEpocTime;

"Epoch", not "Epoc".

	Andrea Marchesini [:baku] Assignee
	Comment 15 • 8 years ago

> Doesn't this mean that we'll keep destroying and recreating it, in general? 

No. It means that the object is destroyed when there are no Performance objects alive. Performance objects keep this PerformanceService alive. This seems correct to me.

	Andrea Marchesini [:baku] Assignee
	Comment 16 • 8 years ago

All the other comments are applied.

	Boris Zbarsky [:bzbarsky]
	Comment 17 • 8 years ago

> This seems correct to me.

It's not, because it means things will keep resynchronizing to a possibly-changing PR_Now value.  That's explicitly _not_ supposed to happen.

	Andrea Marchesini [:baku] Assignee
	Comment 18 • 8 years ago

Ok, I see what you mean. Here a new version.

	Boris Zbarsky [:bzbarsky]
	Comment 19 • 8 years ago

Comment on attachment 8811480 [details] [diff] [review]
part 1 - timeOrigin

This works, but now we have to lock on every timeOrigin get, which could become somewhat perf-sensitive... :(

	Andrea Marchesini [:baku] Assignee
	Comment 20 • 8 years ago

A bit better.

	Boris Zbarsky [:bzbarsky]
	Comment 21 • 8 years ago

Comment on attachment 8811511 [details] [diff] [review]
part 1 - timeOrigin

r=me

	Pulsebot
	Comment 22 • 8 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0f72f0b0be
Implement Performance.timeOrigin - part 1, r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/c22f17e0db9d
Implement Performance.timeOrigin - part 2 - tests, r=bz

	Pulsebot
	Comment 23 • 8 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0f869d4c93ef
Implement Performance.timeOrigin - part 3 - tests, r=me

	Pulsebot
	Comment 24 • 8 years ago

Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49305a317143
Implement Performance.timeOrigin - part 4 - tests, r=me

	Phil Ringnalda (:philor)
	Comment 25 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/cf0f72f0b0be
https://hg.mozilla.org/mozilla-central/rev/c22f17e0db9d
https://hg.mozilla.org/mozilla-central/rev/0f869d4c93ef
https://hg.mozilla.org/mozilla-central/rev/49305a317143

	areya
	Comment 26 • 8 years ago

https://hg.mozilla.org/mozilla-central/rev/cf0f72f0b0be
https://hg.mozilla.org/mozilla-central/rev/c22f17e0db9d
https://hg.mozilla.org/mozilla-central/rev/0f869d4c93ef
https://hg.mozilla.org/mozilla-central/rev/49305a317143

	Chris Mills [:cmills]
	Comment 27 • 6 years ago

I've documented this by updating the support info on the ref page (it was already documented), and adding a note to the Fx59 rel notes:

https://developer.mozilla.org/en-US/docs/Web/API/Performance/timeOrigin#Browser_Compatibility
https://developer.mozilla.org/en-US/Firefox/Releases/59#DOM

Let me know if you think this looks OK. Thanks!

	Chris Mills [:cmills]
	Comment 28 • 6 years ago

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #27)
> I've documented this by updating the support info on the ref page (it was
> already documented), and adding a note to the Fx59 rel notes:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/Performance/
> timeOrigin#Browser_Compatibility
> https://developer.mozilla.org/en-US/Firefox/Releases/59#DOM
> 
> Let me know if you think this looks OK. Thanks!

Ah dammnit, this is a 53 addition. I've moved the note to here:

https://developer.mozilla.org/en-US/Firefox/Releases/53#DOM