[webkit-reviews] review denied: [Bug 78509] [BlackBerry] Upstream touch handling related classes : [Attachment 127397] patch_2/3 v2 - FatFingers

[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 127397: patch_2/3 v2 - FatFingers
https://bugs.webkit.org/attachment.cgi?id=127397&action=review

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


Looks good, still some issues.

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


2012 should be added.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:45
> +#include "WebPage_p.h"

Is above minimal?

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:56
> +using WTF::Vector;

WTF does the above itself, can be removed.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:124
> +    Element* element = toElement(node);

Not sure if temp var is needed here.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:185
> +	   // If we are looking for a Clickable Element and we found one, we
can quite early.

quite -> quit.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:200
> +    // force blit to make the fat fingers rects show up

Not a proper sentence.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.cpp:330
> +	   bool isElement = curNode->isElementNode();

isElement maybe not needed.

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


2012 should be added.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:23
> +#include "RenderLayer.h"

Is this one needed? Also it is in the cpp as well.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:41
> +namespace BlackBerry {

You can remove BlackBerry:: prefix throughout this header.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:100
> +    const FatFingersResult findBestPoint();

Can this be const?

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:109
> +#endif

DEBUG_FAT_FINGERS could be moved into .cpp if these were just local static vars
in the .cpp.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:112
> +

No need for empty line.

> Source/WebKit/blackberry/WebKitSupport/FatFingers.h:135
> +    void setSuccessfulFatFingersResult(FatFingersResult&, WebCore::Node*,
const WebCore::IntPoint&);

Can this be const?