[webkit-reviews] review denied: [Bug 85084] [chromium] Create LinkHighlightLayerChromium class to provide link-highlight preview animations for GraphicsLayerChromium. : [Attachment 139239] 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 85084: [chromium] Create LinkHighlightLayerChromium class to provide
link-highlight preview animations for GraphicsLayerChromium.
https://bugs.webkit.org/show_bug.cgi?id=85084

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

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


I like this in general.  It seems a little bit wasteful of texture memory to
use a content layer for a link highlight, but on the other hand we won't have
many of them at once, they're small, and it's so much simpler to just draw the
path into a GraphicsContext then to try to draw it behind and not use texture
memory.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.cpp:76
> +    OwnPtr<CCActiveAnimation>
animation(CCActiveAnimation::create(curve.release(), ++id, 0,
CCActiveAnimation::Opacity));

How do the animation ids here work with other ids that are potentially
conflicting in the animation system? If somebody removed an animation in css by
name that mapped to a conflicting id, it looks like both would get removed by
CCLayerAnimationController::removeAnimation.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.cpp:114
> +    if (m_parent)
> +	  
static_cast<GraphicsLayerChromium*>(m_parent)->didFinishLinkHighlightLayer();

I think you should just use GraphicsLayerChromium as the type here all the way
through and not static cast.  This is a LinkHilighterLayerChromium, so it's not
like other GraphicsLayer types will work here.

> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.h:38
> +class LinkHighlightLayerChromium : public
RefCounted<LinkHighlightLayerChromium>, public ContentLayerDelegate, public
CCLayerAnimationDelegate {

It's really confusing to suffix this with "LayerChromium" when it's really a
layer delegate and not actually a layer itself.  Even LinkHighlighter on its
own would be clearer to me.