Bug 94987 - Add systemTime to DOM events
: Add systemTime to DOM events
Status: ASSIGNED
: WebKit
HTML Events
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: WebExposed
:
:
  Show dependency treegraph
 
Reported: 2012-08-24 17:41 PST by
Modified: 2013-04-09 12:21 PST (History)


Attachments
Patch (32.57 KB, patch)
2012-08-24 17:47 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-01 (560.29 KB, application/zip)
2012-08-24 18:54 PST, WebKit Review Bot
no flags Details
Archive of layout-test-results from gce-cr-linux-03 (622.88 KB, application/zip)
2012-08-24 19:56 PST, WebKit Review Bot
no flags Details
Patch (32.67 KB, patch)
2012-10-12 06:58 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (32.84 KB, patch)
2012-10-24 19:06 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (38.61 KB, patch)
2012-10-25 07:44 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (38.62 KB, patch)
2012-10-25 08:33 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (38.88 KB, patch)
2012-10-25 08:39 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (46.58 KB, patch)
2012-10-25 15:42 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (48.96 KB, patch)
2012-10-26 11:49 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (49.25 KB, patch)
2012-10-26 11:59 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (49.62 KB, patch)
2012-11-08 07:50 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (57.08 KB, patch)
2012-11-09 12:57 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (59.12 KB, patch)
2012-11-09 14:08 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (59.12 KB, patch)
2012-11-09 19:59 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (62.66 KB, patch)
2012-11-12 05:56 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (65.05 KB, patch)
2012-11-15 07:19 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (68.11 KB, patch)
2012-11-15 09:24 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (71.04 KB, patch)
2012-11-22 11:57 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff
Patch (72.26 KB, patch)
2012-11-23 10:05 PST, Robert Flack
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-08-24 17:41:30 PST
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.
------- Comment #1 From 2012-08-24 17:47:04 PST -------
Created an attachment (id=160531) [details]
Patch
------- Comment #2 From 2012-08-24 17:48:57 PST -------
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.
------- Comment #3 From 2012-08-24 18:19:40 PST -------
(From update of attachment 160531 [details])
Attachment 160531 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13596312
------- Comment #4 From 2012-08-24 18:37:53 PST -------
(From update of attachment 160531 [details])
Attachment 160531 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13599312
------- Comment #5 From 2012-08-24 18:41:43 PST -------
(From update of attachment 160531 [details])
Attachment 160531 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13599315
------- Comment #6 From 2012-08-24 18:54:37 PST -------
(From update of attachment 160531 [details])
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
------- Comment #7 From 2012-08-24 18:54:41 PST -------
Created an attachment (id=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
------- Comment #8 From 2012-08-24 19:56:26 PST -------
(From update of attachment 160531 [details])
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
------- Comment #9 From 2012-08-24 19:56:30 PST -------
Created an attachment (id=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
------- Comment #10 From 2012-08-25 00:16:42 PST -------
(From update of attachment 160531 [details])
Attachment 160531 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13597398
------- Comment #11 From 2012-08-27 13:09:38 PST -------
Has this been discussed on webkit-dev already? Could you post a link?
------- Comment #12 From 2012-10-05 10:08:12 PST -------
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?
------- Comment #13 From 2012-10-11 15:55:25 PST -------
(From update of attachment 160531 [details])
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.
------- Comment #14 From 2012-10-12 06:58:01 PST -------
Created an attachment (id=168415) [details]
Patch
------- Comment #15 From 2012-10-12 11:32:51 PST -------
(From update of attachment 168415 [details])
We'll probably want an ENABLE and a runtime flag for this feature before landing it.
------- Comment #16 From 2012-10-12 11:33:41 PST -------
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.
------- Comment #17 From 2012-10-12 11:34:08 PST -------
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.
------- Comment #18 From 2012-10-12 15:57:39 PST -------
This feature should be announced on webkit-dev.
------- Comment #19 From 2012-10-24 19:06:28 PST -------
Created an attachment (id=170536) [details]
Patch
------- Comment #20 From 2012-10-24 19:08:00 PST -------
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.
------- Comment #21 From 2012-10-24 19:13:30 PST -------
(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?
------- Comment #22 From 2012-10-24 19:15:25 PST -------
(From update of attachment 170536 [details])
Attachment 170536 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14566426
------- Comment #23 From 2012-10-24 19:24:25 PST -------
> we can't change the exposed properties on DOM events (Event.idl) from runtime preferences can we?

Yes we can.  grep for runtimeenabled
------- Comment #24 From 2012-10-24 19:26:25 PST -------
(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.  :(

> 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.
------- Comment #25 From 2012-10-25 07:44:58 PST -------
Created an attachment (id=170648) [details]
Patch
------- Comment #26 From 2012-10-25 07:48:38 PST -------
(In reply to comment #24)
> (From update of attachment 170536 [details] [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.
------- Comment #27 From 2012-10-25 07:52:00 PST -------
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
------- Comment #28 From 2012-10-25 07:52:27 PST -------
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.
------- Comment #29 From 2012-10-25 08:02:19 PST -------
(From update of attachment 170648 [details])
Attachment 170648 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14554664
------- Comment #30 From 2012-10-25 08:22:29 PST -------
(From update of attachment 170648 [details])
Attachment 170648 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14554670
------- Comment #31 From 2012-10-25 08:33:46 PST -------
Created an attachment (id=170658) [details]
Patch
------- Comment #32 From 2012-10-25 08:36:55 PST -------
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.
------- Comment #33 From 2012-10-25 08:39:47 PST -------
Created an attachment (id=170659) [details]
Patch
------- Comment #34 From 2012-10-25 08:42:24 PST -------
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.
------- Comment #35 From 2012-10-25 10:19:41 PST -------
(From update of attachment 170659 [details])
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
------- Comment #36 From 2012-10-25 11:40:23 PST -------
(From update of attachment 170659 [details])
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
------- Comment #37 From 2012-10-25 12:34:50 PST -------
(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?
------- Comment #38 From 2012-10-25 12:36:12 PST -------
(In reply to comment #37)
> (From update of attachment 170659 [details] [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.
------- Comment #39 From 2012-10-25 12:37:02 PST -------
(From update of attachment 170659 [details])
Removing from the review queue since the patch is being updated.
------- Comment #40 From 2012-10-25 14:30:40 PST -------
(From update of attachment 170659 [details])
Attachment 170659 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14563718
------- Comment #41 From 2012-10-25 14:47:19 PST -------
(From update of attachment 170659 [details])
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.
------- Comment #42 From 2012-10-25 15:42:14 PST -------
Created an attachment (id=170750) [details]
Patch
------- Comment #43 From 2012-10-25 15:45:06 PST -------
(From update of attachment 170659 [details])
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.
------- Comment #44 From 2012-10-25 15:50:00 PST -------
> 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.
------- Comment #45 From 2012-10-26 11:49:49 PST -------
Created an attachment (id=170977) [details]
Patch
------- Comment #46 From 2012-10-26 11:53:48 PST -------
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.
------- Comment #47 From 2012-10-26 11:59:18 PST -------
Created an attachment (id=170978) [details]
Patch
------- Comment #48 From 2012-10-26 12:05:18 PST -------
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.
------- Comment #49 From 2012-10-26 12:39:59 PST -------
(From update of attachment 170978 [details])
Attachment 170978 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14612117
------- Comment #50 From 2012-10-26 12:46:06 PST -------
(From update of attachment 170978 [details])
Attachment 170978 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14606197
------- Comment #51 From 2012-10-26 13:59:23 PST -------
(From update of attachment 170978 [details])
Attachment 170978 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14611155
------- Comment #52 From 2012-10-26 15:04:24 PST -------
(From update of attachment 170978 [details])
Attachment 170978 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14613187
------- Comment #53 From 2012-10-26 16:05:44 PST -------
(From update of attachment 170978 [details])
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
------- Comment #54 From 2012-10-26 17:03:57 PST -------
(From update of attachment 170978 [details])
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
------- Comment #55 From 2012-10-26 17:43:47 PST -------
(From update of attachment 170978 [details])
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).
------- Comment #56 From 2012-11-08 07:50:37 PST -------
Created an attachment (id=173044) [details]
Patch
------- Comment #57 From 2012-11-08 07:54:26 PST -------
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.
------- Comment #58 From 2012-11-08 08:32:47 PST -------
(From update of attachment 170978 [details])
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.
------- Comment #59 From 2012-11-08 08:59:59 PST -------
(From update of attachment 173044 [details])
Attachment 173044 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14755950
------- Comment #60 From 2012-11-08 09:23:18 PST -------
(From update of attachment 173044 [details])
Attachment 173044 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14760863
------- Comment #61 From 2012-11-08 22:42:50 PST -------
(From update of attachment 173044 [details])
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
------- Comment #62 From 2012-11-09 12:57:01 PST -------
Created an attachment (id=173346) [details]
Patch
------- Comment #63 From 2012-11-09 12:59:02 PST -------
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.
------- Comment #64 From 2012-11-09 13:28:57 PST -------
(From update of attachment 173346 [details])
Attachment 173346 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14796069
------- Comment #65 From 2012-11-09 13:31:01 PST -------
(From update of attachment 173346 [details])
Attachment 173346 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14779536
------- Comment #66 From 2012-11-09 14:08:03 PST -------
Created an attachment (id=173360) [details]
Patch
------- Comment #67 From 2012-11-09 14:10:57 PST -------
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.
------- Comment #68 From 2012-11-09 14:48:07 PST -------
(From update of attachment 173360 [details])
Attachment 173360 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14790227
------- Comment #69 From 2012-11-09 19:59:43 PST -------
Created an attachment (id=173424) [details]
Patch
------- Comment #70 From 2012-11-09 20:03:04 PST -------
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.
------- Comment #71 From 2012-11-09 20:33:56 PST -------
(From update of attachment 173424 [details])
Attachment 173424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14775817
------- Comment #72 From 2012-11-10 01:50:29 PST -------
(From update of attachment 173424 [details])
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
------- Comment #73 From 2012-11-12 05:56:18 PST -------
Created an attachment (id=173633) [details]
Patch
------- Comment #74 From 2012-11-12 05:58:34 PST -------
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.
------- Comment #75 From 2012-11-12 07:08:01 PST -------
(From update of attachment 173633 [details])
Attachment 173633 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14679553
------- Comment #76 From 2012-11-12 16:56:44 PST -------
(From update of attachment 173633 [details])
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
------- Comment #77 From 2012-11-15 07:19:22 PST -------
Created an attachment (id=174436) [details]
Patch
------- Comment #78 From 2012-11-15 07:26:50 PST -------
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.
------- Comment #79 From 2012-11-15 09:05:01 PST -------
(In reply to comment #78)
> Attachment 174436 [details] [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.
------- Comment #80 From 2012-11-15 09:24:21 PST -------
Created an attachment (id=174465) [details]
Patch
------- Comment #81 From 2012-11-15 10:41:32 PST -------
(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?
------- Comment #82 From 2012-11-15 13:38:31 PST -------
(In reply to comment #81)
> (From update of attachment 174465 [details] [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?
------- Comment #83 From 2012-11-15 14:19:23 PST -------
(From update of attachment 174465 [details])
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
------- Comment #84 From 2012-11-16 10:55:34 PST -------
(From update of attachment 174465 [details])
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
------- Comment #85 From 2012-11-22 11:57:30 PST -------
Created an attachment (id=175697) [details]
Patch
------- Comment #86 From 2012-11-22 11:59:21 PST -------
(In reply to comment #81)
> (From update of attachment 174465 [details] [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?
------- Comment #87 From 2012-11-22 13:22:09 PST -------
(From update of attachment 175697 [details])
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
------- Comment #88 From 2012-11-22 21:05:14 PST -------
(From update of attachment 175697 [details])
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
------- Comment #89 From 2012-11-23 10:05:46 PST -------
Created an attachment (id=175826) [details]
Patch
------- Comment #90 From 2012-11-27 20:32:07 PST -------
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!
------- Comment #91 From 2013-01-11 05:57:51 PST -------
Ping, can you look at the latest patch? Thanks.
------- Comment #92 From 2013-01-11 10:18:55 PST -------
> Ping, can you look at the latest patch? Thanks.

You = ?
------- Comment #93 From 2013-01-11 10:24:37 PST -------
(In reply to comment #92)
> > Ping, can you look at the latest patch? Thanks.
> 
> You = ?

Sorry for the ambiguity, you = abarth :-).
------- Comment #94 From 2013-01-11 10:48:40 PST -------
> Sorry for the ambiguity, you = abarth :-).

That's what I was afraid of!  I'll take a look shortly.
------- Comment #95 From 2013-01-11 12:23:27 PST -------
(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.

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.
------- Comment #96 From 2013-01-17 21:34:30 PST -------
(In reply to comment #95)
> (From update of attachment 175826 [details] [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.