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

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Varun Jain]

[Chromium] Add method to WebViewImpl to extract zoom/scroll params for gesture events on touch devices

[Varun Jain]

Created attachment 116127 [details]
Patch

[Varun Jain]

Created attachment 116128 [details]
Patch

[WebKit Review Bot]

Comment on attachment 116128 [details]
Patch

Attachment 116128 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10475352

[Varun Jain]

Created attachment 116153 [details]
Patch

[James Robinson]

Comment on attachment 116153 [details]
Patch

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

[Varun Jain]

Created attachment 125365 [details]
Patch

[Varun Jain]

(In reply to comment #6)
> Created an attachment (id=125365) [details]
> Patch

Please hold off review... still working on writing tests

[Alexandre Elias]

Andrei, John, could you take a look at this latest patch?  This upstreams code that you originally worked on.

[Varun Jain]

Created attachment 125605 [details]
Patch

[Andrei Popescu]

Comment on attachment 125605 [details]
Patch

I only have some style nits:

View in context: https://bugs.webkit.org/attachment.cgi?id=125605&action=review

> Source/WebKit/chromium/ChangeLog:13
> +        (WebKit::WebViewImpl::WidenRectWithinPageBounds):

any reason why this starts with a capital letter?

> Source/WebKit/chromium/src/WebViewImpl.cpp:818
> +            PlatformGestureEvent& oldPge = gestureEvents->at(i);

What's the "old" prefix meant to say?

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

Here and in some other places: WebKit frowns on bool args. Can we use an enum?

https://lists.webkit.org/pipermail/webkit-dev/2010-December/015192.html

> Source/WebKit/chromium/src/WebViewImpl.cpp:934
> +        const float maxScale = deviceScaleFactor() * 1 /* font scale factor */;

Do we need the trailing comment?

> Source/WebKit/chromium/src/WebViewImpl.cpp:946
> +            static_cast<int>(minimumMargin * rect.width / m_size.width));

Weird indent...should be 8 spaces I think?

[Varun Jain]

Created attachment 125650 [details]
Patch

[Varun Jain]

Created attachment 125654 [details]
Patch

[Andrei Popescu]

LGTM

(In reply to comment #12)
> Created an attachment (id=125654) [details]
> Patch

[Varun Jain]

Created attachment 125659 [details]
Patch

[James Robinson]

Comment on attachment 125659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review

I thought that the WebKit gesture recognizer in chromium was deprecated and due to be deleted soon - I'm very surprised to see new code using it. What's the story?

> Source/WebCore/platform/PlatformGestureEvent.h:47
> +    PlatformGestureEvent(Type type, const IntPoint& position, const IntPoint& globalPosition, double timestamp, float deltaX, float deltaY, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, float scale = 0.0f)

we don't use qualifiers like ".0f" in WebKit. See "Floating point literals" in http://www.webkit.org/coding/coding-style.html

what does it mean for a gesture event to have a scale? is this different from the page scale when the event happens?

> Source/WebKit/chromium/src/WebViewImpl.cpp:202
> +static const float kDoubleTapZoomContentDefaultMargin = 5.0;
> +static const float kDoubleTapZoomContentMinimumMargin = 2.0;

don't append the .0s here

[James Robinson]

Comment on attachment 125659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review

> Source/WebKit/chromium/src/WebViewImpl.h:504
> +    enum BoostedTextType {
> +        NormalText,
> +        BoostedText,
> +    };

this looks like an unrelated change

[Varun Jain]

Created attachment 125679 [details]
Patch

[Varun Jain]

(In reply to comment #15)
> (From update of attachment 125659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review
> 
> I thought that the WebKit gesture recognizer in chromium was deprecated and due to be deleted soon - I'm very surprised to see new code using it. What's the story?

You are correct.. I should be putting this under ENABLE(GESTURE_EVENTS) rather than GESTURE_RECOGNIZER. Fixed.


> 
> > Source/WebCore/platform/PlatformGestureEvent.h:47
> > +    PlatformGestureEvent(Type type, const IntPoint& position, const IntPoint& globalPosition, double timestamp, float deltaX, float deltaY, bool shiftKey, bool ctrlKey, bool altKey, bool metaKey, float scale = 0.0f)
> 
> we don't use qualifiers like ".0f" in WebKit. See "Floating point literals" in http://www.webkit.org/coding/coding-style.html

Done.

> 
> what does it mean for a gesture event to have a scale? is this different from the page scale when the event happens?

For certain gestures such as Double tap or pinch to zoom, one of the attributes is scale.

> 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:202
> > +static const float kDoubleTapZoomContentDefaultMargin = 5.0;
> > +static const float kDoubleTapZoomContentMinimumMargin = 2.0;
> 
> don't append the .0s here

Done.

[Varun Jain]

(In reply to comment #16)
> (From update of attachment 125659 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=125659&action=review
> 
> > Source/WebKit/chromium/src/WebViewImpl.h:504
> > +    enum BoostedTextType {
> > +        NormalText,
> > +        BoostedText,
> > +    };
> 
> this looks like an unrelated change

It is used in the computation of block bounds to zoom to.

[Varun Jain]

Created attachment 125707 [details]
Patch

[Robert Kroeger]

Comment on attachment 125707 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=125707&action=review

> Source/WebCore/platform/PlatformGestureEvent.h:82
> +    float m_scale;

I am concerned about this. startPageScaleAnimation (the ultimate target of what you're constructing) needs a point (existing field) and a scale (deltaX by convention)

> Source/WebKit/chromium/src/WebViewImpl.cpp:632
> +    if (event.type == WebInputEvent::GestureDoubleTap) {

I think this could be better. In particular, the end goal is for

if this WGE is a doubletap,
  compute the arguments needed for startPageScaleAnimation
  forward these arguments to the impl thread
else
  construct a PlatformGestureEvent and sent it to the EventHandler::handleGestureEvent

Since https://bugs.webkit.org/show_bug.cgi?id=77872 is still underway, this code for the time being should being construct a PlatformGestureEvent  as would be consumed by ScrollAnimatorNone::zoom

> Source/WebKit/chromium/src/WebViewImpl.cpp:906
> +void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType zoomType, float& scale, WebPoint& scroll)

is there any way to make this whole function run on the impl thread? it would be a huge gain I think.

> Source/WebKit/chromium/src/WebViewImpl.cpp:994
> +bool WebViewImpl::dispatchGestureEvent(PlatformGestureEvent& pge)

because dispatchGestureEvent implies something specific in EventHandler, I'd prefer a different name.

> Source/WebKit/chromium/src/WebViewImpl.h:553
> +    // Returns the bounding box of the block type node touched by the input

Aren't comments like these are atypical in WebCore? nit: need a period at the end.

[Alexandre Elias]

I also have concerns about the event handling stuff, I think we should avoid adding fields of any kind to the event object.  I wrote more detailed comments at https://bugs.webkit.org/show_bug.cgi?id=77872 .  I think the event stuff could be removed from this CL and dealt with separately.

Another minor concern: the "+ 12" margin in the unit test seems too specific and brittle.  How about for things involving the margin, use EXPECT_NEAR(pageWidth / divWidth, scale, 0.1)?  That would be more robust to minor changes in the margin logic.

[Alexandre Elias]

(In reply to comment #21)
> > Source/WebKit/chromium/src/WebViewImpl.cpp:906
> > +void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType zoomType, float& scale, WebPoint& scroll)
> 
> is there any way to make this whole function run on the impl thread? it would be a huge gain I think.

Unfortunately, no.  The impl thread only knows about the layer tree, whereas this needs detailed information from the render tree.  We could precompute the rects and push it across the thread boundary, but this will be nontrivial to get right (Android browser did it and they spent a long time struggling with corner cases).

[Varun Jain]

(In reply to comment #23)
> (In reply to comment #21)
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:906
> > > +void WebViewImpl::computeScaleAndScrollForHitRect(const WebRect& hitRect, AutoZoomType zoomType, float& scale, WebPoint& scroll)
> > 
> > is there any way to make this whole function run on the impl thread? it would be a huge gain I think.
> 
> Unfortunately, no.  The impl thread only knows about the layer tree, whereas this needs detailed information from the render tree.  We could precompute the rects and push it across the thread boundary, but this will be nontrivial to get right (Android browser did it and they spent a long time struggling with corner cases).

Yes.. moving this to the impl thread would be very hard. We might be able to get everything but computeBlockBounds on to the impl thread but I think that would be useless because computeBlockBounds is the only expensive operation here.

[Varun Jain]

Created attachment 125731 [details]
Patch

[Varun Jain]

Hi All.... I have removed the gesture events related code from this CL. I think this code can be added in https://bugs.webkit.org/show_bug.cgi?id=77872 which actually adds support for the double tap gesture.
PTAL.

[Varun Jain]

ping

[Adam Barth]

> ping

https://lists.webkit.org/pipermail/webkit-dev/2012-January/019023.html

[Alexandre Elias]

LGTM except the 12.0 number in the unit test I mentioned earlier.

[Varun Jain]

(In reply to comment #29)
> LGTM except the 12.0 number in the unit test I mentioned earlier.

Hi Alexandre... the margins are all very well defined. The logic of including the margins is also deterministic. So I am not sure why the test would be brittle. Wouldnt it be better to have these parameters tested out exactly?

[Alexandre Elias]

It's not a huge deal.  But personally, I prefer to avoid overspecific tests, particularly ones with nonobvious, semi-arbitrary parameters.  If someone made a minor change that changed the margin to e.g. 14.0, that's not necessarily incorrect and it's unnecessary for the test to go red.

[Varun Jain]

(In reply to comment #31)
> It's not a huge deal.  But personally, I prefer to avoid overspecific tests, particularly ones with nonobvious, semi-arbitrary parameters.  If someone made a minor change that changed the margin to e.g. 14.0, that's not necessarily incorrect and it's unnecessary for the test to go red.

I think it is fair to require that legit changes to parameters also update the test. The test is there to protect against unrelated/unintentional changes. For example, it would be bad if the block bounds to zoom to are off by 5 pixels due to some unrelated CL.
I guess this is a matter of personal opinion.. I'll remove it if other also have hard feelings against :)

[Varun Jain]

(In reply to comment #32)
> (In reply to comment #31)
> > It's not a huge deal.  But personally, I prefer to avoid overspecific tests, particularly ones with nonobvious, semi-arbitrary parameters.  If someone made a minor change that changed the margin to e.g. 14.0, that's not necessarily incorrect and it's unnecessary for the test to go red.
> 
> I think it is fair to require that legit changes to parameters also update the test. The test is there to protect against unrelated/unintentional changes. For example, it would be bad if the block bounds to zoom to are off by 5 pixels due to some unrelated CL.
> I guess this is a matter of personal opinion.. I'll remove it if other also have hard feelings against :)

@jamesr: I think everyone is ok with the current patch.. please review.

[Robert Kroeger]

This all looks good to me. 

varunjain@ could you add a bug for the changes needed to ::gestureEvent so that detail doesn't get overlooked?

[Alexandre Elias]

LGTM, thanks.

[James Robinson]

Comment on attachment 125731 [details]
Patch

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

[Varun Jain]

Created attachment 126521 [details]
Patch

[Varun Jain]

(In reply to comment #36)
> (From update of attachment 125731 [details])
> 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.

added more tests.

> 
> > 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)

Done.

> 
> > 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

extracted it out to another function. But I want to keep it private, so I cannot test this separately. But it should not matter because the new tests I added cover all of this code.

> 
> > 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 think the goal here is that if the hit rect contains a text node that has unscaled fonts then we want to zoom to device scale factor atleast so that we are guaranteed to get legible font size.
If the rect already contains boosted fonts then we simply zoom enough to bring the div width equal to page width.

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

Added tests to cover specifically this part.


> 
> > 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

Done.

> 
> > 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.

Added test.


> 
> > 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

Removed the margin and using EXPECT_NEAR as aelias suggested :)

[Varun Jain]

(In reply to comment #34)
> This all looks good to me. 
> 
> varunjain@ could you add a bug for the changes needed to ::gestureEvent so that detail doesn't get overlooked?

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

[WebKit Review Bot]

Comment on attachment 126521 [details]
Patch

Attachment 126521 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11447746

[Varun Jain]

Created attachment 126531 [details]
Patch

[Varun Jain]

(In reply to comment #40)
> (From update of attachment 126521 [details])
> Attachment 126521 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/11447746

new patch fixes build failure

[James Robinson]

I'm not sure how we plan to proceed with the font boosting code yet and would like to consult with some people first about what our plan is.  Without some idea of what the big picture is I don't feel comfortable approving bits and pieces of it.

If you think there's a useful subset of this patch without that feel free to post it and I'd be happy to review.  Otherwise this will have to wait.

[Adam Barth]

We probably want to handle the font boosting related code separately.  It's likely going to need some reworking before we land it.

[James Robinson]

Comment on attachment 126531 [details]
Patch

Clearing review? flag from this

[Varun Jain]

Created attachment 127023 [details]
Patch

[Varun Jain]

(In reply to comment #43)
> I'm not sure how we plan to proceed with the font boosting code yet and would like to consult with some people first about what our plan is.  Without some idea of what the big picture is I don't feel comfortable approving bits and pieces of it.
> 
> If you think there's a useful subset of this patch without that feel free to post it and I'd be happy to review.  Otherwise this will have to wait.

Hi James..
I have removed the font scaling related bits. PTAL.

[Varun Jain]

(In reply to comment #47)
> (In reply to comment #43)
> > I'm not sure how we plan to proceed with the font boosting code yet and would like to consult with some people first about what our plan is.  Without some idea of what the big picture is I don't feel comfortable approving bits and pieces of it.
> > 
> > If you think there's a useful subset of this patch without that feel free to post it and I'd be happy to review.  Otherwise this will have to wait.
> 
> Hi James..
> I have removed the font scaling related bits. PTAL.

@jamesr: friendly reminder.. PTAL.

[James Robinson]

Comment on attachment 127023 [details]
Patch

OK

[WebKit Review Bot]

Comment on attachment 127023 [details]
Patch

Clearing flags on attachment: 127023

Committed r108027: <http://trac.webkit.org/changeset/108027>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.