[webkit-reviews] review denied: [Bug 78509] [BlackBerry] Upstream touch handling related classes : [Attachment 127396] patch_1/3 v2 - TouchEventHandler

[bugzilla-daemon at webkit.org]

Rob Buis <rwlbuis at gmail.com> has denied Antonio Gomes <tonikitoo at webkit.org>'s
request for review:
Bug 78509: [BlackBerry] Upstream touch handling related classes
https://bugs.webkit.org/show_bug.cgi?id=78509

Attachment 127396: patch_1/3 v2 - TouchEventHandler
https://bugs.webkit.org/attachment.cgi?id=127396&action=review

------- Additional Comments from Rob Buis <rwlbuis at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=127396&action=review


Looks good, still soem issues.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:74
> +{

I think ASSERT(element); needs to be done here and not in the above two
methods, assuming those are used by elementExpectsMouseEvents only.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:96
> +    // Check for plugin

Add period :)

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:236
> +	       // Create  MouseReleased Event.

Double spacing.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:239
> +	       m_lastFatFingersResult.reset(); // reset the fat finger result
as its no longer valid when a user's finger is not on the screen.

reset -> Reset

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:242
> +	       if (spellLength) {

Can combine these two, also just 'unsigned' is enough and actually is what
spellCheck returns.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:244
> +		   unsigned int start = end - spellLength;

Can be just unsigned.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:282
> +    IntPoint newContentPos = IntPoint(rect.x() + rect.width(), rect.y() +
rect.height() / 2); // midway of right edge

No sentence...

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:297
> +	   // Send the mouse move event

No proper sentences.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:301
> +	   // Then send the MousePressed event

Lacks period.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:312
> +    bool isArea = elementUnderFatFinger->hasTagName(HTMLNames::areaTag);

Can be moved below the if.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:316
> +    // renderer.

Is this the right position for this comment?

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.cpp:374
> +    // FIXME: We can get more precise on the MAP case by calculating the
rect with HTMLAreaElement::computeRect().

MAP case?

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:2
> + * Copyright (C) 2010, 2011 Research In Motion Limited. All rights reserved.


Once you start changing this, 2012 can be added.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:30
> +class IntPoint;

Maybe not needed since we include it?

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:31
> +class Element;

Does not seem used.

> Source/WebKit/blackberry/WebKitSupport/TouchEventHandler.h:44
> +    bool handleTouchPoint(BlackBerry::Platform::TouchPoint&);

BlackBerry:: prefix can be dropped in this header.