[Webkit-unassigned] [Bug 72909] [Chromium] Add method to WebViewImpl to extract zoom/scroll params for gesture events on touch devices

[bugzilla-daemon at webkit.org]

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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #116153|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #5 from James Robinson <jamesr at chromium.org>  2011-11-27 11:03:41 PST ---
(From update of attachment 116153)
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

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