Bug 94987 – Add systemTime to DOM events

• Component: DOM
• Status: RESOLVED DUPLICATE

[Robert Flack]

Pablo Garaizar Sagarminaga recently proposed extending HTML Events with a DOMHighResTimeStamp to the www-dom mailing list. This timestamp would not be subject to system clock skew and could report the raw time at which events arrived to the system:

http://lists.w3.org/Archives/Public/www-dom/2012AprJun/0069.html

With the addition of the Performance.now() function we can use these high res timestamps to get the precise length of time which has passed before handling an event or since since a discrete point in time. This bug tracks implementing an initial version of this feature.

[Robert Flack]

Created attachment 160531 [details]
Patch

[WebKit Review Bot]

Attachment 160531 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/MouseEvent.h:95:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13596312

[Gyuyoung Kim]

Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13599312

[Early Warning System Bot]

Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13599315

[WebKit Review Bot]

Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13601287

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
http/tests/workers/worker-importScriptsOnError.html
plugins/clicking-missing-plugin-fires-delegate.html

[WebKit Review Bot]

Created attachment 160541 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13595383

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
http/tests/workers/worker-importScriptsOnError.html
plugins/clicking-missing-plugin-fires-delegate.html

[WebKit Review Bot]

Created attachment 160547 [details]
Archive of layout-test-results from gce-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Build Bot]

Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13597398

[Alexey Proskuryakov]

Has this been discussed on webkit-dev already? Could you post a link?

[Robert Flack]

No, this has only been discussed on the www-dom w3c list as far as I know. Should I start a discussion on webkit-dev as well?

[Ryosuke Niwa]

Comment on attachment 160531 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Adds a webkitHRTimeStamp property to DOM events which exposes the platform event timestamp consistent with Performance.now() timestamps.

Please add a reference to relevant specifications or standards discussions.
Also, this line is way too long. Please line break appropriately.

[Robert Flack]

Created attachment 168415 [details]
Patch

[Adam Barth]

Comment on attachment 168415 [details]
Patch

We'll probably want an ENABLE and a runtime flag for this feature before landing it.

[Adam Barth]

This feature seems minor enough that we'd probably want to avoid shipping it to the stable channel in Chrome until we could do so unprefixed.

[Adam Barth]

Perhaps that means we should implement it without a prefix and just the ENABLE and runtime flags to avoid shipping it to stable until we think it is ready.

[Ryosuke Niwa]

This feature should be announced on webkit-dev.

[Robert Flack]

Created attachment 170536 [details]
Patch

[WebKit Review Bot]

Attachment 170536 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3819:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3820:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Flack]

(In reply to comment #17)
> Perhaps that means we should implement it without a prefix and just the ENABLE and runtime flags to avoid shipping it to stable until we think it is ready.

I've removed the prefix and hidden the accessor behind ENABLE_EVENT_SYSTEM_TIME. I'm not sure I understand how we would enable this with a runtime flag, we can't change the exposed properties on DOM events (Event.idl) from runtime preferences can we? I'll try to find some other examples but any advice is appreciated, thanks!

Also, I haven't actually removed the property from event since it's passed through in so many places. Should I if ENABLE(...) all of these places too?

[Early Warning System Bot]

Comment on attachment 170536 [details]
Patch

Attachment 170536 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14566426

[Adam Barth]

> we can't change the exposed properties on DOM events (Event.idl) from runtime preferences can we?

Yes we can.  grep for runtimeenabled

[Adam Barth]

Comment on attachment 170536 [details]
Patch

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

Also you don't appear to have written any tests.  :(

> Source/WebKit/chromium/ChangeLog:8
> +        Adds a webkitSystemTime property to DOM events which exposes the platform event

webkitSystemTime <--- does this need to be updated?

> Source/WebCore/dom/Event.idl:65
> +#if defined(ENABLE_EVENT_SYSTEM_TIME)
> +    readonly attribute double           systemTime;
> +#endif

Should this be InitializedByEventConstructor ?

Please make this runtime enabled.

You don't need to use #if.  You can use the Conditional attribute.

[Robert Flack]

Created attachment 170648 [details]
Patch

[Robert Flack]

(In reply to comment #24)
> (From update of attachment 170536 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170536&action=review
> 
> Also you don't appear to have written any tests.  :(

I'll write some tests in the next patch, just updating the rest of it.

> 
> > Source/WebKit/chromium/ChangeLog:8
> > +        Adds a webkitSystemTime property to DOM events which exposes the platform event
> 
> webkitSystemTime <--- does this need to be updated?

Done.

> 
> > Source/WebCore/dom/Event.idl:65
> > +#if defined(ENABLE_EVENT_SYSTEM_TIME)
> > +    readonly attribute double           systemTime;
> > +#endif
> 
> Should this be InitializedByEventConstructor ?

I'm not sure I fully understand what this is for. timeStamp, type, etc are not initializedByEventConstructor. Should this be?

> 
> Please make this runtime enabled.

Done.

> 
> You don't need to use #if.  You can use the Conditional attribute.

Done.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[WebKit Review Bot]

Attachment 170648 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3819:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3820:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 170648 [details]
Patch

Attachment 170648 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14554664

[Build Bot]

Comment on attachment 170648 [details]
Patch

Attachment 170648 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14554670

[Robert Flack]

Created attachment 170658 [details]
Patch

[WebKit Review Bot]

Attachment 170658 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3819:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3820:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Flack]

Created attachment 170659 [details]
Patch

[WebKit Review Bot]

Attachment 170659 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3819:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3820:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 170659 [details]
Patch

Attachment 170659 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14552649

New failing tests:
plugins/clicking-missing-plugin-fires-delegate.html

[WebKit Review Bot]

Comment on attachment 170659 [details]
Patch

Attachment 170659 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14552672

New failing tests:
plugins/clicking-missing-plugin-fires-delegate.html

[Ojan Vafai]

Comment on attachment 170659 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        No new tests as this just exposes a new property on events.

What's the reason for putting off adding tests? Every webkit patch needs tests unless there's a really good reason it can't be done. In this case, it looks like you have a full implementation, so why not include the tests in this patch?

[Robert Flack]

(In reply to comment #37)
> (From update of attachment 170659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=170659&action=review
> 
> > Source/WebCore/ChangeLog:13
> > +        No new tests as this just exposes a new property on events.
> 
> What's the reason for putting off adding tests? Every webkit patch needs tests unless there's a really good reason it can't be done. In this case, it looks like you have a full implementation, so why not include the tests in this patch?

Sorry, hadn't updated the Changelog yet. I'm in the process of adding tests to the patch.

[Ojan Vafai]

Comment on attachment 170659 [details]
Patch

Removing from the review queue since the patch is being updated.

[Build Bot]

Comment on attachment 170659 [details]
Patch

Attachment 170659 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14563718

[Adam Barth]

Comment on attachment 170659 [details]
Patch

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

> Source/WebCore/page/Settings.h:652
> +#if ENABLE(EVENT_SYSTEM_TIME)
> +        void setEventSystemTimeEnabled(bool enabled) {m_eventSystemTimeEnabled = enabled; }
> +        bool eventSystemTimeEnabled() const { return m_eventSystemTimeEnabled; }
> +#endif

These changes don't seem to be needed.

[Robert Flack]

Created attachment 170750 [details]
Patch

[Robert Flack]

Comment on attachment 170659 [details]
Patch

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

I'm having trouble with my tests, enabling the runtime feature from the test doesn't immediately affect newly created events and event.systemTime is undefined when queried later. Is there a way to refresh the Event object after changing runtime features?

>> Source/WebCore/page/Settings.h:652
>> +#endif
> 
> These changes don't seem to be needed.

This is the entry point with which we usually enable runtime features from chromium isn't it? I guess just the set method would be required for that.

[Adam Barth]

> I'm having trouble with my tests, enabling the runtime feature from the test doesn't immediately affect newly created events and event.systemTime is undefined when queried later. Is there a way to refresh the Event object after changing runtime features?

Nope.  You should just turn it on all the time during testing.

> >> Source/WebCore/page/Settings.h:652
> >> +#endif
> > 
> > These changes don't seem to be needed.
> 
> This is the entry point with which we usually enable runtime features from chromium isn't it? I guess just the set method would be required for that.

Nope.  The runtime enabled features are different from WebCore::Settings.  The former is static state, whereas the latter is per-page state.

[Robert Flack]

Created attachment 170977 [details]
Patch

[WebKit Review Bot]

Attachment 170977 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3819:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3820:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Flack]

Created attachment 170978 [details]
Patch

[WebKit Review Bot]

Attachment 170978 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3819:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3820:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  The parameter name "clipboard" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 14 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14612117

[Build Bot]

Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14606197

[Peter Beverloo (cr-android ews)]

Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14611155

[EFL EWS Bot]

Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14613187

[WebKit Review Bot]

Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14603485

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
http/tests/workers/worker-importScriptsOnError.html
plugins/clicking-missing-plugin-fires-delegate.html

[WebKit Review Bot]

Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14608257

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
http/tests/workers/worker-importScriptsOnError.html
plugins/clicking-missing-plugin-fires-delegate.html

[Rafael Brandao]

Comment on attachment 170978 [details]
Patch

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

Informal review, just a few comments.

> Source/WebCore/dom/Event.h:108
> +    double systemTime() const { return m_timeStamp; }

This is a bit confusing... so m_timeStamp is systemTime and m_createTime is timeStamp()? Could we use m_systemTime instead?

> Source/WebCore/dom/Node.cpp:2661
> +    return EventDispatcher::dispatchEvent(this, WheelEventDispatchMediator::create(event, document()->defaultView(), 1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(event.timestamp())));

Perhaps you could make an auxiliary function to deal with this timestamp transformation? It's repeated three times already in the same file.

> Source/WebCore/page/EventHandler.cpp:3065
> +    RefPtr<KeyboardEvent> keypress = KeyboardEvent::create(keyPressEvent, m_frame->document()->defaultView(), 1000.0 * m_frame->document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(initialKeyEvent.timestamp()));

I still don't like much "m_frame->document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime". Are we sure that no one in the middle can be null pointer? Perhaps you should add asserts on your auxiliary function (if you plan to do it).

[Robert Flack]

Created attachment 173044 [details]
Patch

[WebKit Review Bot]

Attachment 173044 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3858:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3859:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 13 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Flack]

Comment on attachment 170978 [details]
Patch

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

>> Source/WebCore/dom/Event.h:108
>> +    double systemTime() const { return m_timeStamp; }
> 
> This is a bit confusing... so m_timeStamp is systemTime and m_createTime is timeStamp()? Could we use m_systemTime instead?

Done.

>> Source/WebCore/dom/Node.cpp:2661
>> +    return EventDispatcher::dispatchEvent(this, WheelEventDispatchMediator::create(event, document()->defaultView(), 1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(event.timestamp())));
> 
> Perhaps you could make an auxiliary function to deal with this timestamp transformation? It's repeated three times already in the same file.

I'm a little unsure about this, it would just be wrapping DocumentLoadTiming::monotonicTimeToZeroBasedDocumentTime. I could create a function higher up the chain (Document?) to do this but then it could be confusing why we have two functions in different places that do the same thing. What do you think is best?

>> Source/WebCore/page/EventHandler.cpp:3065
>> +    RefPtr<KeyboardEvent> keypress = KeyboardEvent::create(keyPressEvent, m_frame->document()->defaultView(), 1000.0 * m_frame->document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(initialKeyEvent.timestamp()));
> 
> I still don't like much "m_frame->document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime". Are we sure that no one in the middle can be null pointer? Perhaps you should add asserts on your auxiliary function (if you plan to do it).

We use this in Performance::now(), it should always exist but if this gets refactored into a common function I can add asserts.

[Build Bot]

Comment on attachment 173044 [details]
Patch

Attachment 173044 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14755950

[Build Bot]

Comment on attachment 173044 [details]
Patch

Attachment 173044 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14760863

[WebKit Review Bot]

Comment on attachment 173044 [details]
Patch

Attachment 173044 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14778272

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
http/tests/workers/worker-importScriptsOnError.html
plugins/clicking-missing-plugin-fires-delegate.html

[Robert Flack]

Created attachment 173346 [details]
Patch

[WebKit Review Bot]

Attachment 173346 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3858:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3859:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 13 in 42 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 173346 [details]
Patch

Attachment 173346 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14796069

[Build Bot]

Comment on attachment 173346 [details]
Patch

Attachment 173346 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14779536

[Robert Flack]

Created attachment 173360 [details]
Patch

[WebKit Review Bot]

Attachment 173360 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3858:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3859:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 13 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 173360 [details]
Patch

Attachment 173360 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14790227

[Robert Flack]

Created attachment 173424 [details]
Patch

[WebKit Review Bot]

Attachment 173424 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3859:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3860:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 13 in 43 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 173424 [details]
Patch

Attachment 173424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14775817

[WebKit Review Bot]

Comment on attachment 173424 [details]
Patch

Attachment 173424 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14787433

New failing tests:
fast/xmlhttprequest/xmlhttprequest-get.xhtml
http/tests/workers/worker-importScriptsOnError.html
plugins/clicking-missing-plugin-fires-delegate.html

[Robert Flack]

Created attachment 173633 [details]
Patch

[WebKit Review Bot]

Attachment 173633 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3868:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3869:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 13 in 48 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 173633 [details]
Patch

Attachment 173633 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14679553

[WebKit Review Bot]

Comment on attachment 173633 [details]
Patch

Attachment 173633 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14806772

New failing tests:
plugins/clicking-missing-plugin-fires-delegate.html

[Robert Flack]

Created attachment 174436 [details]
Patch

[WebKit Review Bot]

Attachment 174436 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3867:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/page/EventHandler.cpp:3868:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 13 in 49 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Robert Flack]

(In reply to comment #78)
> Attachment 174436 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/plat..." exit_code: 1
> Source/WebCore/dom/WheelEvent.h:80:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/WheelEvent.cpp:44:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/WheelEvent.cpp:51:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/UIEventWithKeyState.h:48:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
> Source/WebCore/dom/MouseRelatedEvent.h:70:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/page/EventHandler.cpp:3867:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/page/EventHandler.cpp:3868:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/MouseEvent.cpp:63:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/MouseEvent.cpp:64:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/MouseEvent.cpp:197:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/MouseEvent.h:95:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/KeyboardEvent.cpp:103:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Source/WebCore/dom/MouseRelatedEvent.cpp:60:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 13 in 49 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

These are pre-existing indentation styles which are consistent with the rest of the files. Should I fix them, and if so just the affected methods or the rest of the file too?

I don't know why the mac bot hasn't run on the latest patch but I've fixed the compile error on a local mac webkit build.

[Robert Flack]

Created attachment 174465 [details]
Patch

[Adam Barth]

Comment on attachment 174465 [details]
Patch

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

> Source/WebCore/dom/Node.cpp:2601
> +    return EventDispatcher::dispatchEvent(this, KeyboardEventDispatchMediator::create(KeyboardEvent::create(event, document()->defaultView(), 1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(event.timestamp()))));

1000.0 -> unnamed constants are sadness.  Perhaps we need a version of monotonicTimeToZeroBasedDocumentTime that returns the correct units?

[James Robinson]

(In reply to comment #81)
> (From update of attachment 174465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174465&action=review
> 
> > Source/WebCore/dom/Node.cpp:2601
> > +    return EventDispatcher::dispatchEvent(this, KeyboardEventDispatchMediator::create(KeyboardEvent::create(event, document()->defaultView(), 1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(event.timestamp()))));
> 
> 1000.0 -> unnamed constants are sadness.  Perhaps we need a version of monotonicTimeToZeroBasedDocumentTime that returns the correct units?

Perhaps something like DOMTimeStamp.h's convertSecondsToDOMTimeStamp() but for DOMHighResTimeStamp?

[WebKit Review Bot]

Comment on attachment 174465 [details]
Patch

Attachment 174465 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14832954

New failing tests:
plugins/clicking-missing-plugin-fires-delegate.html

[Build Bot]

Comment on attachment 174465 [details]
Patch

Attachment 174465 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14861260

New failing tests:
fast/events/keyevent-iframe-removed-crash.html

[Robert Flack]

Created attachment 175697 [details]
Patch

[Robert Flack]

(In reply to comment #81)
> (From update of attachment 174465 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174465&action=review
> 
> > Source/WebCore/dom/Node.cpp:2601
> > +    return EventDispatcher::dispatchEvent(this, KeyboardEventDispatchMediator::create(KeyboardEvent::create(event, document()->defaultView(), 1000.0 * document()->loader()->timing()->monotonicTimeToZeroBasedDocumentTime(event.timestamp()))));
> 
> 1000.0 -> unnamed constants are sadness.  Perhaps we need a version of monotonicTimeToZeroBasedDocumentTime that returns the correct units?

Turns out we never actually wanted the number in seconds so I've renamed / modified this method to return in milliseconds.

As for the test failures they haven't reproduced locally for me (either the mac or cr-linux ones), is there any way I can get more information about the failures on the bots or is it possible it's just a flake?

[Build Bot]

Comment on attachment 175697 [details]
Patch

Attachment 175697 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14967476

New failing tests:
fast/events/keyevent-iframe-removed-crash.html

[WebKit Review Bot]

Comment on attachment 175697 [details]
Patch

Attachment 175697 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14966587

New failing tests:
plugins/clicking-missing-plugin-fires-delegate.html

[Robert Flack]

Created attachment 175826 [details]
Patch

[Robert Flack]

I've fixed the crash on the occasional test. It only reproduced when running the entire set of tests and was the result of using the document loader class before it existed. I guess the timing was different for an individual test. Now I've added a function on Document (suggested by Rafael) which checks whether DocumentLoader is set and returns 0 otherwise. I believe I've addressed all of the comments, please take another look. Thanks for the reviews!

[Robert Flack]

Ping, can you look at the latest patch? Thanks.

[Adam Barth]

> Ping, can you look at the latest patch? Thanks.

You = ?

[Robert Flack]

(In reply to comment #92)
> > Ping, can you look at the latest patch? Thanks.
> 
> You = ?

Sorry for the ambiguity, you = abarth :-).

[Adam Barth]

> Sorry for the ambiguity, you = abarth :-).

That's what I was afraid of!  I'll take a look shortly.

[Adam Barth]

Comment on attachment 175826 [details]
Patch

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

Honestly, I'm not super excited about this patch.  You're making these event constructors even more epic than they already are.  It seems odd that m_createTime and m_systemTime are intialized differently.  It's also unfortunate that we're going to end up with two timestamps for events.

Perhaps it would be better for someone else who is more excited about this feature to review this change?

> Source/WebCore/ChangeLog:10
> +        http://lists.w3.org/Archives/Public/public-web-perf/2012Oct/0046.html and

Is there more of a spec that this email?  That email just says that he's interested and might include the feature in v2.

> Source/WebCore/dom/MouseEvent.cpp:197
>      : MouseEvent(eventType, true, true, view, 0, 0, 0, 0, 0,
>  #if ENABLE(POINTER_LOCK)
> -                 0, 0,
> +        0, 0,
>  #endif
> -                 false, false, false, false, 0, 0, 0, true)
> +        false, false, false, false, 0, 0, 0, true, 0)

Epic initializers are epic.

[Robert Flack]

(In reply to comment #95)
> (From update of attachment 175826 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=175826&action=review
> 
> Honestly, I'm not super excited about this patch.  You're making these event constructors even more epic than they already are.  It seems odd that m_createTime and m_systemTime are intialized differently.  It's also unfortunate that we're going to end up with two timestamps for events.

It is odd, and unfortunate. I'm just not convinced there's a better way. For compatibility, we need the epoch based timeStamp property giving the creation time. The system time needs to be passed all the way up from the implementer's PlatformEvent time.

> Perhaps it would be better for someone else who is more excited about this feature to review this change?

Thanks for the suggestion. James, is this you?

> 
> > Source/WebCore/ChangeLog:10
> > +        http://lists.w3.org/Archives/Public/public-web-perf/2012Oct/0046.html and
> 
> Is there more of a spec that this email?  That email just says that he's interested and might include the feature in v2.

Not yet, to the best of my knowledge.

> 
> > Source/WebCore/dom/MouseEvent.cpp:197
> >      : MouseEvent(eventType, true, true, view, 0, 0, 0, 0, 0,
> >  #if ENABLE(POINTER_LOCK)
> > -                 0, 0,
> > +        0, 0,
> >  #endif
> > -                 false, false, false, false, 0, 0, 0, true)
> > +        false, false, false, false, 0, 0, 0, true, 0)
> 
> Epic initializers are epic.

[Ms2ger (he/him; ⌚ UTC+1/+2)]

Is this still something people want to add or can we close this bug?

[Chris Dumez]

*** This bug has been marked as a duplicate of bug 154246 ***

[Lucas Forschler]

Mass move bugs into the DOM component.