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

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


I think the design (specifically ownership and layering) needs a bit of
refinement here.  One question - can there ever be more than one active link
highlight?

> Source/WebCore/page/FrameView.cpp:2618
> +    // Notify containing composited layer that we have scrolled.

this seems pretty invasive and potentially not necessary.  If I understand, the
goal here is that if a highlighted link somewhere inside a composited layer
moves we want to update the link highlight geometry to stay on "top" of the
link, right?  Any such movement should trigger a paint invalidation of the
composited layer, so can we just figure out the position of any highlighted
link inside a composited layer whenever we paint it?  I.e. make the compositor
pull the position out of the layer instead of having FrameView push a
notification out, since we're getting an invalidation anyway.

> Source/WebCore/platform/graphics/GraphicsLayer.h:405
> +    virtual void updateAfterLayout() { }
> +    virtual void containedLayersHaveScrolled() { }

GraphicsLayer shouldn't really know anything about layout - layout is a
WebCore/rendering/ concept and GraphicsLayer is a WebCore/platform/ concept.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:49
> +class Node;

GraphicsLayerChromium cannot have any knowledge of Node - GLC is in
WebCore/platform and Node is in WebCore/dom/

> Source/WebCore/platform/graphics/chromium/LinkHighlight.cpp:36
> +#include "Frame.h"
> +#include "FrameView.h"
>  #include "GraphicsLayerChromium.h"
> +#include "LayerChromium.h"
> +#include "Node.h"
>  #include "PlatformContextSkia.h"
> +#include "RenderObject.h"

these are all layering violations - code in WebCore/platform/ should not know
anything about concepts anywhere else in WebCore (like /dom/, /rendering/, or
/page/) and compositor code should know about either GraphicsLayerChromium or
about compositor internals like LayerChromium, but not both.  If this class
wants to talk to GraphicsLayerChromium it should be using WebLayer types to
talk to the compositor