WheelEvent should inherit from MouseEvent

• Component: UI Events
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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

[Dominic Cooney]

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)]

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]

Created attachment 122196 [details]
WIP patch, ifdefs to avoid ObjC API changes

Waiting for advice on ObjC API changes.

[WebKit Review Bot]

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 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]

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]

Created attachment 145328 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail] (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]

The ObjC changes are fine. I'll leave the rest for another reviewer that knows this area better.

[Kentaro Hara]

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 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]

Created attachment 145478 [details]
Patch

[Kentaro Hara]

Comment on attachment 145478 [details]
Patch

Thanks for the clarification. The patch looks OK.

[WebKit Review Bot]

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 on attachment 145478 [details]
Patch

Thanks for reviewing.

[Kentaro Hara]

(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 on attachment 145478 [details]
Patch

Clearing flags on attachment: 145478

Committed r119359: <http://trac.webkit.org/changeset/119359>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by 88189

[Dominic Cooney]

Created attachment 145484 [details]
Patch

[WebKit Review Bot]

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 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]

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]

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 on attachment 145484 [details]
Patch

Clearing r? while I look at browser test failures.

[Dominic Cooney]

Created attachment 149453 [details]
Patch

[Kentaro Hara]

What was the cause of the browser test failure? (How did the latest patch fix it?)

[WebKit Review Bot]

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 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 on attachment 149453 [details]
Patch

Clearing flags on attachment: 149453

Committed r121234: <http://trac.webkit.org/changeset/121234>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.