[webkit-reviews] review denied: [Bug 84487] [chromium] Add touch link highlight animation layers. : [Attachment 142729] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 84487: [chromium] Add touch link highlight animation layers.
https://bugs.webkit.org/show_bug.cgi?id=84487

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

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


I'm not sure I understand all of the lifetimes here yet.  If it helps, one
property of WebCore::Timer<>s is that you can delete one to cancel it.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:126
> +    // If this layer already has active animations, the host needs to be
notified.
> +    if (host && m_layerAnimationController->hasActiveAnimation())
> +	   host->didAddAnimation();

This does not feel like it's logically part of the same patch - is this fixing
an underlying bug in the animation system?

I would prefer to keep the link highlight logic in a different patch from
general fixes.	This change also needs test coverage on its own (not link
highlight tests).

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:670
> +    if (m_layerAnimationDelegate)
> +	   m_layerAnimationDelegate->notifyAnimationFinished(wallClockTime);

same here as below.

when would you have an animation active but no animation delegate? that seems
very odd

> Source/WebKit/chromium/src/WebViewImpl.cpp:503
> +
> +

spurious whitespace here

> Source/WebKit/chromium/src/WebViewImpl.cpp:1058
> +void WebViewImpl::enableTouchHighlight(IntPoint touchEventLocation, Path*
outputPath)

this is a bit of an odd name. "enable" makes me think this is turning some
setting on for the future, but this appears to be doing a good bit more

> Source/WebKit/chromium/src/WebViewImpl.cpp:1063
> +    // FIXME: make the padding size coume from the touch event generator.

typo "coume"

> Source/WebKit/chromium/src/WebViewImpl.cpp:1075
> +    // FIXME: At present we need to generate the path before checking for a
valid graphics layer
> +    // in order to create a useful unit test. It would be nice instead to
not bother generating
> +    // the path if no backing is found.

This is quite awkward. I think it's a sign that this function is doing too many
distinct things and should be broken down into separate testable pieces that
each do a distinct, logical bit of work.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1092
> +    do {
> +	   backing = renderLayer->backing();
> +	   renderLayer = renderLayer->parent();
> +    } while (!backing && renderLayer);

this is very awkward. are you attempting to find this object's nearest
enclosing composited layer? if so, loop up through parent pointers checking for
isComposited() and then grab the backing from that.

what if you hit the root of the tree without hitting a composited layer (i.e.
compositing is not on for this page, or the root is composited but RenderLayers
are, or the node is in a subframe)?  have you tested these cases?

> Source/WebKit/chromium/src/WebViewImpl.h:59
> +#if ENABLE(GESTURE_EVENTS)
> +#include "TouchLinkHighlightTimer.h"
> +#endif

You should only need a forward declaration of this type, not the actual
definition of the class

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:39
> +#include <webkit/support/webkit_support.h>

This is bad - we really should never be doing this in unit tests.  It probably
means this testcase won't be able to run in the component build.

Why do you need this?