[webkit-reviews] review denied: [Bug 72909] [Chromium] Add method to WebViewImpl to extract zoom/scroll params for gesture events on touch devices : [Attachment 125731] 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 125731: Patch
https://bugs.webkit.org/attachment.cgi?id=125731&action=review

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


I'm glad you have tests.  However, there's a lot more logic here than tests and
not all of it is very clear.

> Source/WebKit/chromium/src/WebViewImpl.cpp:202
> +static const int kTouchPointPadding = 32;
> +static const float kMinScaleDifference = 0.01;
> +static const float kDoubleTapZoomContentDefaultMargin = 5;
> +static const float kDoubleTapZoomContentMinimumMargin = 2;

this is incorrect naming style for WebKit - in WebKit, constants are named just
like normal variables (no "k" prefix)

> Source/WebKit/chromium/src/WebViewImpl.cpp:835
> +    // Find the most boosted text node intersected by the hit test (if any).


this looks like a great candidate for being moved into its own function and
tested separately

> Source/WebKit/chromium/src/WebViewImpl.cpp:943
> +	   // Fit block to screen, respecting limits.
> +	   if (textType == NormalText)
> +	       scale = maxScale;
> +	   else {
> +	       scale *= static_cast<float>(m_size.width) / rect.width;
> +	       scale = min(scale, maxScale);
> +	   }

I'm not sure why this logic makes sense - wouldn't you want to fix the block
the same way no matter what the text size is?

I'm also pretty sure this bit of logic is not covered by your tests.

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

we never use c-style casts in webkit. use static_cast<> if you need to, or
max<float>() to avoid it

> Source/WebKit/chromium/tests/WebFrameTest.cpp:155
> +TEST_F(WebFrameTest, DivAutoZoomParamsTest)

there's a lot more logic in WebViewImpl than there are test cases here. I can't
find any tests for the logic to find the text with the largest font size, for
example.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:184
> +    EXPECT_FLOAT_EQ(pageWidth / (divWidth + 12.0 /* margin */), scale);

where does 12 come from? that's a pretty bizarre number to have in the middle
of this test and I can't find it anywhere else