[webkit-reviews] review denied: [Bug 105463] [Feature] Implement pointer events : [Attachment 189248] Updated Pointer Events implementation to live entirely in WebCore (as opposed to creeping into platform layer)

Tue Feb 19 23:05:51 PST 2013

Adam Barth <abarth at webkit.org> has denied  review:
Bug 105463: [Feature] Implement pointer events
https://bugs.webkit.org/show_bug.cgi?id=105463

Attachment 189248: Updated Pointer Events implementation to live entirely in
WebCore (as opposed to creeping into platform layer)
https://bugs.webkit.org/attachment.cgi?id=189248&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
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.