Bug 76104

Summary: WheelEvent should inherit from MouseEvent
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: UI EventsAssignee: Dominic Cooney <dominicc>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, dglazkov, dominicc, haraken, hayato, japhet, jochen, ojan, rniwa, sam, timothy, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 88189    
Bug Blocks: 76198, 76214    
Attachments:
Description Flags
WIP patch, ifdefs to avoid ObjC API changes
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ec2-cr-linux-02
none
Patch none

Eric Seidel (no email)
Reported 2012-01-11 14:36:22 PST
WheelEvent should inherit from MouseEvent At least according to the most recent DOM3 Events spec I found: http://dev.w3.org/2006/webapi/DOM-Level-3-Events/html/DOM3-Events.html#events-wheelevents This is causing this IETC sample to fail: http://samples.msdn.microsoft.com/ietestcenter/dominheritance/showdominheritancetest.htm?Prototype_WheelEvent Looks like this file has been with us a long time. :) http://trac.webkit.org/log/trunk/Source/WebCore/dom/WheelEvent.h
Attachments
WIP patch, ifdefs to avoid ObjC API changes (8.25 KB, patch)
2012-01-12 02:24 PST, Dominic Cooney
no flags
Patch (16.13 KB, patch)
2012-06-01 09:32 PDT, Dominic Cooney
no flags
Patch (10.44 KB, patch)
2012-06-03 03:21 PDT, Dominic Cooney
no flags
Patch (11.57 KB, patch)
2012-06-03 09:26 PDT, Dominic Cooney
no flags
Archive of layout-test-results from ec2-cr-linux-02 (712.69 KB, application/zip)
2012-06-03 11:03 PDT, WebKit Review Bot
no flags
Patch (11.36 KB, patch)
2012-06-25 22:21 PDT, Dominic Cooney
no flags
Dominic Cooney
Comment 1 2012-01-12 01:51:38 PST
This would be a public API change for Objective-C. What is the right way to approach that aspect of this?
Eric Seidel (no email)
Comment 2 2012-01-12 02:02:15 PST
Tim is the man to talk to. But I doubt it will be a problem. It is unlikely that any WebKit users would depend on this exact inheritance chain in their Obj-C.
Dominic Cooney
Comment 3 2012-01-12 02:24:19 PST
Created attachment 122196 [details] WIP patch, ifdefs to avoid ObjC API changes Waiting for advice on ObjC API changes.
WebKit Review Bot
Comment 4 2012-01-12 02:26:46 PST
Attachment 122196 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/dom/WheelEvent.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 5 2012-01-12 02:55:57 PST
Comment on attachment 122196 [details] WIP patch, ifdefs to avoid ObjC API changes Attachment 122196 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/11170752
Timothy Hatcher
Comment 6 2012-01-12 09:40:18 PST
Since DOMMouseEvent inherits from DOMUIEvent, which was the old superclass for DOMWheelEvent there should be no problem moving DOMWheelEvent to inherit from DOMMouseEvent.
Dominic Cooney
Comment 7 2012-06-01 09:32:25 PDT
WebKit Review Bot
Comment 8 2012-06-01 09:34:14 PDT
Please wait for approval from timothy@apple.com (or another member of the Apple Safari Team) before submitting because this patch contains changes to the Apple Mac WebKit.framework public API.
Timothy Hatcher
Comment 9 2012-06-02 11:07:12 PDT
The ObjC changes are fine. I'll leave the rest for another reviewer that knows this area better.
Kentaro Hara
Comment 10 2012-06-02 16:58:48 PDT
Comment on attachment 145328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145328&action=review > Source/WebCore/ChangeLog:7 > + Please describe the link of the spec and the IETC test that support this change. > Source/WebCore/dom/WheelEvent.h:-34 > - class WheelEvent : public MouseRelatedEvent { Isn't there any concern to lose the inheritance from MouseRelatedEvent? I guess there might have been some reason behind the fact that MouseRelatedEvent was inherited instead of MouseEvent. > Source/WebCore/dom/WheelEvent.h:-85 > - }; We might not want to fix the indentation, to keep the git/svn history sane.
Dominic Cooney
Comment 11 2012-06-03 02:52:09 PDT
Comment on attachment 145328 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=145328&action=review >> Source/WebCore/dom/WheelEvent.h:-34 >> - class WheelEvent : public MouseRelatedEvent { > > Isn't there any concern to lose the inheritance from MouseRelatedEvent? I guess there might have been some reason behind the fact that MouseRelatedEvent was inherited instead of MouseEvent. MouseRelatedEvent was created to give a common interface between MouseEvent and WheelEvent. So WheelEvent still gets MouseRelatedEvent via MouseEvent. We can’t remove MouseRelatedEvent because it is also used by TouchEvent.
Dominic Cooney
Comment 12 2012-06-03 03:21:09 PDT
Kentaro Hara
Comment 13 2012-06-03 03:37:13 PDT
Comment on attachment 145478 [details] Patch Thanks for the clarification. The patch looks OK.
WebKit Review Bot
Comment 14 2012-06-03 04:25:09 PDT
Attachment 145478 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/dom/WheelEvent.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dominic Cooney
Comment 15 2012-06-03 06:34:03 PDT
Comment on attachment 145478 [details] Patch Thanks for reviewing.
Kentaro Hara
Comment 16 2012-06-03 06:35:35 PDT
(In reply to comment #15) > (From update of attachment 145478 [details]) > Thanks for reviewing. Maybe you need to land it manually to avoid the style-bot error.
WebKit Review Bot
Comment 17 2012-06-03 07:33:45 PDT
Comment on attachment 145478 [details] Patch Clearing flags on attachment: 145478 Committed r119359: <http://trac.webkit.org/changeset/119359>
WebKit Review Bot
Comment 18 2012-06-03 07:33:51 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 19 2012-06-03 08:42:29 PDT
Re-opened since this is blocked by 88189
Dominic Cooney
Comment 20 2012-06-03 09:26:32 PDT
WebKit Review Bot
Comment 21 2012-06-03 10:30:15 PDT
Attachment 145484 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/dom/WheelEvent.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Review Bot
Comment 22 2012-06-03 11:03:14 PDT
Comment on attachment 145484 [details] Patch Attachment 145484 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12890214 New failing tests: http/tests/media/media-source/video-media-source-event-attributes.html
WebKit Review Bot
Comment 23 2012-06-03 11:03:19 PDT
Created attachment 145489 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Ryosuke Niwa
Comment 24 2012-06-03 15:18:30 PDT
It appears that this patch also broke a couple of browser tests: http://build.chromium.org/p/chromium.webkit/builders/Win%20%28dbg%29/builds/6453
Dominic Cooney
Comment 25 2012-06-03 18:08:52 PDT
Comment on attachment 145484 [details] Patch Clearing r? while I look at browser test failures.
Dominic Cooney
Comment 26 2012-06-25 22:21:50 PDT
Kentaro Hara
Comment 27 2012-06-25 22:23:59 PDT
What was the cause of the browser test failure? (How did the latest patch fix it?)
WebKit Review Bot
Comment 28 2012-06-25 22:25:17 PDT
Attachment 149453 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/dom/WheelEvent.h:34: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Kentaro Hara
Comment 29 2012-06-25 23:14:01 PDT
Comment on attachment 149453 [details] Patch > What was the cause of the browser test failure? (How did the latest patch fix it?) Discussed with dominicc offline. WheelEvent::isMouseEvent() returns false in the latest patch. Looks OK.
WebKit Review Bot
Comment 30 2012-06-26 00:41:50 PDT
Comment on attachment 149453 [details] Patch Clearing flags on attachment: 149453 Committed r121234: <http://trac.webkit.org/changeset/121234>
WebKit Review Bot
Comment 31 2012-06-26 00:41:57 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.