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

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


Please add your manual tests to the patch - you can post them as patches to
ManualTests/ for now while you work on getting layout tests up and running - so
we can see what does and doesn't have coverage.  This patch has a potentially
serious (as in crashy) ordering issue, see the big block comment, but otherwise
seems to be converging.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:437
> +    if (!m_contentsLayer.isNull()) {
>	   m_contentsLayer.invalidate();
> +	   if (m_linkHighlight)
> +	       m_linkHighlight->invalidate();
> +    }

I don't think you need this. m_contentsLayer is used when a layer has
video/WebGL/canvas/plugin/etc content - not normal HTML contents. Links will
always live in m_layer, so you should only have to worry about those
invalidations.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:54
> +};

please add a virtual d'tor - since this is a non-owning client interface, it
should go in the protected section.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:122
> +    virtual void setLinkHighlight(LinkHighlightClient*);
> +    // Next function for testing purposes.
> +    virtual LinkHighlightClient* linkHighlight() { return m_linkHighlight; }

> +    // Walks GraphicsLayer tree and clears any existing link highlight
pointer found.
> +    // Should only be called externally on the root layer.
> +    virtual void clearLinkHighlightFromTree();

Why are all of these virtual? They aren't overrides from GraphicsLayer and
nobody should be subclassing GraphicsLayerChromium.

> Source/WebKit/chromium/src/LinkHighlight.cpp:124
> +    m_compositingLayer = renderLayer;

This pointer is unsafe to dereference outside of this callstack (just like
m_currentGraphicsLayer) - I think it'd be better to pass this around on the
stack and not store it in a member variable.  I.e. this function could put this
pointer into an out parameter and then that could be passed to
computeHighlightLayerPathAndPosition()

> Source/WebKit/chromium/src/LinkHighlight.cpp:163
> +    // This is a simplified, but basically correct, transformation of the
target location, converted

This looks quite a bit nicer - thanks for cleaning this up!

> Source/WebKit/chromium/src/LinkHighlight.cpp:166
> +    // FIXME: We also need to transform the target's size in case of
scaling. This can be done by also transforming

The link might also be rotated or otherwise transformed in some weird way such
that the top-left pre-transform is not still the top-left post-transform. I
think for now adding a testcase and having a FIXME is good enough, to fix this
in general we should probably get a bounding quad and map that through all the
transforms.

> Source/WebKit/chromium/src/LinkHighlight.cpp:183
> +    // Before we paint, check for movement of the target node.
> +    if (m_node && m_node->renderer()) {

The comment doesn't appear to match the code.  Is the comment meant to be a
FIXME describing something we should do, or talking about something this code
is doing?

WebKit style is to prefer early returns for something like this

> Source/WebKit/chromium/src/LinkHighlight.cpp:230
> +void LinkHighlight::invalidate()
> +{
> +    computeEnclosingGraphicsLayer();
> +    if (computeHighlightLayerPathAndPosition()) {

The timing here is still wrong.  This is too early (just as paint time was too
late), the GraphicsLayer tree might not be valid at this point.  Let me back up
and explain the big picture a bit.

The normal flow of things is:

*) On arbitrary callstacks (timers, JS, events, etc) WebCore sends invalidate()
calls out through its GraphicsLayer whenever it knows that something will need
to be repainted. It makes these calls all over the place, including in places
where the full RenderLayer/GraphicsLayer trees are not in any particular state
(i.e. during creation and destruction of RenderObjects).  Logically this call
means "The next time you paint, this area will need to be repainted."  It's not
making any statement about the state of the world at the time of the invalidate
call.

*) When the compositor decides it's time to make a frame, it does the following
things on the same callstack:

  1.) Ticks all JS animations (requestAnimationFrame callbacks) and WebCore
imperative animations (CSS animations, scroll animator ticks, etc).  After
these are done, WebCore regenerates (if needed) the GraphicsLayer tree.
  2.) Ticks all compositor-driven animations.
  3.) Calculate a set of layers that will need a repaint because they are new,
they were invalidated since the last paint, they have newly exposed content, or
their contents were evicted since last frame.
  4.) Iterates through the list of layers that need an update and calls paint()
on each
  5.) Syncs the layers and textures over to the compositor thread's data
structures so it can draw

With that in mind, let's consider the case of link highlights.

Detecting damage and attempting to reparent/regenerate the link highlight in
the paint call is no good since it's too late - the composited layer tree and
the set of layers that will get a paint() call already, so the link highlight
layer won't be updated.  This may be why you saw flickering.  We have to run
earlier.

Attempting to reparent/regenerate the link highlight during the invalidate()
call is too early.  At this point, the RenderLayer and GraphicsLayer trees in
WebCore are not necessarily in any particular state - in particular WebCore
regenerates the GraphicsLayer tree lazily (see
RenderLayerCompositor::updateCompositingLayers and its callsites to see how
this happens).	Additionally, we can get lots and lots of invalidate() calls
during a single frame and attempting to map+repaint the link highlight on each
of these is really wasteful.

Given this, your constraints are that you want to run before (3) but after the
invalidates.  I suggest that whenever you get an invalidate() on the link
highlight's hosting GraphicsLayer, you call WebViewImpl::scheduleAnimation(). 
Then in WebViewImpl::updateAnimations() after calling
PageWidgetDelegate::animate() (which calls into WebCore to tick JS/CSS
animations, corresponding with step (1) above) you call some code on the link
highlight to find out where it should be.  At this point, the GraphicsLayer
tree should be up to date and it should be safe for you to manipulate the layer
tree.

It's important that you understand this flow to get this working well - if
something isn't clear please speak up, don't just blindly try things that
aren't clear.

> Source/WebKit/chromium/src/LinkHighlightTimer.h:60
> +    RefPtr<LinkHighlight> m_linkHighlight;

why is LinkHighlight ref counted? Does anyone else ever hold a reference to
this?

If it truly is ref counted, then you need to make sure to clear out all
backpointers when WebViewImpl / LinkHighlightTimer are destroyed.

> Source/WebKit/chromium/src/WebViewImpl.cpp:747
> +	   // Pr-empt any unstarted highlight animations, then fall through to
regular handler.

typo: "Pr-empt"

Will we always get a TapDown before a scroll begin? How do we decide whether to
send a TapDown to the main thread if we get a touch sequence that may result in
scrolling?