[webkit-reviews] review denied: [Bug 84487] [chromium] Add touch link highlight animation layers. : [Attachment 159155] 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 159155: Patch
https://bugs.webkit.org/attachment.cgi?id=159155&action=review

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


Great!	Putting the hook at the end of layout() makes sense - at that point
we're done mucking with the render tree.  It will be too often in some cases -
we call WebView::layout() in some cases even if we aren't gonna composite right
then - but it should always happen before we do composite.

You have a (minor) memory leak and some dead code to clean up, but otherwise
this looks just about ready to land.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:668
> +	  
newChildren.append(m_linkHighlight->layer()->unwrap<LayerChromium>());

This doesn't seem right, you should be appending a WebLayer to newChildren not
a LayerChromium

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:788
> +

spurious newline

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:54
> +    virtual void clearOwningLayer() = 0;
> +    virtual WebKit::WebLayer* layer() = 0;

please be consistent with terminology - should this be clearOwningLayer() and
owningLayer(), or clearLayer() and layer()?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:124
> +    bool ownsLinkHighlight();

I can't find any callers of this - dead?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:125
> +    static void clearLinkHighlight(GraphicsLayerChromium* root);

I can't find any definition or callers of this - is it dead?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:172
> +    // Walks GraphicsLayer tree and clears any existing link highlight
pointer found.
> +    // Should only be called externally on the root layer.
> +    void clearLinkHighlightFromTree();

also appears to be dead

> Source/WebKit/chromium/src/LinkHighlight.cpp:45
> +#include "public/WebAnimationCurve.h"
> +#include "public/WebFloatAnimationCurve.h"

use <> for public/... includes

> Source/WebKit/chromium/src/LinkHighlight.cpp:50
> +#if USE(ACCELERATED_COMPOSITING)

this guard is unnecessary, ACCELERATED_COMPOSITING is on for all /chromium/
code and will be

> Source/WebKit/chromium/src/LinkHighlight.cpp:143
> +	   // This is the only place m_currentGraphicsLayer should ever be
dereferenced; it can be
> +	   // safely used for comparisons in other places.

this comment appears to be a lie - clearGraphicsLayerLinkHighlightPointer() is
definitely dereferencing m_currentGraphicsLayer.  I think this comment is just
stale since we get notified when the GraphicsLayerChromium is destroyed, so
it's safe to deref it until it goes null

> Source/WebKit/chromium/src/LinkHighlight.cpp:217
> +    WebAnimation* animation = WebAnimation::create(curve,
WebAnimation::TargetPropertyOpacity);
> +    m_contentLayer.setDrawsContent(true);
> +    m_contentLayer.addAnimation(animation);

addAnimation() doesn't take ownership of the passed-in animation so this is a
memory leak.  Store the animation in a local OwnPtr<> if you aren't going to
use it later

> Source/WebKit/chromium/src/LinkHighlight.cpp:219
> +    invalidate();

if you patch invalidate() as suggested (see below), remember to schedule an
animation explicitly here

> Source/WebKit/chromium/src/LinkHighlight.cpp:268
> +    // This will trigger a callback after the structure of the GraphicsLayer
tree
> +    // has settled, when it will be safe for us to update our own geometry.
> +    m_owningWebViewImpl->scheduleAnimation();

If we aren't hooking animate() any more you don't need this - whatever
generated the invalidation will trigger a new frame.

you do still need this to know whether to update geometry on the next layout
call, however

> Source/WebKit/chromium/src/LinkHighlight.cpp:269
> +    m_didRequestAnimationUpdate = true;

rename this flag to reflect what it means now since it doesn't have anything to
do with the animation calls

> Source/WebKit/chromium/src/LinkHighlight.h:72
> +    void clearOwningLayer();

OVERRIDE

> Source/WebKit/chromium/src/WebViewImpl.h:44
> +#if ENABLE(GESTURE_EVENTS)
> +#endif

looks like whatever you had here isn't need any more

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

it looks like you don't need this any more

>
LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1
-frame-scrolled-clipped.html:1
> +<!DOCTYPE HTML PUBLIC "-//IETF//DTD HTML//EN">

just <!DOCTYPE html> to make sure you're in standards mode here and in all
other tests

>
LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1
-frame-scrolled-clipped.html:5
> +<frameset rows="20, 120, *">

woah, frames? i think we should just worry about iframes - i can't imagine
enough people are using frameset for us to care about them explicitly (and I
think they aren't different for the code we care about)

>
LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1
-iframe-expected.txt:1
> + 

where are these completely empty expectation files coming from? are these files
actually resources and not tests?