[webkit-reviews] review granted: [Bug 94081] Implement DOM3 wheel event : [Attachment 209589] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Christophe Dumez <dchris at gmail.com>'s
request for review:
Bug 94081: Implement DOM3 wheel event
https://bugs.webkit.org/show_bug.cgi?id=94081

Attachment 209589: Patch
https://bugs.webkit.org/attachment.cgi?id=209589&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=209589&action=review


Is there enough test coverage here? There are so many changes!

> Source/WebCore/dom/Document.idl:272
> -    [NotEnumerable] attribute EventListener onmousewheel;
> +    [NotEnumerable] attribute EventListener onmousewheel; // Deprecated.

I’m don’t think we should add these comments in the IDL files. We don’t do this
elsewhere in other IDL files. And I’m not sure that people who need to know
this is deprecated will be reading IDL files.

We should think about finding a useful place to put a comment about
deprecation, where it will help someone make good decisions. Maybe somewhere we
can put more than a single word, too.

> Source/WebCore/dom/Element.idl:188
> +    [NotEnumerable] attribute EventListener onmousewheel; // Deprecated.

Ditto.

> Source/WebCore/dom/EventTarget.cpp:164
> +static AtomicString legacyType(const Event* event)

The return type here could and should be const AtomicString& since these are
all pre-allocated AtomicString objects.

> Source/WebCore/dom/EventTarget.cpp:185
> +    AtomicString legacyTypeName = legacyType(event);

Type here should be const AtomicString&.

> Source/WebCore/dom/EventTarget.cpp:200
> -    if (!prefixedTypeName.isEmpty()) {
> +    if (legacyTypeName == eventNames().webkitTransitionEndEvent) {

This code is now confusing. Before it made sense that the prefixing code was
based on whether the name was prefixed. But now this is looking for a specific
event name, so it at least needs a why comment. I suggest we add a function
called isPrefixedEventType or something like that just so this code is
readable, and then we won't need a comment.

> Source/WebCore/dom/WheelEvent.cpp:77
> +    , m_deltaX(-rawDelta.x())
> +    , m_deltaY(-rawDelta.y())

Old code did rounding, but this new code instead truncates. Is that better? Not
clear why the raw delta here is floating point but elsewhere is integer.

> Source/WebCore/dom/WheelEvent.cpp:99
> -    // Normalize to the Windows 120 multiple
>      m_wheelDelta = IntPoint(rawDeltaX * TickMultiplier, rawDeltaY *
TickMultiplier);

Might have been nicer to improve this comment rather than removing it. Also
seems strange that the rawDeltaX/Y arguments are integers here, given that they
get multiplied by 120.

> Source/WebCore/dom/WheelEvent.idl:36
> +    // Non-standard API

Comment is a good idea. Not sure that this is clear enough, though. Something
that mentions this is leftover from older pre-standard versions might be
useful; not sure.

> Source/WebCore/svg/SVGElementInstance.idl:71
> +    [NotEnumerable] attribute EventListener onmousewheel; // Deprecated.

Same thought about Deprecated comments in the IDL files.