Bug 92144 – [chromium] Add a bounding box member in WebGestureEvent

• Component: Platform
• Status: RESOLVED FIXED

[Terry Anderson]

The new |boundingBox| member will be used to store the touch region for a WebGestureEvent. This will be useful for gesture target fuzzing across multiple different gesture types (tap, long press, scroll, etc.)

[Terry Anderson]

Created attachment 154111 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Sadrul Habib Chowdhury]

Comment on attachment 154111 [details]
Patch

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

Can you also update the computation of PlatformGestureEvent::m_area at the same time? (looks like should be trivial?)

> Source/WebKit/chromium/public/WebInputEvent.h:42
> +struct WebRect;

This probably isn't necessary since you include the header?

> Source/WebKit/chromium/public/WebInputEvent.h:388
> +        , boundingBox(0, 0, 0, 0)

WebRect initializes itself correctly. This should not be necessary.

[Adam Barth]

Comment on attachment 154111 [details]
Patch

Please address sadrul's concerns before landing.

[Terry Anderson]

Created attachment 154117 [details]
Patch for landing

[Adam Barth]

Comment on attachment 154117 [details]
Patch for landing

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

> Source/WebKit/chromium/src/WebInputEventConversion.cpp:155
>          m_type = PlatformEvent::GestureTap;
> -        m_area = IntSize(e.deltaX * 2, e.deltaY * 2);
> +        IntRect bounds(e.boundingBox);
> +        m_area = IntSize(bounds.width(), bounds.height());

Don't you need to do this in a subsequent patch once your change to WebInputEvent has landed and you've updated Chromium to use this API?

[Adam Barth]

Comment on attachment 154117 [details]
Patch for landing

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

> Source/WebKit/chromium/public/WebInputEvent.h:373
>      // NOTE: |deltaX| and |deltaY| represents the amount to scroll for Scroll gesture events. For Pinch gesture events, |deltaX| represents the scaling/magnification factor. For a GestureTap event, |deltaX| and |deltaY| represent the horizontal and vertical radii of the touch region.

Do we need to update this comment?

[Terry Anderson]

(In reply to comment #6)
> (From update of attachment 154117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154117&action=review
> 
> > Source/WebKit/chromium/src/WebInputEventConversion.cpp:155
> >          m_type = PlatformEvent::GestureTap;
> > -        m_area = IntSize(e.deltaX * 2, e.deltaY * 2);
> > +        IntRect bounds(e.boundingBox);
> > +        m_area = IntSize(bounds.width(), bounds.height());
> 
> Don't you need to do this in a subsequent patch once your change to WebInputEvent has landed and you've updated Chromium to use this API?

Yes, you're right... I will make this change in another patch once the Chromium side has landed.

[Terry Anderson]

(In reply to comment #7)
> (From update of attachment 154117 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154117&action=review
> 
> > Source/WebKit/chromium/public/WebInputEvent.h:373
> >      // NOTE: |deltaX| and |deltaY| represents the amount to scroll for Scroll gesture events. For Pinch gesture events, |deltaX| represents the scaling/magnification factor. For a GestureTap event, |deltaX| and |deltaY| represent the horizontal and vertical radii of the touch region.
> 
> Do we need to update this comment?

Yes, we do. I'll update it in the next patch after this and the Chromium patch both land.

[Terry Anderson]

Created attachment 154121 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154121 [details]
Patch

Clearing flags on attachment: 154121

Committed r123520: <http://trac.webkit.org/changeset/123520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.