Bug 94987 - Add systemTime to DOM events
Summary: Add systemTime to DOM events
Status: RESOLVED DUPLICATE of bug 154246
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: WebExposed
Depends on:
Blocks:
 
Reported: 2012-08-24 17:41 PDT by Robert Flack
Modified: 2019-02-06 09:18 PST (History)
17 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Flack 2012-08-24 17:41:30 PDT
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 Robert Flack 2012-08-24 17:47:04 PDT
Created attachment 160531 [details]
Patch
Comment 2 WebKit Review Bot 2012-08-24 17:48:57 PDT
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 Build Bot 2012-08-24 18:19:40 PDT
Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13596312
Comment 4 Gyuyoung Kim 2012-08-24 18:37:53 PDT
Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/13599312
Comment 5 Early Warning System Bot 2012-08-24 18:41:43 PDT
Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13599315
Comment 6 WebKit Review Bot 2012-08-24 18:54:37 PDT
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
Comment 7 WebKit Review Bot 2012-08-24 18:54:41 PDT
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
Comment 8 WebKit Review Bot 2012-08-24 19:56:26 PDT
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
Comment 9 WebKit Review Bot 2012-08-24 19:56:30 PDT
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
Comment 10 Build Bot 2012-08-25 00:16:42 PDT
Comment on attachment 160531 [details]
Patch

Attachment 160531 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13597398
Comment 11 Alexey Proskuryakov 2012-08-27 13:09:38 PDT
Has this been discussed on webkit-dev already? Could you post a link?
Comment 12 Robert Flack 2012-10-05 10:08:12 PDT
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 Ryosuke Niwa 2012-10-11 15:55:25 PDT
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.
Comment 14 Robert Flack 2012-10-12 06:58:01 PDT
Created attachment 168415 [details]
Patch
Comment 15 Adam Barth 2012-10-12 11:32:51 PDT
Comment on attachment 168415 [details]
Patch

We'll probably want an ENABLE and a runtime flag for this feature before landing it.
Comment 16 Adam Barth 2012-10-12 11:33:41 PDT
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 Adam Barth 2012-10-12 11:34:08 PDT
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 Ryosuke Niwa 2012-10-12 15:57:39 PDT
This feature should be announced on webkit-dev.
Comment 19 Robert Flack 2012-10-24 19:06:28 PDT
Created attachment 170536 [details]
Patch
Comment 20 WebKit Review Bot 2012-10-24 19:08:00 PDT
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 Robert Flack 2012-10-24 19:13:30 PDT
(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 Early Warning System Bot 2012-10-24 19:15:25 PDT
Comment on attachment 170536 [details]
Patch

Attachment 170536 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14566426
Comment 23 Adam Barth 2012-10-24 19:24:25 PDT
> 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 Adam Barth 2012-10-24 19:26:25 PDT
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.
Comment 25 Robert Flack 2012-10-25 07:44:58 PDT
Created attachment 170648 [details]
Patch
Comment 26 Robert Flack 2012-10-25 07:48:38 PDT
(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.
Comment 27 WebKit Review Bot 2012-10-25 07:52:00 PDT
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 WebKit Review Bot 2012-10-25 07:52:27 PDT
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 Early Warning System Bot 2012-10-25 08:02:19 PDT
Comment on attachment 170648 [details]
Patch

Attachment 170648 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14554664
Comment 30 Build Bot 2012-10-25 08:22:29 PDT
Comment on attachment 170648 [details]
Patch

Attachment 170648 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14554670
Comment 31 Robert Flack 2012-10-25 08:33:46 PDT
Created attachment 170658 [details]
Patch
Comment 32 WebKit Review Bot 2012-10-25 08:36:55 PDT
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 Robert Flack 2012-10-25 08:39:47 PDT
Created attachment 170659 [details]
Patch
Comment 34 WebKit Review Bot 2012-10-25 08:42:24 PDT
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 WebKit Review Bot 2012-10-25 10:19:41 PDT
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
Comment 36 WebKit Review Bot 2012-10-25 11:40:23 PDT
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
Comment 37 Ojan Vafai 2012-10-25 12:34:50 PDT
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?
Comment 38 Robert Flack 2012-10-25 12:36:12 PDT
(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.
Comment 39 Ojan Vafai 2012-10-25 12:37:02 PDT
Comment on attachment 170659 [details]
Patch

Removing from the review queue since the patch is being updated.
Comment 40 Build Bot 2012-10-25 14:30:40 PDT
Comment on attachment 170659 [details]
Patch

Attachment 170659 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14563718
Comment 41 Adam Barth 2012-10-25 14:47:19 PDT
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.
Comment 42 Robert Flack 2012-10-25 15:42:14 PDT
Created attachment 170750 [details]
Patch
Comment 43 Robert Flack 2012-10-25 15:45:06 PDT
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.
Comment 44 Adam Barth 2012-10-25 15:50:00 PDT
> 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 Robert Flack 2012-10-26 11:49:49 PDT
Created attachment 170977 [details]
Patch
Comment 46 WebKit Review Bot 2012-10-26 11:53:48 PDT
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 Robert Flack 2012-10-26 11:59:18 PDT
Created attachment 170978 [details]
Patch
Comment 48 WebKit Review Bot 2012-10-26 12:05:18 PDT
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 Build Bot 2012-10-26 12:39:59 PDT
Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14612117
Comment 50 Build Bot 2012-10-26 12:46:06 PDT
Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14606197
Comment 51 Peter Beverloo (cr-android ews) 2012-10-26 13:59:23 PDT
Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14611155
Comment 52 EFL EWS Bot 2012-10-26 15:04:24 PDT
Comment on attachment 170978 [details]
Patch

Attachment 170978 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14613187
Comment 53 WebKit Review Bot 2012-10-26 16:05:44 PDT
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
Comment 54 WebKit Review Bot 2012-10-26 17:03:57 PDT
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
Comment 55 Rafael Brandao 2012-10-26 17:43:47 PDT
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).
Comment 56 Robert Flack 2012-11-08 07:50:37 PST
Created attachment 173044 [details]
Patch
Comment 57 WebKit Review Bot 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 Robert Flack 2012-11-08 08:32:47 PST
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.
Comment 59 Build Bot 2012-11-08 08:59:59 PST
Comment on attachment 173044 [details]
Patch

Attachment 173044 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14755950
Comment 60 Build Bot 2012-11-08 09:23:18 PST
Comment on attachment 173044 [details]
Patch

Attachment 173044 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14760863
Comment 61 WebKit Review Bot 2012-11-08 22:42:50 PST
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
Comment 62 Robert Flack 2012-11-09 12:57:01 PST
Created attachment 173346 [details]
Patch
Comment 63 WebKit Review Bot 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 Build Bot 2012-11-09 13:28:57 PST
Comment on attachment 173346 [details]
Patch

Attachment 173346 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14796069
Comment 65 Build Bot 2012-11-09 13:31:01 PST
Comment on attachment 173346 [details]
Patch

Attachment 173346 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14779536
Comment 66 Robert Flack 2012-11-09 14:08:03 PST
Created attachment 173360 [details]
Patch
Comment 67 WebKit Review Bot 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 Build Bot 2012-11-09 14:48:07 PST
Comment on attachment 173360 [details]
Patch

Attachment 173360 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14790227
Comment 69 Robert Flack 2012-11-09 19:59:43 PST
Created attachment 173424 [details]
Patch
Comment 70 WebKit Review Bot 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 Build Bot 2012-11-09 20:33:56 PST
Comment on attachment 173424 [details]
Patch

Attachment 173424 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14775817
Comment 72 WebKit Review Bot 2012-11-10 01:50:29 PST
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
Comment 73 Robert Flack 2012-11-12 05:56:18 PST
Created attachment 173633 [details]
Patch
Comment 74 WebKit Review Bot 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 Build Bot 2012-11-12 07:08:01 PST
Comment on attachment 173633 [details]
Patch

Attachment 173633 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14679553
Comment 76 WebKit Review Bot 2012-11-12 16:56:44 PST
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
Comment 77 Robert Flack 2012-11-15 07:19:22 PST
Created attachment 174436 [details]
Patch
Comment 78 WebKit Review Bot 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 Robert Flack 2012-11-15 09:05:01 PST
(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.
Comment 80 Robert Flack 2012-11-15 09:24:21 PST
Created attachment 174465 [details]
Patch
Comment 81 Adam Barth 2012-11-15 10:41:32 PST
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?
Comment 82 James Robinson 2012-11-15 13:38:31 PST
(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?
Comment 83 WebKit Review Bot 2012-11-15 14:19:23 PST
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
Comment 84 Build Bot 2012-11-16 10:55:34 PST
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
Comment 85 Robert Flack 2012-11-22 11:57:30 PST
Created attachment 175697 [details]
Patch
Comment 86 Robert Flack 2012-11-22 11:59:21 PST
(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?
Comment 87 Build Bot 2012-11-22 13:22:09 PST
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
Comment 88 WebKit Review Bot 2012-11-22 21:05:14 PST
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
Comment 89 Robert Flack 2012-11-23 10:05:46 PST
Created attachment 175826 [details]
Patch
Comment 90 Robert Flack 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 Robert Flack 2013-01-11 05:57:51 PST
Ping, can you look at the latest patch? Thanks.
Comment 92 Adam Barth 2013-01-11 10:18:55 PST
> Ping, can you look at the latest patch? Thanks.

You = ?
Comment 93 Robert Flack 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 Adam Barth 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 Adam Barth 2013-01-11 12:23:27 PST
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.
Comment 96 Robert Flack 2013-01-17 21:34:30 PST
(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.
Comment 97 Ms2ger (he/him; ⌚ UTC+1/+2) 2017-10-11 02:19:47 PDT
Is this still something people want to add or can we close this bug?
Comment 98 Chris Dumez 2017-10-11 09:05:09 PDT

*** This bug has been marked as a duplicate of bug 154246 ***
Comment 99 Lucas Forschler 2019-02-06 09:18:52 PST
Mass move bugs into the DOM component.