[webkit-reviews] review denied: [Bug 72909] [Chromium] Add method to WebViewImpl to extract zoom/scroll params for gesture events on touch devices : [Attachment 116153] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Varun Jain
<varunjain at chromium.org>'s request for review:
Bug 72909: [Chromium] Add method to WebViewImpl to extract zoom/scroll params
for gesture events on touch devices
https://bugs.webkit.org/show_bug.cgi?id=72909

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=116153&action=review


> Source/WebCore/ChangeLog:8
> +	   No new tests. (OOPS!)

this is a problem. there's a reasonable amount of non-trivial logic in here and
it needs to be covered by some tests

> Source/WebCore/platform/PlatformGestureEvent.h:59
> +	   , m_scale(0.0f)

we just say '0' for constants like this in WebKit. See
http://www.webkit.org/coding/coding-style.html

> Source/WebCore/platform/PlatformGestureEvent.h:85
> +    void setDeltaX(float deltaX) { m_deltaX = deltaX; }
> +    void setDeltaY(float deltaY) { m_deltaY = deltaY; }
> +    void setScale(float scale) { m_scale = scale; }
> +

this is somewhat weird - most of these PlatformXEvent types are wrappers around
native events and only provide getters. where do we need to manipulate a
PlatformGestureEvent directly?

> Source/WebKit/chromium/src/WebViewImpl.cpp:193
> +static const float kMinDoubleTapZoomInRatio = 1.33f;
> +static const float kMinScaleDifference = 0.01f;

trailing f's are not very useful here

where are these numbers coming from? can you add some comments indicating how
these values were arrived at and what the process would be to change them?

> Source/WebKit/chromium/src/WebViewImpl.cpp:792
> +    for (unsigned int  i = 0; i < gestureEvents->size(); i++) {

double space here between 'int' and 'i'

we don't typically use 'unsigned int' as a type for iteration. 'unsigned' or
size_t please

> Source/WebKit/chromium/src/WebViewImpl.cpp:794
> +	       float scale = 1.f;

kill the .1f

> Source/WebKit/chromium/src/WebViewImpl.cpp:816
> +    IntPoint point =
mainFrameImpl()->frameView()->windowToContents(WebCore::IntPoint(rect.x,
rect.y));

the WebCore:: prefix isn't needed here

> Source/WebKit/chromium/src/WebViewImpl.cpp:828
> +	   IntRect rect = node->Node::getRect(); // workaround
ContainerNode::getRect transformation bug

what bug is this talking about? can you add a link to it and a FIXME?

> Source/WebKit/chromium/src/WebViewImpl.cpp:839
> +	 return 0.0f;

kill the .0f

> Source/WebKit/chromium/src/WebViewImpl.cpp:843
> +    minScale = std::max(minScale, (float) m_size.width / (contentSize.width
/ pageScaleFactor()));

drop the std::

> Source/WebKit/chromium/src/WebViewImpl.cpp:848
> +void WebViewImpl::getScaleAndScrollForHitRect(const WebRect& hitRect, bool
forDoubleTap, float* scale, WebPoint* scroll)

is it valid for 'scale' to be NULL? if not can you make this a reference
instead of a pointer?

same for scroll

> Source/WebKit/chromium/src/WebViewImpl.cpp:865
> +	   const float minZoomInRatio = forDoubleTap ? kMinDoubleTapZoomInRatio
: 1.0f;

.0f :(

> Source/WebKit/chromium/src/WebViewImpl.cpp:875
> +	   *scale *= static_cast<float>(m_size.width) / rect.width;

are you sure this is the correct rounding?

> Source/WebKit/chromium/src/WebViewImpl.cpp:876
> +	   *scale = std::min(std::max(*scale, minScale), maxScale);

drop std::

> Source/WebKit/chromium/src/WebViewImpl.cpp:910
> +	   rect.y = std::max((float) rect.y, hitRect.y + kTouchPointPadding -
screenHeight);

drop std::

> Source/WebKit/chromium/src/WebViewImpl.cpp:917
> +	   rect.x = std::max((float) rect.x, hitRect.x + kTouchPointPadding -
screenWidth);

drop the std::. there's a using namespace std; at the top of this file

> Source/WebKit/chromium/src/WebViewImpl.h:478
> +    // Returns the bounding box of the block type node touched by the input
> +    // point with the padding.
> +    WebRect getBlockBounds(const WebRect&, bool ignoreClipping);

we don't use the 'get' prefix very much in WebKit. a more idomatic name for
this might be 'calculateBlockBounds', or perhaps 'compute' to be consistent
with the following function