Closed Bug 1256562 Opened 8 years ago Closed 7 years ago

Convert native event times to TimeStamps for Mac

Categories

(Core :: Widget: Cocoa, defect, P3)

Product:
Core
Component:
Widget: Cocoa
Platform:
All
macOS
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox48 --- wontfix
firefox-esr45 --- wontfix
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: birtles, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Whiteboard: tpi:+)

Attachments

(3 files)

Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp
58 bytes, text/x-review-board-request
birtles
: review+
mstange
: review+
Details
Bug 1256562 part.2 All event dispatchers under widget/cocoa should initialize WidgetEventTime::mTimeStamp with nsCocoaUtils::GetEventTimeStamp()
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1256562 part.3 Enable high resolution timestamp on macOS
58 bytes, text/x-review-board-request
smaug
: review+
Details 
As with bug 1026803 and bug 77992, we need to convert events times provided by the OS to corresponding TimeStamp values.
Flags: needinfo?(ajones)
I know bug 1026803 was in DOM: Events but it seems like it should be in Widget: Cocoa. Feel free to disagree, of course :)
Component: DOM: Events → Widget: Cocoa
Whiteboard: tpi:?
Whiteboard: tpi:? → tpi:-
Priority: -- → P1
Whiteboard: tpi:- → tpi:+
Priority: P1 → P3
Removing ni? because I'm no longer involved in this bug in any way.
Flags: needinfo?(ajones)
Assignee: nobody → masayuki
Are there good testcases for Event.timeStamp? I briefly tested for keydown, keypress, keyup, mousedown, mouseup, click, dblclick, compositionstart, compositionupdate and compositionend.
Status: NEW → ASSIGNED
Hardware: Unspecified → All
Brian, could you check if I implement CurrentCocoaTimeGetter as you expected first? After your r+, I'll request to review to mstange and smaug.
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review100444

I've only done a very rough review. The general flow seems correct although I'm not sure the check for inflight requests is correct but perhaps I'm missing something. I think karlt should check this though.

::: widget/cocoa/nsCocoaUtils.mm:51
(Diff revision 1)
>  using std::ceil;
>  
> +// The type of [NSEvent timestamp] is NSTimeInterval which is an alias of
> +// double.  However, SystemTimeConverter allows only unsigned type and
> +// we don't need such big range (NSTimeInterval supports over a range of
> +// 10,000 years!).  So, let's use uint32_t here (Note that SystemTimeConver

SystemTimeConverter

::: widget/cocoa/nsCocoaUtils.mm:92
(Diff revision 1)
> +    NSTimeInterval currentTime = GetCurrentTimeNative();
> +    if (mBackwardsSkewStamp && currentTime == mLastPostTime) {

Does this ever evaluate true? Doesn't the value of GetCurrentTimeNative() always return a different value?

(Also, with CurrentX11TimeGetter we avoid calling get synchronous time getter in this method because it's expensive but is that not the case for OSX?)

::: widget/cocoa/nsCocoaUtils.mm:130
(Diff revision 1)
> +  }
> +
> +private:
> +  static CurrentCocoaTimeGetter* sInstance;
> +
> +  Maybe<TimeStamp> mBackwardsSkewStamp;

TimeStamp has a null value so I don't think we need Maybe?

::: widget/cocoa/nsCocoaUtils.mm:1141
(Diff revision 1)
> +
> +TimeStamp
> +nsCocoaUtils::GetEventTimeStamp(NSTimeInterval aEventTime)
> +{
> +  if (!aEventTime) {
> +    // If the event is generaged by 3rd party's apps or something, its timestamp

generated
Hmm, looks like karlt is away until Dec 29.
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review100448

I'm inclined to give say r=me but I think I'd like to know first:
* Is the check for currentTime == mLastPostTime in GetTimeAsyncForPossibleBackwardsSkew needed? If not, can we drop it?
* Is GetCurrentTimeNative() expensive? If so, can we avoid it? For that matter, why is the approach used by CurrentX11TimeGetter not suitable here?
* Is it possible to use more similar names to CurrentX11TimeGetter? e.g. s/mBackwardsSkewStamp/mAsyncUpdateStart/ ?

::: widget/cocoa/nsCocoaUtils.mm:1141
(Diff revision 1)
> +
> +TimeStamp
> +nsCocoaUtils::GetEventTimeStamp(NSTimeInterval aEventTime)
> +{
> +  if (!aEventTime) {
> +    // If the event is generaged by 3rd party's apps or something, its timestamp

("or something" seems a bit vague. Perhaps, "If the event is generated by a 3rd party application, its timestamp...")
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review100546

As discussed in person, this looks good but:

* We should drop the check for currentTime == mLastPostTime in GetTimeAsyncForPossibleBackwardsSkew and file a bug for doing the same thing for the Windows version
* We *don't* need to use more similar names to CurrentX11TimeGetter since the names here match the Windows version
* We can drop the use of Maybe<> since TimeStamp has a null value
* We should make the comment tweaks in comment 10 and comment 12

Other than that r=me

I'd like to get karlt to review but since he's away I think this is fine. If he finds issues once he returns we can problem uplift the necessary fixes.
Attachment #8820295 - Flags: review?(bbirtles) → review+
Oh, I misunderstood about the limitation of SystemTimeConverter. It uses int64_t in it, but it's used for storing the difference between 2 TimeStamp instances difference. So, my patch should be able to use uint64_t.
https://jsfiddle.net/d_toybox/kp2beobg/3/
Hmm, at dispatching eKeyPress event, TextInputHandler uses NSKeyDown event at initializing the mTimeStamp of eKeyPress event. However, the Event.timeStamp values of "keydown" and "keypress" are different and it sometimes causes setting older timestamp to keypress event. I've not understood about Event.TimeStamp yet...

Anyway, we should fix it later. There are other issues of timeStamp of keyboard/composition event vs. input event. So, might be caused by same issue.
Attachment #8820296 - Flags: review?(mstange)
Comment on attachment 8820296 [details]
Bug 1256562 part.2 All event dispatchers under widget/cocoa should initialize WidgetEventTime::mTimeStamp with nsCocoaUtils::GetEventTimeStamp()

https://reviewboard.mozilla.org/r/99826/#review101044

::: widget/cocoa/SwipeTracker.mm:204
(Diff revision 2)
>  /* static */ WidgetSimpleGestureEvent
>  SwipeTracker::CreateSwipeGestureEvent(EventMessage aMsg, nsIWidget* aWidget,
> -                                      const LayoutDeviceIntPoint& aPosition)
> +                                      const LayoutDeviceIntPoint& aPosition,
> +                                      const TimeStamp& aTimeStamp)
>  {
> +  // XXX Why isn't this initialized with nsCocoaUtils::InitInputEvent()?

Not sure, probably because we don't need modifiers information on the event. And now with this patch, you'd have to overwrite the mTimeStamp field afterwards anyway because we don't have an NSEvent to pull it from.

::: widget/cocoa/nsChildView.mm:3546
(Diff revision 2)
>  - (BOOL)isOpaque
>  {
>    return [[self window] isOpaque];
>  }
>  
> +// XXX Is this really used?

Looks like it's unused.
Attachment #8820296 - Flags: review?(mstange) → review+
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review101048

::: widget/cocoa/nsCocoaUtils.mm:80
(Diff revision 2)
> +    static_assert(sizeof(unsigned long) == sizeof(uint64_t),
> +                  "Assuming that unsignedLongValue returns uint64_t value");
> +    return [[NSNumber numberWithDouble:aTime * 1000] unsignedLongValue];

Why did you choose this way of converting the value, instead of just `return uint64_t(aTime * 1000.0);`?

Also, using 1000 only gives you millisecond resolution. Why not go higher? We might as well use 1000000 and go for microsecond resolution (and rename this function to ToMicroseconds).

::: widget/cocoa/nsCocoaUtils.mm:101
(Diff revision 2)
> +    mBackwardsSkewStamp = aNow;
> +    NSEvent* event =
> +      [NSEvent otherEventWithType:NSApplicationDefined
> +                         location:NSMakePoint(0, 0)
> +                    modifierFlags:0
> +                        timestamp:GetCurrentTimeNative()

So here you're setting the timestamp that will be on the event. This makes me think that it doesn't make sense to go through the event loop for this because you have all the information you need to correct time skew already.
I can see how it would be useful to post an event on platforms where events are the only way to receive a new timestamp in "native event time". But that's not the case here on Mac.

We can correct for time skew synchronously on Mac by just querying the new time. I think the ideal implementation of GetTimeAsyncForPossibleBackwardsSkew would be:

```
void GetTimeAsyncForPossibleBackwardsSkew(const TimeStamp& aNow)
{
  TimeConverter().CompensateForBackwardsSkew(GetCurrentTime(), aNow);
}
```

But the caller of GetTimeAsyncForPossibleBackwardsSkew would probably need to be adjusted to be able to deal with synchronous responses.

::: widget/cocoa/nsCocoaUtils.mm:130
(Diff revision 2)
> +  {
> +  }
> +
> +  NSTimeInterval GetCurrentTimeNative() const
> +  {
> +    return [[NSProcessInfo processInfo] systemUptime];

Please add a comment that says that, since we care about NSEvent timestamps here, we need a reference timestamp that is in the same "space" as NSEvent timestamps. And `[[NSProcessInfo processInfo] systemUptime]` is how you get the current time in the same space as `-[NSEvent timestamp]`.
You could also mention that CurrentCocoaTimeGetter does not let us express timestamps before the system startup, and that that's ok because we only care about event timestamps.
Attachment #8820295 - Flags: review?(mstange)
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review101058

::: widget/cocoa/nsCocoaUtils.mm:1128
(Diff revision 2)
> +  uint32_t milliseconds = CurrentCocoaTimeGetter::ToMilliseconds(aPostedTime);
> +  TimeConverter().CompensateForBackwardsSkew(milliseconds, skewStamp);
> +}
> +
> +TimeStamp
> +nsCocoaUtils::GetEventTimeStamp(NSTimeInterval aEventTime)

Actually, I think we don't need any of the SystemTimeConverter mechanisms on Mac. TimeStamp::mValue is in nanoseconds since boot, and `mach_absolute_time() == [[NSProcessInfo processInfo] systemUptime] * 1000000000.0`, so I think all you need is:

```
nsCocoaUtils::GetEventTimeStamp(NSTimeInterval aEventTime)
{
  return TimeStamp::FromSystemTime(uint64_t(aEventTime * 1000000000.0));
}
```
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review101058

> Actually, I think we don't need any of the SystemTimeConverter mechanisms on Mac. TimeStamp::mValue is in nanoseconds since boot, and `mach_absolute_time() == [[NSProcessInfo processInfo] systemUptime] * 1000000000.0`, so I think all you need is:
> 
> ```
> nsCocoaUtils::GetEventTimeStamp(NSTimeInterval aEventTime)
> {
>   return TimeStamp::FromSystemTime(uint64_t(aEventTime * 1000000000.0));
> }
> ```

Oh, let me correct that:

```
  return TimeStamp::FromSystemTime(BaseTimeDurationPlatformUtils::TicksFromMilliseconds(aEventTime * 1000.0));
```

mach_absolute_time is not necessarily in milliseconds (though on my system it is, because timebase.numer and timebase.denom are both 1).
Comment on attachment 8820297 [details]
Bug 1256562 part.3 Enable high resolution timestamp on macOS

https://reviewboard.mozilla.org/r/99828/#review101094

::: modules/libpref/init/all.js:1225
(Diff revision 2)
>  // Annotate channels based on the tracking protection list in all modes
>  pref("privacy.trackingprotection.annotate_channels",  false);
>  
>  pref("dom.event.contextmenu.enabled",       true);
>  pref("dom.event.clipboardevents.enabled",   true);
> -#if defined(XP_WIN) && !defined(RELEASE_OR_BETA) || defined(MOZ_WIDGET_GTK) && !defined(RELEASE_OR_BETA)
> +#if defined(XP_WIN) && !defined(RELEASE_OR_BETA) || defined(MOZ_WIDGET_GTK) && !defined(RELEASE_OR_BETA) || defined(XP_MACOSX) && !defined(RELEASE_OR_BETA)

Ok, so we need still support for Android.
Looks like that is bug 1256565.
Attachment #8820297 - Flags: review?(bugs) → review+
Comment on attachment 8820296 [details]
Bug 1256562 part.2 All event dispatchers under widget/cocoa should initialize WidgetEventTime::mTimeStamp with nsCocoaUtils::GetEventTimeStamp()

https://reviewboard.mozilla.org/r/99826/#review101104

::: widget/cocoa/nsCocoaUtils.mm:707
(Diff revision 2)
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>  
>    aInputEvent.mModifiers = ModifiersForEvent(aNativeEvent);
>    aInputEvent.mTime = PR_IntervalNow();
> +  aInputEvent.mTimeStamp = GetEventTimeStamp([aNativeEvent timestamp]);

Ok, I trust that GetEventTimeStamp does the right thing.
Attachment #8820296 - Flags: review?(bugs) → review+
Comment on attachment 8820296 [details]
Bug 1256562 part.2 All event dispatchers under widget/cocoa should initialize WidgetEventTime::mTimeStamp with nsCocoaUtils::GetEventTimeStamp()

https://reviewboard.mozilla.org/r/99826/#review101152

::: widget/cocoa/SwipeTracker.mm:204
(Diff revision 2)
>  /* static */ WidgetSimpleGestureEvent
>  SwipeTracker::CreateSwipeGestureEvent(EventMessage aMsg, nsIWidget* aWidget,
> -                                      const LayoutDeviceIntPoint& aPosition)
> +                                      const LayoutDeviceIntPoint& aPosition,
> +                                      const TimeStamp& aTimeStamp)
>  {
> +  // XXX Why isn't this initialized with nsCocoaUtils::InitInputEvent()?

Hmm, even if current event listeners don't need the modifiers, it may be referred by others. Perhaps, we need to improve here in another bug.

::: widget/cocoa/nsChildView.mm:3546
(Diff revision 2)
>  - (BOOL)isOpaque
>  {
>    return [[self window] isOpaque];
>  }
>  
> +// XXX Is this really used?

Okay, I'll file a bug to remove this.

::: widget/cocoa/nsCocoaUtils.mm:707
(Diff revision 2)
>  {
>    NS_OBJC_BEGIN_TRY_ABORT_BLOCK;
>  
>    aInputEvent.mModifiers = ModifiersForEvent(aNativeEvent);
>    aInputEvent.mTime = PR_IntervalNow();
> +  aInputEvent.mTimeStamp = GetEventTimeStamp([aNativeEvent timestamp]);

Me too :-)

We should wrap the whole things to compute mTimeStamp with a utility method.
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review101058

> Oh, let me correct that:
> 
> ```
>   return TimeStamp::FromSystemTime(BaseTimeDurationPlatformUtils::TicksFromMilliseconds(aEventTime * 1000.0));
> ```
> 
> mach_absolute_time is not necessarily in milliseconds (though on my system it is, because timebase.numer and timebase.denom are both 1).

Oh, really? If we don't need SystemTimeConverter nor CurrentCocoaTimeGetter, we do this much simpler!
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review101048

> Why did you choose this way of converting the value, instead of just `return uint64_t(aTime * 1000.0);`?
> 
> Also, using 1000 only gives you millisecond resolution. Why not go higher? We might as well use 1000000 and go for microsecond resolution (and rename this function to ToMicroseconds).

> Why did you choose this way of converting the value, instead of just return uint64_t(aTime * 1000.0);?

I'm not familiar with cast with too big double value. So, I was thinking that using such API is safer.

> Also, using 1000 only gives you millisecond resolution. Why not go higher? We might as well
> use 1000000 and go for microsecond resolution (and rename this function to ToMicroseconds).

Because of SystemTimeConverter's limitation. It doesn't allow to use |double| and if we use microsecond with unsigned integer type, the range becomes narrower.

> So here you're setting the timestamp that will be on the event. This makes me think that it doesn't make sense to go through the event loop for this because you have all the information you need to correct time skew already.
> I can see how it would be useful to post an event on platforms where events are the only way to receive a new timestamp in "native event time". But that's not the case here on Mac.
> 
> We can correct for time skew synchronously on Mac by just querying the new time. I think the ideal implementation of GetTimeAsyncForPossibleBackwardsSkew would be:
> 
> ```
> void GetTimeAsyncForPossibleBackwardsSkew(const TimeStamp& aNow)
> {
>   TimeConverter().CompensateForBackwardsSkew(GetCurrentTime(), aNow);
> }
> ```
> 
> But the caller of GetTimeAsyncForPossibleBackwardsSkew would probably need to be adjusted to be able to deal with synchronous responses.

Okay, I'll retry to understand deeper what we should do.
Yeah I changed my mind a bit during the review. Comment 24 has the code I think we should use.
Comment on attachment 8820295 [details]
Bug 1256562 part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp

https://reviewboard.mozilla.org/r/99824/#review101414

::: widget/cocoa/nsCocoaUtils.mm:1033
(Diff revision 3)
> +    // If the event is generated by a 3rd party application, its timestamp
> +    // may be 0.  In this case, just return current timestamp.
> +    // XXX Should we cache last event time?
> +    return TimeStamp::Now();
> +  }
> +  int64_t tick =

Let's add a comment:

  // The internal value of the macOS implementation of TimeStamp is based on
  // mach_absolute_time(), which measures "ticks" since boot.
  // Event timestamps are NSTimeIntervals (seconds) since boot. So the two time
  // representations already have the same base; we only need to convert
  // seconds into ticks.

And you can use the same explanation in the commit message.
Attachment #8820295 - Flags: review?(mstange) → review+
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/5e87a3190b95
part.1 Implement nsCocoaUtils::GetEventTimeStamp() to convert from timeStamp of NSEvent to TimeStamp r=birtles,mstange
https://hg.mozilla.org/integration/autoland/rev/725f7a643301
part.2 All event dispatchers under widget/cocoa should initialize WidgetEventTime::mTimeStamp with nsCocoaUtils::GetEventTimeStamp() r=mstange,smaug
https://hg.mozilla.org/integration/autoland/rev/d3343e6ab57e
part.3 Enable high resolution timestamp on macOS r=smaug
See Also: → 1338396
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: