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

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


Better, but this needs a lot more testing.  I think you should hook up a way to
trigger the link highlight in DumpRenderTree from a layout test and then
generate a bunch of pixel tests for all permutations of 2d transforms
transforms / 3d transforms / iframes / scrolling / DOM mutations.

One case that I can't find any handling for here is the case where either the
DOM or the compositing tree changes such that the link Node switches to a
different composited layer mid-animation.  What's the story there?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:559
> +	   // If we are sure there is only one link highlight pointer active,
we can return from here, but
> +	   // the tree is relatively small so for safety's sake we traverse the
entire tree.

This comment doesn't make sense to me. What is it talking about?  When would
there be more than one link highlight?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:875
> +    // if they repaint a lot (e.g. during load) the highlight can flicker.

Why does the highlight flicker? That seems like a bug in this implementation

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:883
> +	   ASSERT(m_linkHighlight->numberOfChildren() == 1);
> +	   m_linkHighlight->childAt(0).invalidate();

You don't want to invalidate during *paint*, you need to invalidate the
highlight at the same time as the layer is invalidated so you paint at the same
time as the rest of the layer.

> Source/WebKit/chromium/src/LinkHighlight.cpp:63
> +    , m_clipLayer(WebLayer::create())

What is the clip layer used for? Can't you just size the contentLayer to the
bounds you want?

> Source/WebKit/chromium/src/LinkHighlight.cpp:64
> +    , m_path(path)

Why do you pass the Path in if you're passing a Node in - couldn't this code
figure out the Path itself just as easily?

> Source/WebKit/chromium/src/LinkHighlight.cpp:66
> +    , m_color(node ? node->renderer()->style()->tapHighlightColor() :
Color(0, 0, 0, 128))

Why would Node be null?

> Source/WebKit/chromium/src/LinkHighlight.cpp:82
> +    m_contentLayer.setPosition(WebFloatPoint(rect.x(), rect.y())); // WJM -
new!

What's this comment?

> Source/WebKit/chromium/src/LinkHighlight.cpp:84
> +    m_path.translate(FloatSize(-rect.x(), -rect.y()));

I can't find any logic that mutates m_path after the initial creation.	That
can't be right, the link could change shape arbitrarily while the timer is
active and you want to update the highlight to reflect that.

> Source/WebKit/chromium/src/LinkHighlight.cpp:116
> +	   IntPoint newPosition = m_node->getPixelSnappedRect().location();

I believe this gives you the position in absolute coordinates (specifically
absolute coordinates within the FrameView that the Node is in), which is not
what you want. You want to know the offset of the Node within its composited
layer.	To do this you probably need to do two steps:

1.) Figure out the position of the Node within its enclosing RenderLayer. To do
this, you probably have to iterate up RenderObjects until you hit the Node's
renderer's enclosingLayer()
2.) Figure out the RenderLayer's position within its composited ancestor. There
are (at least) two cases here:
 2.a) The composited layer is within the same frame. Just call
convertToLayerCoords() and keep in mind there might be rotations involved so
rects won't stay rects
 2.b) The composited layer is an ancestor frame. Then you need to figure out
the offset of the RenderLayer within its frame and then the offset of that
frame within the composited layer (keeping in mind there could be an arbitrary
number of intermediate frames).

I *strongly* recommend you start by constructing test cases for all of these
and then going through them one at a time.  To start, be sure to have tests
where the link is at different offsets to its compositing layer due to normal
css layout and transforms, tests where the composited layer is at different
offsets to its enclosing view, and tests with different layers of iframes
between the link and its composited layer.

> Source/WebKit/chromium/src/LinkHighlight.cpp:131
> +	   if (m_useFrameScroll && (view =
m_node->renderer()->frame()->view())) {

I don't understand this - this function should not need to care at all about
scrolling.  All you want to do in here is:

1.) Figure out which composited layer the Node is in (possibly this belongs
outside of paintContents, but it needs to happen)
2.) Figure out the bounding box and offset of the Node within its composited
layer
3.) Paint a highlight rect
4.) Position the highlight layer within its composited ancestor

At no point in this should you care at all about things as specific about
scroll offset.	A Node could be at a different offset within its layer for any
number of reasons - layout changing, transforms changing, etc etc etc.	All you
care about here is what the final offset result is.  Trying to track more
granular changes is just going to make this code super fragile without handling
anything more.

> Source/WebKit/chromium/src/LinkHighlight.cpp:176
> +    // We assume here that if mainFrameImpl() returns 0, then there's no
GraphicsLayer tree and
> +    // thus nothing to cleanup.

This comment doesn't make sense. At any point in time, the WebViewImpl has a
pointer to the root of the GraphicsLayer tree (m_rootGraphicsLayer).  You
should direct any walks through the GraphicsLayer tree through this.

> Source/WebKit/chromium/src/LinkHighlight.cpp:178
> +	   RenderLayer* renderLayer =
m_owningWebViewImpl->mainFrameImpl()->frame()->contentRenderer()->enclosingLaye
r();

This is fragile and unnecessary, WebViewImpl has the root of the GraphicsLayer
tree

> Source/WebKit/chromium/src/LinkHighlight.cpp:181
> +	   GraphicsLayer* gl = renderLayer->backing()->graphicsLayer();

No abbreviations in WebKit

> Source/WebKit/chromium/src/LinkHighlight.h:52
> +class LinkHighlight : public WebContentLayerClient, public
WebAnimationDelegate, public WTF::RefCounted<LinkHighlight> {

No WTF::

> Source/WebKit/chromium/src/LinkHighlight.h:75
> +    WTF::RefPtr<WebCore::Node> m_node;

No WTF::.  <wtf/RefPtr.h> has a using statement that puts WTF::RefPtr<> in the
global namespace, the other WTF smart pointer types do as well.  You never say
WTF:: in WebKit code

> Source/WebKit/chromium/src/LinkHighlightTimer.cpp:67
> +	   // FIXME: We handle startDelay == 0 separately to facilitate
testing, it would be nice to not need to.

Why do you need to do this for testing?

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

No WTF::

> Source/WebKit/chromium/src/WebViewImpl.cpp:532
> +    // The following block is development code only! DO NOT COMMIT!!
> +    // For review: uncomment lines below to test this feature using mouse
middle button clicks.

I think you should add a way to trigger the link highlight from within
DumpRenderTree for testing

> Source/WebKit/chromium/src/WebViewImpl.cpp:1120
> +bool highlightConditions(Node *node)

static, or put this in an anonymous namespace

The * belongs with the type - i.e "Node* node"

> Source/WebKit/chromium/src/WebViewImpl.cpp:1137
> +   
m_page->mainFrame()->eventHandler()->bestClickableNodeForTouchPoint(touchEventL
ocation, IntSize(4, 2), touchEventLocation, bestTouchNode);

Put the magic value IntSize(4, 2) in a descriptively-named local variable so we
can know what it's supposed to represent

> Source/WebKit/chromium/src/WebViewImpl.cpp:1152
> +    // FIXME: We would prefer to do something like path =
GestureTapHighlighter::pathForNodeHighlight(node), but
> +    // pathForNodeHighlight() doesn't always generate a path, and crashes in
some cases. For now we just use
> +    // a naive bounding box.

That's somewhat terrifying!

> Source/WebKit/chromium/src/WebViewImpl.cpp:1153
> +    path.addRoundedRect(node->getPixelSnappedRect(), FloatSize(3, 3));

Same comment as above re FloatSize(3, 3)

> Source/WebKit/chromium/src/WebViewImpl.cpp:1158
> +void WebViewImpl::enableTouchHighlight(IntPoint touchEventLocation)

This function is too long - it needs to be broken up into logical pieces

> Source/WebKit/chromium/src/WebViewImpl.cpp:1166
> +    // CSS specifications state that setting alpha to zero on tap highlight
color should disable highlights.

A specific citation here (spec name + section) would be better

> Source/WebKit/chromium/src/WebViewImpl.cpp:1190
> +    // Find bounds for clipping based on ScrollableArea.

Why does the link highlight get different clipping than the layer it lives in?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1205
> +    GraphicsLayerChromium* glc =
static_cast<GraphicsLayerChromium*>(renderLayer->backing()->graphicsLayer());

No abbreviations in WebKit

> Source/WebKit/chromium/src/WebViewImpl.cpp:1206
> +    if (!glc->drawsContent())

I don't understand this case, when will you hit this? Where's the test case for
this branch?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1213
> +    // Get coords figured out.

This comment doesn't seem terribly helpful - reword or refactor the code so
that it's not necessary

> Source/WebKit/chromium/src/WebViewImpl.cpp:1223
> +    if (didCrossFrameBoundary) {

I'm not sure what this is for, but it doesn't seem right. We may have crossed
an arbitrary number of frame offsets on our walk, each of which could have a
scroll offset, so offsetting by one scroll offset seems pretty dodgy.

Where are the tests for this case?

> Source/WebKit/chromium/src/WebViewImpl.cpp:1232
> +    // The next two lines seem only to matter in the composited overflow-div
case only.

I'm not sure what this comment is supposed to communicate to people reading the
code.  Where are the tests for it?