[webkit-reviews] review denied: [Bug 97355] [chromium] Modify gesture highlight behaviour. Cancel on GestureLongPress and animate on GestureTapCancel. : [Attachment 165178] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 97355: [chromium] Modify gesture highlight behaviour. Cancel on
GestureLongPress and animate on GestureTapCancel.
https://bugs.webkit.org/show_bug.cgi?id=97355

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

------- Additional Comments from Adrienne Walker <enne at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=165178&action=review


> Source/WebKit/chromium/src/LinkHighlight.cpp:-79
> -    // We don't want to show the highlight until startAnimation is called,
even though the highlight
> -    // layer may be added to the tree immediately.

Why is this comment no longer true? What has changed?

> Source/WebKit/chromium/src/WebViewImpl.cpp:747
> +	   fprintf(stderr, "wjm: GestureLongPress event.\n");

...

> Source/WebKit/chromium/src/WebViewImpl.cpp:779
> +	   if (m_linkHighlight && !m_linkHighlight->isAnimating())
> +	       m_linkHighlight->startHighlightAnimation();

This is just a small nit, but it almost seems like startHighlightAnimation()
should internally make it impossible to call multiple times.  For example, if
it were startHighlightAnimationIfNeeded, then callers don't have to know about
isAnimating or not and can't screw it up.

> Source/WebKit/chromium/src/WebViewImpl.cpp:787
> +	   if (m_linkHighlight && !m_linkHighlight->isAnimating())
> +	       m_linkHighlight->startHighlightAnimation();

Are you maybe missing some "break" statements here in this switch statement?
Surely GesturePinchBegin should not start and then immediately clear the link
highlight?