[Webkit-unassigned] [Bug 105463] [Feature] Implement pointer events

Tue Feb 19 23:05:55 PST 2013

https://bugs.webkit.org/show_bug.cgi?id=105463

Adam Barth <abarth at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #189248|                            |review-
               Flag|                            |

--- Comment #12 from Adam Barth <abarth at webkit.org>  2013-02-19 23:08:15 PST ---
(From update of attachment 189248)
View in context: https://bugs.webkit.org/attachment.cgi?id=189248&action=review

Below are a large number of minor style comments.  This patch needs a bit of work before we can land it.  I haven't reviewed much beyond style and structure.  Also, we'll need LayoutTests for this feature.

> Source/WebCore/dom/Document.h:10
> + * Portions Copyright Microsoft Open Technologies, Inc. All rights reserved.

Is it possible to use a copyright notice similar to the ones above (e.g., with a year)?

> Source/WebCore/dom/Document.h:294
> +    DEFINE_ATTRIBUTE_EVENT_LISTENER(webkitpointerdown);

We'll probably want to implement pointer events without a vendor prefix given the current state of its standardization.

> Source/WebCore/dom/Node.cpp:2398
> +bool Node::dispatchPointerEvent(const PlatformPointerEvent& e, const AtomicString& eventType,
> +    int detail, Node* relatedTarget)

This should be on one line.

> Source/WebCore/dom/Node.cpp:2400
> +    return EventDispatcher::dispatchEvent(this, PointerEventDispatchMediator::create(PointerEvent::create(eventType, document()->defaultView(), e, detail, relatedTarget)));

e -> event

> Source/WebCore/dom/PointerEvent.cpp:29
> +#include "config.h"
> +#include "MouseEvent.h"
> +#include "PointerEvent.h"

Please move MouseEvent.h into the #include group below (alphabetized).

> Source/WebCore/dom/PointerEvent.cpp:38
> +

Please remove this extra blank line.

> Source/WebCore/dom/PointerEvent.cpp:42
> +// PointerEvent is based on MouseEvent
> +//

Please remove these comments.

> Source/WebCore/dom/PointerEvent.cpp:68
> +PointerEvent::PointerEvent(
> +			const AtomicString& type, 

Please use spaces instead of tabs.

> Source/WebCore/dom/PointerEvent.cpp:114
> +    m_pointerId(eventInfo.m_pointerId),

WebKit style is to begin these lines with ", " and then the name of the member variable.

> Source/WebCore/dom/PointerEvent.cpp:157
> +int PointerEvent::ConvertPointerTypeNameToInt(const String& name)

Should this function use AtomicStrings for faster comparison?  Also, the "C" should be lower-case.  WebKit uses camelCase names for functions.

> Source/WebCore/dom/PointerEvent.cpp:159
> +    if (name == "mouse") {

No need for { } with one-line conditionals.

> Source/WebCore/dom/PointerEvent.cpp:172
> +const AtomicString PointerEvent::ConvertPointerTypeIntToString(int pointerType) const

Why does this return a "const" string?  That doesn't make much sense.

> Source/WebCore/dom/PointerEvent.cpp:175
> +    switch(pointerType)
> +    {

These lines should be combined.

> Source/WebCore/dom/PointerEvent.cpp:190
> +inline static int adjustedClientX(int innerClientX, HTMLIFrameElement* iframe, FrameView* frameView)

Please move static functions to the top of the file (just inside the "namespace WebCore" declaration).  Also, the "inline" keyword is redundant with the "static" keyword here.

> Source/WebCore/dom/PointerEvent.cpp:217
> +    if (dispatcher->node()->disabled()) // Don't even send DOM events for disabled controls..

Please remove this comment.  WebKit enjoys comments that explain the "why" behind the code.  Comments like this that just say "what" the code does are redundant with the code itself.

> Source/WebCore/dom/PointerEvent.cpp:225
> +    if ((event()->type() != eventNames().webkitpointeroverEvent)  &&
> +       (event()->type() != eventNames().webkitpointeroutEvent)) 
> +    {

These lines do not conform to the WebKit style guide.

> Source/WebCore/dom/PointerEvent.h:40
> +    class PointerEvent : public MouseEvent {

Class declarations should not be indented.

> Source/WebCore/dom/PointerEvent.h:92
> +        // provide interface name for pointer object

I'm not sure what you're trying to say with this comment.  Perhaps we should remove it?

> Source/WebCore/dom/PointerEvent.h:95
> +        // Overload to separate from MouseEvent

This comment is redundant with the code.

> Source/WebCore/dom/PointerEvent.h:100
> +        const AtomicString pointerType() const { return ConvertPointerTypeIntToString(m_pointerType); }

The "const" here doesn't really make sense.

> Source/WebCore/dom/PointerEvent.h:127
> +    protected:

Why protected and not private?

> Source/WebCore/dom/PointerEvent.idl:12
> +[
> +    Conditional=POINTER_EVENTS
> +] interface PointerEvent : MouseEvent {

We should use ConstructorTemplate=Event to allow for DOM4-style construction of PointerEvents.

> Source/WebCore/dom/PointerEvent.idl:16
> +    [InitializedByEventConstructor] readonly attribute boolean isPrimary;
> +    [InitializedByEventConstructor] readonly attribute long pointerId;
> +    [InitializedByEventConstructor] readonly attribute DOMString pointerType;

InitializedByEventConstructor <-- These attributes are good, but they require the ConstructorTemplate attribute on the interface in order to work.

> Source/WebCore/dom/PointerEvent.idl:18
> +    [ObjCLegacyUnnamedParameters] void initPointerEvent(in [Optional=DefaultIsUndefined] DOMString type, 

Please remove initPointerEvent.  Instead, script should use a DOM4-style event constructor.

If the Pointer Events spec is still using this old style of event construction, we should provide feedback to the working group that it should instead using DOM4-style event constructors.

> Source/WebCore/html/HTMLAttributeNames.in:228
> +onwebkitpointerdown
> +onwebkitpointerup
> +onwebkitpointermove
> +onwebkitpointerover
> +onwebkitpointerout
> +onwebkitpointercancel

Again, we should remove the vendor prefixes.  The spec is already in LC.

> Source/WebCore/page/EventHandler.cpp:4053
> +bool EventHandler::handlePointerEvents(PlatformPointerEventCollection & coll)

No space needed between PlatformPointerEventCollection and &.  Should this be a const reference?

Also, "coll" isn't a great name.  WebKit prefers that we use complete words in variable names.

> Source/WebCore/page/EventHandler.cpp:4056
> +    for (Vector<PlatformPointerEvent>::iterator iter = coll.events.begin(); iter != coll.events.end(); iter++)
> +    {

Please combine these lines.

Is it safe to iterate the vector from the parameter?  Should we make a local copy of the vector first and iterate that?  Given that we're dispatching events below, we might worry that JavaScript will destroy the parameter.  (I haven't analyzed the codepath to see if that's a real problem.)

> Source/WebCore/page/EventHandler.cpp:4061
> +        if (e.PointerProcessed()) {

No need for { } for one-line conditionals.

Also, please replace "e" with a complete word.

> Source/WebCore/page/EventHandler.cpp:4082
> +        default:

Generally we leave off the default case for this sort of switch so that the compiler will tell us if we forget a case.

> Source/WebCore/page/EventHandler.cpp:4089
> +bool EventHandler::dispatchPointerEvent(const AtomicString& eventType, const PlatformPointerEvent& e)

This function is too long.  Can we factor out various pieces into smaller functions?

> Source/WebCore/page/EventHandler.cpp:4093
> +    ASSERT(m_frame);
> +    if (!m_frame)
> +        return false;

It isn't correct to both ASSERT a condition and to handle its negation.  If the case can occur, then we should handle it and not have an ASSERT.  If the case cannot occur, then we should have the ASSERT and not handle the impossible case.

> Source/WebCore/page/EventHandler.cpp:4128
> +    Node *nodeUnderPointer = result;
> +    Node *lastNodeUnderPointer = m_lastNodeUnderMouse.get();

"Node *"  ->  "Node* "

> Source/WebCore/platform/PlatformPointerEvent.cpp:2
> + * Copyright (C) 2005, 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.

Why is there an Apple copyright in this new file?

> Source/WebCore/platform/PlatformPointerEvent.cpp:31
> +/*
> +
> +This implements Pointer Events as defined in Pointer Events specification submission to W3C.
> +
> + */

This comment is useless and should be removed.

> Source/WebCore/platform/PlatformPointerEvent.cpp:38
> +static long convertButtonToChordedValue(PointerButton);
> +static long convertMouseButtonToChordedValue(MouseButton);

Usually we implement static functions at the same time we declare them.

> Source/WebCore/platform/PlatformPointerEvent.cpp:48
> +//----------------------------------

This comment is useless and should be removed.

> Source/WebCore/platform/PlatformPointerEvent.cpp:109
> +PlatformEvent::Type PlatformPointerEvent::convertToPointerType(PlatformEvent::Type t)

"t" -> please use complete words in variable names.

> Source/WebCore/platform/PlatformPointerEvent.cpp:149
> +static long convertButtonToChordedValue(PointerButton btn)

btn -> please use complete words in variable names.

> Source/WebCore/platform/PlatformPointerEvent.cpp:198
> +//-----------------------------------
> +
> +//#ifdef __clang__
> +//#pragma clang diagnostic ignored "-Wglobal-constructors"
> +//#pragma clang diagnostic ignored "-Wexit-time-destructors"
> +//#endif
> +//PlatformPointerEventTracker PlatformPointerEvent::m_trackedPointerEvents;

Please remove commented out code.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.