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

Description Eric Seidel (no email) 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
Comment 1 Dominic Cooney 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?
Comment 2 Eric Seidel (no email) 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.
Comment 3 Dominic Cooney 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.
Comment 4 WebKit Review Bot 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.
Comment 5 WebKit Review Bot 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
Comment 6 Timothy Hatcher 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.
Comment 7 Dominic Cooney 2012-06-01 09:32:25 PDT
Created attachment 145328 [details]
Patch
Comment 8 WebKit Review Bot 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.
Comment 9 Timothy Hatcher 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.
Comment 10 Kentaro Hara 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.
Comment 11 Dominic Cooney 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.
Comment 12 Dominic Cooney 2012-06-03 03:21:09 PDT
Created attachment 145478 [details]
Patch
Comment 13 Kentaro Hara 2012-06-03 03:37:13 PDT
Comment on attachment 145478 [details]
Patch

Thanks for the clarification. The patch looks OK.
Comment 14 WebKit Review Bot 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.
Comment 15 Dominic Cooney 2012-06-03 06:34:03 PDT
Comment on attachment 145478 [details]
Patch

Thanks for reviewing.
Comment 16 Kentaro Hara 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.
Comment 17 WebKit Review Bot 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>
Comment 18 WebKit Review Bot 2012-06-03 07:33:51 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 WebKit Review Bot 2012-06-03 08:42:29 PDT
Re-opened since this is blocked by 88189
Comment 20 Dominic Cooney 2012-06-03 09:26:32 PDT
Created attachment 145484 [details]
Patch
Comment 21 WebKit Review Bot 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.
Comment 22 WebKit Review Bot 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
Comment 23 WebKit Review Bot 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
Comment 24 Ryosuke Niwa 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
Comment 25 Dominic Cooney 2012-06-03 18:08:52 PDT
Comment on attachment 145484 [details]
Patch

Clearing r? while I look at browser test failures.
Comment 26 Dominic Cooney 2012-06-25 22:21:50 PDT
Created attachment 149453 [details]
Patch
Comment 27 Kentaro Hara 2012-06-25 22:23:59 PDT
What was the cause of the browser test failure? (How did the latest patch fix it?)
Comment 28 WebKit Review Bot 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.
Comment 29 Kentaro Hara 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.
Comment 30 WebKit Review Bot 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>
Comment 31 WebKit Review Bot 2012-06-26 00:41:57 PDT
All reviewed patches have been landed.  Closing bug.