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.
Created attachment 160531 [details] Patch
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 on attachment 160531 [details] Patch Attachment 160531 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13596312
Comment on attachment 160531 [details] Patch Attachment 160531 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/13599312
Comment on attachment 160531 [details] Patch Attachment 160531 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13599315
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
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 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
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 on attachment 160531 [details] Patch Attachment 160531 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13597398
Has this been discussed on webkit-dev already? Could you post a link?
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 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.
Created attachment 168415 [details] Patch
Comment on attachment 168415 [details] Patch We'll probably want an ENABLE and a runtime flag for this feature before landing it.
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.
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.
This feature should be announced on webkit-dev.
Created attachment 170536 [details] Patch
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.
(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 on attachment 170536 [details] Patch Attachment 170536 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14566426
> we can't change the exposed properties on DOM events (Event.idl) from runtime preferences can we? Yes we can. grep for runtimeenabled
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.
Created attachment 170648 [details] Patch
(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.
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.
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 on attachment 170648 [details] Patch Attachment 170648 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14554664
Comment on attachment 170648 [details] Patch Attachment 170648 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14554670
Created attachment 170658 [details] Patch
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.
Created attachment 170659 [details] Patch
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 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 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 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?
(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 on attachment 170659 [details] Patch Removing from the review queue since the patch is being updated.
Comment on attachment 170659 [details] Patch Attachment 170659 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14563718
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.
Created attachment 170750 [details] Patch
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.
> 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.
Created attachment 170977 [details] Patch
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.
Created attachment 170978 [details] Patch
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 on attachment 170978 [details] Patch Attachment 170978 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14612117
Comment on attachment 170978 [details] Patch Attachment 170978 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14606197
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 on attachment 170978 [details] Patch Attachment 170978 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/14613187
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 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 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).
Created attachment 173044 [details] Patch
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 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 on attachment 173044 [details] Patch Attachment 173044 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14755950
Comment on attachment 173044 [details] Patch Attachment 173044 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14760863
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
Created attachment 173346 [details] Patch
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 on attachment 173346 [details] Patch Attachment 173346 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14796069
Comment on attachment 173346 [details] Patch Attachment 173346 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/14779536
Created attachment 173360 [details] Patch
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 on attachment 173360 [details] Patch Attachment 173360 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14790227
Created attachment 173424 [details] Patch
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 on attachment 173424 [details] Patch Attachment 173424 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14775817
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
Created attachment 173633 [details] Patch
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 on attachment 173633 [details] Patch Attachment 173633 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14679553
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
Created attachment 174436 [details] Patch
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.
(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.
Created attachment 174465 [details] Patch
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?
(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 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 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
Created attachment 175697 [details] Patch
(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 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 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
Created attachment 175826 [details] Patch
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!
Ping, can you look at the latest patch? Thanks.
> Ping, can you look at the latest patch? Thanks. You = ?
(In reply to comment #92) > > Ping, can you look at the latest patch? Thanks. > > You = ? Sorry for the ambiguity, you = abarth :-).
> Sorry for the ambiguity, you = abarth :-). That's what I was afraid of! I'll take a look shortly.
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.
(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.
Is this still something people want to add or can we close this bug?
*** This bug has been marked as a duplicate of bug 154246 ***
Mass move bugs into the DOM component.