Per the spec, WheelEvent::deltaMode should return DOM_DELTA_PIXEL or DOM_DELTA_LINE or DOM_DELTA_PAGE. Spec: http://www.w3.org/TR/DOM-Level-3-Events/#events-wheelevents https://dvcs.w3.org/hg/d4e/raw-file/tip/source_respec.htm#constructor-wheelevent
Created attachment 185740 [details] Patch
Attachment 185740 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/events/constructors/wheel-event-constructor-expected.txt', u'LayoutTests/fast/events/constructors/wheel-event-constructor.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/WheelEvent.cpp', u'Source/WebCore/dom/WheelEvent.h', u'Source/WebCore/dom/WheelEvent.idl', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebInputEventConversion.cpp']" exit_code: 1 Source/WebCore/dom/WheelEvent.h:46: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/WheelEvent.h:47: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/WheelEvent.h:48: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 185740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185740&action=review >> Source/WebCore/dom/WheelEvent.h:46 >> + DOM_DELTA_PIXEL = 0, > > enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] This is OK. We conventionally use the same constant name as the Web IDL spec.
Comment on attachment 185740 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185740&action=review > Source/WebCore/dom/WheelEvent.idl:28 > + const unsigned long DOM_DELTA_PIXEL = 0x00; > + const unsigned long DOM_DELTA_LINE = 0x01; > + const unsigned long DOM_DELTA_PAGE = 0x02; > + Are these constants on the WheelEvent constructor or on the instances? > Source/WebCore/page/EventHandler.cpp:256 > switch (deltaMode) { Should we add a default case to this switch now that the type of deltaMode is unsigned rather than WheelEvent::DeltaMode ?
The spec says: ---8<--- Initializes the deltaMode attribute on the WheelEvent object to the enumerated values 0, 1, or 2, which represent the amount of pixels scrolled (DOM_DELTA_PIXEL), lines scrolled (DOM_DELTA_LINE), or pages scrolled (DOM_DELTA_PAGE) if the rotation of the wheel would have resulted in scrolling. --->8--- Does that mean the constructor should enforce that deltaMode is only initialized to 0, 1, or 2? If the caller supplies the value 42, what is supposed to happen?
Created attachment 185895 [details] Patch
(In reply to comment #4) > > Source/WebCore/dom/WheelEvent.idl:28 > > + const unsigned long DOM_DELTA_PIXEL = 0x00; > > + const unsigned long DOM_DELTA_LINE = 0x01; > > + const unsigned long DOM_DELTA_PAGE = 0x02; > > + > > Are these constants on the WheelEvent constructor or on the instances? Both. The spec requires both (http://www.w3.org/TR/WebIDL/#es-constants). The WebKit implementation is conformant with the spec. > > Source/WebCore/page/EventHandler.cpp:256 > > switch (deltaMode) { > > Should we add a default case to this switch now that the type of deltaMode is unsigned rather than WheelEvent::DeltaMode ? Done. > The spec says: > > ---8<--- > Initializes the deltaMode attribute on the WheelEvent object to the enumerated values 0, 1, or 2, which represent the amount of pixels scrolled (DOM_DELTA_PIXEL), lines scrolled (DOM_DELTA_LINE), or pages scrolled (DOM_DELTA_PAGE) if the rotation of the wheel would have resulted in scrolling. > --->8--- > > Does that mean the constructor should enforce that deltaMode is only initialized to 0, 1, or 2? If the caller supplies the value 42, what is supposed to happen? Good point. As far as I read the spec, I couldn't find any explicit description about that. Given that the type of deltaMode is unsigned, I think the behavior should follow the spec of normal unsigned attributes. If so, assigning 42 will result in 42. (c.f. If deltaMode is speced as enum, invalid value assignment will be simply ignored. http://www.w3.org/TR/WebIDL/#idl-enums)
Attachment 185895 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/events/constructors/wheel-event-constructor-expected.txt', u'LayoutTests/fast/events/constructors/wheel-event-constructor.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/dom/WheelEvent.cpp', u'Source/WebCore/dom/WheelEvent.h', u'Source/WebCore/dom/WheelEvent.idl', u'Source/WebCore/page/EventHandler.cpp', u'Source/WebKit/chromium/ChangeLog', u'Source/WebKit/chromium/src/WebInputEventConversion.cpp']" exit_code: 1 Source/WebCore/dom/WheelEvent.h:46: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/WheelEvent.h:47: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Source/WebCore/dom/WheelEvent.h:48: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 10 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 185895 [details] Patch Ok. Would you be willing to double-check with the working group about the out-of-bounds case?
(In reply to comment #9) > (From update of attachment 185895 [details]) > Ok. Would you be willing to double-check with the working group about the out-of-bounds case? Sure. Will do before landing.
(In reply to comment #10) > (In reply to comment #9) > > (From update of attachment 185895 [details] [details]) > > Ok. Would you be willing to double-check with the working group about the out-of-bounds case? > > Sure. Will do before landing. I discussed with spec authors. In conclusion, we can set out-of-bound values: https://www.w3.org/Bugs/Public/show_bug.cgi?id=14053#c6 Will land the patch soon.
Committed r141826: <http://trac.webkit.org/changeset/141826>