[webkit-reviews] review granted: [Bug 117179] Use the PlatformEvent timestamp when creating a DOM Event : [Attachment 203971] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Benjamin Poulain
<benjamin at webkit.org>'s request for review:
Bug 117179: Use the PlatformEvent timestamp when creating a DOM Event
https://bugs.webkit.org/show_bug.cgi?id=117179

Attachment 203971: Patch
https://bugs.webkit.org/attachment.cgi?id=203971&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203971&action=review


> Source/WebCore/platform/mac/PlatformEventFactoryMac.h:56
> +double getEventTimeStampSince1970(NSEvent*);

WebKit coding style says not to use the word “get” in a function name like this
one.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:350
> +static CFAbsoluteTime systemStartupTime;

Annoying to have this here since only two functions are allowed to use this.
Using it instead of getCachedStartupTimeIntervalSince1970 would be a bug.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:358
> +

Strange use of a blank line here. I say delete it or add more blank lines.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:360
> +    systemStartupTime =  kCFAbsoluteTimeIntervalSince1970 +
CFAbsoluteTimeGetCurrent() - elapsedTimeSinceStartup;

Extra space here.

> Source/WebCore/platform/mac/PlatformEventFactoryMac.mm:363
> +static CFTimeInterval getCachedStartupTimeIntervalSince1970()

WebKit coding style does not use the word “get” in the name of a function like
this one.

> Tools/WebKitTestRunner/mac/EventSenderProxy.mm:148
> -					  timestamp:currentEventTime()
> +					  timestamp:GetCurrentEventTime() +
currentEventTime()

I don’t understand the changes of this form; why would we add current event
time to current event time? Seems like that would double it. If the code is
this hard to read it needs a comment or named function or something.