Bug 76104

Summary:

WheelEvent should inherit from MouseEvent

Product:

WebKit

Reporter:

Eric Seidel (no email) <eric>

Component:

UI Events

Assignee:

Dominic Cooney <dominicc>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, dglazkov, dominicc, haraken, hayato, japhet, jochen, ojan, rniwa, sam, timothy, webkit.review.bot

Priority:

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	Details Formatted Diff Diff
Patch (16.13 KB, patch) 2012-06-01 09:32 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
Patch (10.44 KB, patch) 2012-06-03 03:21 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
Patch (11.57 KB, patch) 2012-06-03 09:26 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
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	Details
Patch (11.36 KB, patch) 2012-06-25 22:21 PDT, Dominic Cooney	no flags	Details Formatted Diff Diff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.

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

Created attachment 145328 [details] Patch

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

Created attachment 145478 [details] Patch

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

Created attachment 145484 [details] Patch

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

Created attachment 149453 [details] Patch

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.