Bug 84487 – [chromium] Add touch link highlight animation layers.

• Component: New Bugs
• Status: RESOLVED FIXED

[W. James MacLean]

[chromium] Add touch link highlight animation layers. [Not For Review (yet)]

[W. James MacLean]

Created attachment 138151 [details]
Patch

[WebKit Review Bot]

Attachment 138151 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/ChangeLog:8:  You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible.  [changelog/nonewtests] [5]
Total errors found: 1 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[W. James MacLean]

This is a first draft of a (fairly) complete link highlighter.

* It has no tests yet.

* It contains some development code in WebViewImpl::handleMouseDown() to allow trying it if you have no way to generate a GestureTap* event.

* I don't think bestClickableNodeForTouchPoint() does a great job, this should be investigated.

* GestureTapHighlighter::pathForNodeHighlight() doesn't seem to always create a path! (But for an example of where it does, go to cbc.ca/news and click the banner advert at the top ... you'll get a rect with top-left corner rounded).

* There are a couple of FIXME's included where I need some input on what to do.

rjkroege@ and nduca@, comments?

[W. James MacLean]

(In reply to comment #3)

Oh, and there may be an issue in distanceSquaredToTargetCenterLine() as it seems to use window coords to test against the touch point, while the hit testing would seem to require contents coords from the touch point.

[WebKit Review Bot]

Comment on attachment 138151 [details]
Patch

Attachment 138151 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12474301

[James Robinson]

Comment on attachment 138151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138151&action=review

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:110
> +    virtual void addLinkHighlightLayer(const IntRect&, const Path&);
> +    virtual void didFinishLinkHighlightLayer();

does every single layer need a link highlight layer? the user-observable question is "are the link highlights clipped the same way the layer is?" that seems dubious - in many cases, the highlight would be completely clipped out if the layer is roughly the same size as the link.

i think a better answer is that link highlights are an overlay (on top of all layers) and should thus have one overlay layer per view instead of one per layer

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

this is really funky. if this is a ContentLayerDelegate, why is it also a layer? why not just instantiate a ContentLayerChromium with the appropriate delegate?

> Source/WebKit/chromium/src/TouchLinkHighlightTimer.cpp:55
> +GraphicsLayerChromium* getGraphicsLayerChromium(Node* hitNode)

this is crazy ugly. if the link highlight was a per-view overlay, then you wouldn't have to do any of this - just figure out the bounds and then paint at that point.

> Source/WebKit/chromium/src/TouchLinkHighlightTimer.cpp:106
> +    Node* hitNode = getHitNode(m_frame, m_touchPoint);

we hit test off a timer? what? why?

> Source/WebKit/chromium/src/WebViewImpl.cpp:685
> +        // Pr-empt any unstarted highlight animations, then fall through to regular handler.
> +        m_touchLinkHighlightTimer->stop(TouchLinkHighlightTimer::IgnoreActiveAnimation);

so wait, on the start of a scroll you get a TapDown then a ScrollBegin so you'll see the link highlight before the scrolling starts and then it just poofs away?

[W. James MacLean]

Comment on attachment 138151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138151&action=review

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:110
>> +    virtual void didFinishLinkHighlightLayer();
> 
> does every single layer need a link highlight layer? the user-observable question is "are the link highlights clipped the same way the layer is?" that seems dubious - in many cases, the highlight would be completely clipped out if the layer is roughly the same size as the link.
> 
> i think a better answer is that link highlights are an overlay (on top of all layers) and should thus have one overlay layer per view instead of one per layer

Not every layer will need a link highlight layer, but any layer that has a touchable link *may* need one. I would think the answer to the clipping question is "yes" - for example, a link in a scrollable (overflow?) div: if the target link is only partially visible, then I would prefer to only see the corresponding part of the highlight.

If link highlights are put in an overlay layer, then it seems to me that making the highlight scroll with the corresponding layer will be a lot of work.

I think the main argument against doing this per-layer is the storage of the OwnPtr, is that right? If so, is that worth the extra complication of figuring out how to scroll the highlight?

>> Source/WebCore/platform/graphics/chromium/LinkHighlightLayerChromium.h:38
>> +class LinkHighlightLayerChromium : public ContentLayerChromium, public ContentLayerDelegate, public CCLayerAnimationDelegate {
> 
> this is really funky. if this is a ContentLayerDelegate, why is it also a layer? why not just instantiate a ContentLayerChromium with the appropriate delegate?

I hadn't realised this would be considered weird, but I'll change it.

>> Source/WebKit/chromium/src/TouchLinkHighlightTimer.cpp:55
>> +GraphicsLayerChromium* getGraphicsLayerChromium(Node* hitNode)
> 
> this is crazy ugly. if the link highlight was a per-view overlay, then you wouldn't have to do any of this - just figure out the bounds and then paint at that point.

True, although I think you'd have a similar problem in that you'd have to hit-test to get the right target in order to get the bounds. It might not be any prettier. See my comment below about hit testing.

>> Source/WebKit/chromium/src/TouchLinkHighlightTimer.cpp:106
>> +    Node* hitNode = getHitNode(m_frame, m_touchPoint);
> 
> we hit test off a timer? what? why?

I wasn't happy about this either, I would have preferred to have kept the hit testing out of this class. But I wasn't sure if it is safe to assume that the hitNode (or other layers) would still be around when the timer fired, and didn't want to be holding a pointer that might go stale. If it's safe to assume the node will still be around, or if there's a safe way to test the validity of the pointer, then this would be unnecessary.

>> Source/WebKit/chromium/src/WebViewImpl.cpp:685
>> +        m_touchLinkHighlightTimer->stop(TouchLinkHighlightTimer::IgnoreActiveAnimation);
> 
> so wait, on the start of a scroll you get a TapDown then a ScrollBegin so you'll see the link highlight before the scrolling starts and then it just poofs away?

The sequence is this:
- on GestureTapDown, queue up an animation to occur in the near future
- if we get a scroll begin
  - if the animation is already running, let it fade away on its own
  - otherwise, don't let the animation start

[Robert Kroeger]

Comment on attachment 138151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138151&action=review

> Source/WebKit/chromium/src/TouchLinkHighlightTimer.cpp:44
> +Node* getHitNode(WebCore::Frame* frame, const IntPoint& touchPoint)

nit: i don't like the name touchPoint here -- a TouchPoint is something specific and related but quite different.

>>> Source/WebKit/chromium/src/TouchLinkHighlightTimer.cpp:106
>>> +    Node* hitNode = getHitNode(m_frame, m_touchPoint);
>> 
>> we hit test off a timer? what? why?
> 
> I wasn't happy about this either, I would have preferred to have kept the hit testing out of this class. But I wasn't sure if it is safe to assume that the hitNode (or other layers) would still be around when the timer fired, and didn't want to be holding a pointer that might go stale. If it's safe to assume the node will still be around, or if there's a safe way to test the validity of the pointer, then this would be unnecessary.

EventHandler stashes nodes (in a RefPtr). So could this as long as you check that its RenderBox still exists?

[Nat Duca]

Seems to me that these should be part of layers, not an overlay. We talked about this a week ago and we had consensus it seemed on the layers direction. Can you help me understand what changed  since then? Is it just less appetizing in practice than it seemed at the time?

The "it needs to move with scrolls" is  less an issue for link highlights, but iirw, the intent was to use this same functionality for selection handles. In that case, the use case of the annotation moving with scrolls is much clearer-cut. It seems that this is exactly what layers provide us and avoids all sorts of bugs, e.g. the highlight being on a layer that is oddly perspective rotated... a page overlay would have to duplicate a pretty impressive amount of transform stuff to get this right. Whereas, a with a sub layer on the graphics layer, it just works.

[W. James MacLean]

Making this bug the master bug for this effort, as the final patch is getting long. Will land the final patch which connects everything together against this bug.

[W. James MacLean]

Created attachment 142486 [details]
Patch

[W. James MacLean]

Created attachment 142508 [details]
Patch

[W. James MacLean]

(In reply to comment #12)
> Created an attachment (id=142508) [details]
> Patch

Ooops, forgot the .html file for the unit test.

[Tien-Ren Chen]

View in context: https://bugs.webkit.org/attachment.cgi?id=142508&action=review

> Source/WebKit/chromium/src/TouchLinkHighlightTimer.h:57
> +    WebCore::GraphicsLayerChromium* m_layer;

How do we ensure the layer's lifetime? Consider the following sequence:
1. The user tap down on a link, TouchLinkHighlightTimer initialized
2. Some javascript removed the link and its layer (assume the link has -webkit-transform:translateZ(0))
3. TouchLinkHighlightTimer::fired() access the dead m_layer
4. SIGSEGV

> Source/WebKit/chromium/src/WebViewImpl.cpp:1092
> +    RenderLayer* renderLayer = bestTouchNode->renderer()->enclosingLayer();
> +    RenderLayerBacking* backing = 0;
> +    do {
> +        backing = renderLayer->backing();
> +        renderLayer = renderLayer->parent();
> +    } while (!backing && renderLayer);

It is cleaner this way:
    RenderLayerBacking* backing = 0;
    for (RenderLayer* renderLayer = bestTouchNode->renderer()->enclosingLayer(); renderLayer; renderLayer = renderLayer->parent())
        if ((backing = renderLayer->backing()))
            break;

[Tien-Ren Chen]

View in context: https://bugs.webkit.org/attachment.cgi?id=142508&action=review

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:451
>  {
> -    if (m_linkHighlight)
> +    if (m_linkHighlight) {
> +        ASSERT(m_linkHighlight->contentLayer());
>          m_linkHighlight->contentLayer()->removeFromParent();
> -
> -    m_linkHighlight.clear();
> +        updateChildList();
> +        m_linkHighlight.clear();
> +    }

Looks weird. updateChildList() will add m_linkHighlight->contentLayer() back to the children of m_layer. Why not just
    if (!m_linkHighlight)
        return;
    m_linkHighlight.clear();
    updateChildList();

[Tien-Ren Chen]

IMO TouchLinkHighlightTimer should never hold reference to GraphicsLayerChromium.

Instead, I think LinkHighlight should be created at the time of WebViewImpl::enableTouchHighlight(), and the TouchLinkHighlightTimer and GraphicsLayerChromium cooperatively own the LinkHighlight. (Otherwise why LinkHighlight needs to be ref-counted?)

[W. James MacLean]

(In reply to comment #16)
> IMO TouchLinkHighlightTimer should never hold reference to GraphicsLayerChromium.
> 
> Instead, I think LinkHighlight should be created at the time of WebViewImpl::enableTouchHighlight(), and the TouchLinkHighlightTimer and GraphicsLayerChromium cooperatively own the LinkHighlight. (Otherwise why LinkHighlight needs to be ref-counted?)

Yes, that was going to be my next plan of attack.

I didn't quite realize that a graphics layer could disappear that quickly.

[W. James MacLean]

Created attachment 142729 [details]
Patch

[W. James MacLean]

(In reply to comment #18)
> Created an attachment (id=142729) [details]
> Patch

Modified to prevent TouchLinkHighlightTimer from having to hold a GraphicsLayerChromium* that might become stale. This required a re-work of some of the interface for LinkHighlight as we now need to create the LinkHighlight object at the time TouchLinkHighlightTimer::triggerTouchLinkHighlight() is called.

[W. James MacLean]

(In reply to comment #15)
> View in context: https://bugs.webkit.org/attachment.cgi?id=142508&action=review
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:451
> >  {
> > -    if (m_linkHighlight)
> > +    if (m_linkHighlight) {
> > +        ASSERT(m_linkHighlight->contentLayer());
> >          m_linkHighlight->contentLayer()->removeFromParent();
> > -
> > -    m_linkHighlight.clear();
> > +        updateChildList();
> > +        m_linkHighlight.clear();
> > +    }
> 
> Looks weird. updateChildList() will add m_linkHighlight->contentLayer() back to the children of m_layer. Why not just
>     if (!m_linkHighlight)
>         return;
>     m_linkHighlight.clear();
>     updateChildList();

I was concerned about lingering RefPtrs to the LinkHighlight object's content layer, but this seems to work OK.

[James Robinson]

Comment on attachment 142729 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=142729&action=review

I'm not sure I understand all of the lifetimes here yet.  If it helps, one property of WebCore::Timer<>s is that you can delete one to cancel it.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:126
> +    // If this layer already has active animations, the host needs to be notified.
> +    if (host && m_layerAnimationController->hasActiveAnimation())
> +        host->didAddAnimation();

This does not feel like it's logically part of the same patch - is this fixing an underlying bug in the animation system?

I would prefer to keep the link highlight logic in a different patch from general fixes.  This change also needs test coverage on its own (not link highlight tests).

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:670
> +    if (m_layerAnimationDelegate)
> +        m_layerAnimationDelegate->notifyAnimationFinished(wallClockTime);

same here as below.

when would you have an animation active but no animation delegate? that seems very odd

> Source/WebKit/chromium/src/WebViewImpl.cpp:503
> +
> +

spurious whitespace here

> Source/WebKit/chromium/src/WebViewImpl.cpp:1058
> +void WebViewImpl::enableTouchHighlight(IntPoint touchEventLocation, Path* outputPath)

this is a bit of an odd name. "enable" makes me think this is turning some setting on for the future, but this appears to be doing a good bit more

> Source/WebKit/chromium/src/WebViewImpl.cpp:1063
> +    // FIXME: make the padding size coume from the touch event generator.

typo "coume"

> Source/WebKit/chromium/src/WebViewImpl.cpp:1075
> +    // FIXME: At present we need to generate the path before checking for a valid graphics layer
> +    // in order to create a useful unit test. It would be nice instead to not bother generating
> +    // the path if no backing is found.

This is quite awkward. I think it's a sign that this function is doing too many distinct things and should be broken down into separate testable pieces that each do a distinct, logical bit of work.

> Source/WebKit/chromium/src/WebViewImpl.cpp:1092
> +    do {
> +        backing = renderLayer->backing();
> +        renderLayer = renderLayer->parent();
> +    } while (!backing && renderLayer);

this is very awkward. are you attempting to find this object's nearest enclosing composited layer? if so, loop up through parent pointers checking for isComposited() and then grab the backing from that.

what if you hit the root of the tree without hitting a composited layer (i.e. compositing is not on for this page, or the root is composited but RenderLayers are, or the node is in a subframe)?  have you tested these cases?

> Source/WebKit/chromium/src/WebViewImpl.h:59
> +#if ENABLE(GESTURE_EVENTS)
> +#include "TouchLinkHighlightTimer.h"
> +#endif

You should only need a forward declaration of this type, not the actual definition of the class

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

This is bad - we really should never be doing this in unit tests.  It probably means this testcase won't be able to run in the component build.

Why do you need this?

[W. James MacLean]

(In reply to comment #21)
> (From update of attachment 142729 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142729&action=review

I'll just address the major comments here, minor ones will just be fixed in the next patch (which won't come until early next week now).
 
> I'm not sure I understand all of the lifetimes here yet. If it helps, one property of WebCore::Timer<>s is that you can delete one to cancel it.

Good to know :-) I'll see if I can use this to simplify ...
 
> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:126
> > +    // If this layer already has active animations, the host needs to be notified.
> > +    if (host && m_layerAnimationController->hasActiveAnimation())
> > +        host->didAddAnimation();
> 
> This does not feel like it's logically part of the same patch - is this fixing an underlying bug in the animation system?

Yes.

> I would prefer to keep the link highlight logic in a different patch from general fixes.  This change also needs test coverage on its own (not link highlight tests).

OK. Will revise in separate patch.

> > Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:670
> > +    if (m_layerAnimationDelegate)
> > +        m_layerAnimationDelegate->notifyAnimationFinished(wallClockTime);
> 
> same here as below.
> 
> when would you have an animation active but no animation delegate? that seems very odd

I did run into it at one point. It could presumably happen if an animating layer was in the process of being deleted while this notification is propagating back from the CC thread? Ian Vollick thinks this is possible.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:1075
> > +    // FIXME: At present we need to generate the path before checking for a valid graphics layer
> > +    // in order to create a useful unit test. It would be nice instead to not bother generating
> > +    // the path if no backing is found.
> 
> This is quite awkward. I think it's a sign that this function is doing too many distinct things and should be broken down into separate testable pieces that each do a distinct, logical bit of work.

OK, will revise.
 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1092
> > +    do {
> > +        backing = renderLayer->backing();
> > +        renderLayer = renderLayer->parent();
> > +    } while (!backing && renderLayer);
> 
> this is very awkward. are you attempting to find this object's nearest enclosing composited layer? 

Yes.

> if so, loop up through parent pointers checking for isComposited() and then grab the backing from that.

Will do!

> what if you hit the root of the tree without hitting a composited layer (i.e. compositing is not on for this page, or the root is composited but RenderLayers are, or the node is in a subframe)?  have you tested these cases?

Then the function should exit without trying to generate a link highlight. Will double check to make sure this is indeed happening as planned.

> > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:39
> > +#include <webkit/support/webkit_support.h>
> 
> This is bad - we really should never be doing this in unit tests.  It probably means this testcase won't be able to run in the component build.
> 
> Why do you need this?

I can probably get the test to run OK without it. I was modelling it after another test I found, when I was trying to figure out an integration test that would test on actual HTML. I was unaware of the potential for conflict with the component build.

Will remove.

[Tien-Ren Chen]

Hello I made a new version of https://bugs.webkit.org/show_bug.cgi?id=84936
which provides a more accurate version of the highlight, and move some clutter in WebViewImpl::enableTouchHighlight to a new class called WebHighlight.

I also added some test cases in the bug attachment that shows the difference between my implementation and GestureTapHighlighter.

BTW, I rewrote the patch to be based on this patch, could you move 84936 from the dependency list to the block list?

[W. James MacLean]

Can I get some clarification on comments you made in #21?

>what if you hit the root of the tree without hitting a composited layer (i.e. compositing is not on for this page,

Here we presumably get a NULL ptr when we hit the root, right? In this case we exit without doing anything.

> or the root is composited but RenderLayers are,

Not sure what you mean here.

> or the node is in a subframe)?  

Not quite sure what this entails? So far we do not distinguish this case, and I'm not sure why we should (I'm assuming we can handle it via a clip-rect if needed?)

> have you tested these cases?

I'm not sure how to get a mock compositor/graphics-layers behind WebViewImpl to do an integration test for these cases.

[W. James MacLean]

Created attachment 146898 [details]
Patch

[W. James MacLean]

Comment on attachment 146898 [details]
Patch

This patch attempts to resolve (in most cases) correct behaviour when iframes and scrollable divs are used (wether or not they get their own layers).

It has the initial framework to make sure the highlight is clipped to the boundaries of the scroll region, but that is delayed on a method for finding the correct origin at which to place the clip layer when a non-composited scroll region is used.

I've used an 'observer' to get relevant scroll events to the LinkHighlight class, since highlights may now need to move as the region is scrolled (depending on which layer is composited). I attached the observer to ScrollAnimator as putting it in ScrollableArea seemed like a bad idea.

Is the use of an observer OK?

There is still work to be done, but I'd like to know if the overall approach is reasonable before sorting out the remaining issues. The patch has gotten large again, and needs to be broken up, so I'm not asking for a formal review but rather a 'quick look' at the approach.

Still to be done:
1) get the correct origin for the clipLayer when a non-composited scroll region is used, and enable clipping.
2) Make ScrollAnimator support a map of observers.
3) Determine logic for the case where nested scroll layers are used inside a composited container.
4) Add unit tests for link highlights in frames.

[W. James MacLean]

Created attachment 146901 [details]
A test page with scrolling iframe and scrolling div.

To test the case when the scrolling containers are not given composited layers, remove "translateZ(0)" from the style for the desired container.

[WebKit Review Bot]

Comment on attachment 146898 [details]
Patch

Attachment 146898 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12941504

New failing tests:
editing/selection/4895428-2.html
editing/pasteboard/4944770-2.html
http/tests/local/drag-over-remote-content.html
editing/selection/5195166-1.html
editing/deleting/5408255.html
editing/selection/4895428-1.html
editing/selection/5131716-1.html
editing/selection/5131716-3.html
editing/selection/4895428-3.html
editing/spelling/context-menu-suggestions.html
editing/selection/4895428-4.html
editing/shadow/selection-of-shadowroot.html
editing/selection/5131716-2.html
editing/spelling/design-mode-spellcheck-off.html
compositing/iframes/layout-on-compositing-change.html
editing/selection/14971.html
editing/deleting/smart-delete-004.html
editing/deleting/smart-delete-across-editable-boundaries-2.html
editing/selection/5057506.html
editing/selection/5131716-4.html
editing/selection/6476.html
editing/pasteboard/4947130.html
editing/deleting/smart-delete-001.html
editing/deleting/smart-delete-002.html
editing/execCommand/findString-2.html
editing/selection/5057506-2.html
editing/deleting/smart-delete-003.html
editing/shadow/compare-positions-in-nested-shadow.html

[WebKit Review Bot]

Created attachment 146991 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Iain Merrick]

Comment on attachment 146898 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=146898&action=review

> Source/WebCore/platform/ScrollAnimator.h:48
> +    virtual void notifyScroll(const IntSize&) = 0;

It's not clear from the method name whether this is an absolute scroll offset or a scroll delta.

Looks like it's a delta, from the implementation. Wouldn't it be more robust to send an absolute value, and/or provide the ScrollableArea itself as an extra parameter? That way the observer doesn't have to duplicate state that's already held in the scrollable.

> Source/WebCore/platform/ScrollAnimator.h:105
> +    void addScrollObserver(PassRefPtr<ScrollObserver>);

I think this method should be on ScrollableArea rather than ScrollAnimator. The observer is watching a property (the scroll position), and that property belongs to ScrollableArea. ScrollAnimator seems like an implementation detail.

Alternatively, implement your observer as ScrollableArea adapter, wrapping the real one. The animator calls your adapter, which siphons off the scroll value and repositions the link highlight, then calls the underlying ScrollableArea.

Either of those approaches would loosen the coupling between all these classes.

[Iain Merrick]

FYI, I uploaded a patch with the code we're currently using on Android (https://bugs.webkit.org/show_bug.cgi?id=89025) but it looks like that can be replaced with this patch when it's ready.

[W. James MacLean]

(In reply to comment #30)
> (From update of attachment 146898 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=146898&action=review
> 
> > Source/WebCore/platform/ScrollAnimator.h:48
> > +    virtual void notifyScroll(const IntSize&) = 0;
> 
> It's not clear from the method name whether this is an absolute scroll offset or a scroll delta.

Good point. We always want deltas so I will modify accordingly.

> Looks like it's a delta, from the implementation. Wouldn't it be more robust to send an absolute value, and/or provide the ScrollableArea itself as an extra parameter? That way the observer doesn't have to duplicate state that's already held in the scrollable.

If we use absolute position then we need to store last scroll position within LinkHighlight (it's doable, and an earlier version did this). But deltas are easier if available, as that's what we use to modify the highlight layer's transform. You're right that it would be nice to store the scrollable area, but it can in theory disappear without notice, and there's no way to RefPtr a ScrollableArea.

> > Source/WebCore/platform/ScrollAnimator.h:105
> > +    void addScrollObserver(PassRefPtr<ScrollObserver>);
> 
> I think this method should be on ScrollableArea rather than ScrollAnimator. The observer is watching a property (the scroll position), and that property belongs to ScrollableArea. ScrollAnimator seems like an implementation detail.

It's a good observation, and we originally wrote it this way, but if you look at the top of ScrollableArea.cpp you'll see there's a rather unique compiler check trying to warn people away from increasing the size of ScrollableArea, even by a little bit. Ultimately an observer infrastructure would want a list/map, which would be even bigger again. So I made the concious decision to move this to ScrollAnimator since it seems to be under less constraints with respect to size (and it's easier to generate deltas there.)

> Alternatively, implement your observer as ScrollableArea adapter, wrapping the real one. The animator calls your adapter, which siphons off the scroll value and repositions the link highlight, then calls the underlying ScrollableArea.

Again, I would think this runs into the problem of not knowing if and when the underlying ScrollableArea disappears, otherwise this would be an attractive option.
 
> Either of those approaches would loosen the coupling between all these classes.

I'm re-thinking some of this based on a conversation with Nat, to see if I can avoid the need for the entire observer framework altogether.

[Iain Merrick]

[...]
> It's a good observation, and we originally wrote it this way, but if you look at the top of ScrollableArea.cpp you'll see there's a rather unique compiler check trying to warn people away from increasing the size of ScrollableArea, even by a little bit.

Ha! I didn't notice that. OK, in that case, avoiding tinkering with ScrollableArea definitely makes sense (although it sounds like there's some underlying design issue that Somebody really ought to fix some day...)

[W. James MacLean]

(In reply to comment #33)
> [...]
> > It's a good observation, and we originally wrote it this way, but if you look at the top of ScrollableArea.cpp you'll see there's a rather unique compiler check trying to warn people away from increasing the size of ScrollableArea, even by a little bit.
> 
> Ha! I didn't notice that. OK, in that case, avoiding tinkering with ScrollableArea definitely makes sense (although it sounds like there's some underlying design issue that Somebody really ought to fix some day...)

It was a bit of a shock to me when I first encountered it via a strange-looking compiler error ;-)

[Iain Merrick]

*** Bug 89025 has been marked as a duplicate of this bug. ***

[W. James MacLean]

Created attachment 155373 [details]
Patch

[W. James MacLean]

This patch finishes implementing link highlight, although it has several limitations:

1) At present it uses a simplified highlight shape, namely just a rounded rect based on the target's bounding. When Tien-Ren's cl lands, we'll be able to have much nicer highlight shapes.

2) This patch retains the texture content layer, but in a future CL this can be modified to use one or more solid color layers to avoid the initial paint.

3) This patch at present assume the target is separated from its compositing layer by at most a single frame. This assumption will be relaxed in a follow-on patch, but at present meets the needs of most web sites (at worst the highlight won't scroll properly, but it shouldn't crash either).

4) This patch assumes frames/overflow-divs all clip, but this can easily be modified in a follow on CL.

Finally, I'd be interested in some input as to what might be a suitable test for the linkages from RenderLayer/FrameView down to GraphicsLayerChromium.

[W. James MacLean]

One more comment ... I'll be working tomorrow to add more unit tests, so what's presently in the CL will be expanded on before submission.

[James Robinson]

Comment on attachment 155373 [details]
Patch

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

[W. James MacLean]

(In reply to comment #39)
> (From update of attachment 155373 [details])
> 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?

No, there should be at most one link highlight active. A second GestureTapDown event while a highlight is present should cause the original highlight to be cancelled and a new one created.

> > 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.

I'd be happy not to have RenderLayer/FrameView push notifications. But GLC doesn't seem to get paint notifications on scroll. I can convert LinkHighlight to use WebLayer types, as they do seem to get the notifications, although the invalidations are broad, so I think we'd still need the Node* to figure out necessary layer transformations.

> > 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/

OK.

> > 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

Understood. This makes me wonder if LinkHighlight would be better off in Source/WebKit/chromium? Then we would need to create the LH object outside GLC and pass it in ... would that work?

That being said, if we can get more targeted scroll information, then LH would never need to retain the Node*, and the layering violations would disappear.

[W. James MacLean]

Created attachment 156119 [details]
Patch

[W. James MacLean]

This is a refactored version of the patch to address comments in #39.

* LinkHighlight now lives in WebKit, and uses the public WebX interface to access the compositor, thus removing layering violations

* The highlight updates its position in response to scrolls via GraphicsLayerChromium::paint() in order to remove the invasive addition of code to FrameView.

* GraphicsLayerChromium now only holds a WebLayer* to represent the LinkHighlight.

Possible improvements: I think we can improve performance if we introduce a LinkHighlightClient interface in WebCore so that (1) GLC holds a LinkHighlightClient*, and (2) computation of LinkHighlight position changes can thus be decoupled from GLC::paint() calls so that (i) we only invalidate the linkHighlight WebLayer when necessary, and (ii) LinkHighlight can update its position even in cases where invalidating its content layer won't lead to a repaint due to clipping.

I have temporarily disable the existing unit tests due to the refactor, but I am revising them right now.

[W. James MacLean]

Created attachment 156196 [details]
Patch

[W. James MacLean]

Revised patch that now includes refactored versions of the original unit tests, plus additional unit tests for the new functionality.

I will spend tomorrow creating more unit tests.

[WebKit Review Bot]

Comment on attachment 156196 [details]
Patch

Attachment 156196 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13423557

New failing tests:
fast/events/mouse-click-events.html
scrollbars/scrollbar-middleclick-nopaste.html
fast/events/fire-mousedown-while-pressing-mouse-button.html
platform/chromium/fast/loader/create-view-target-blank.html

[WebKit Review Bot]

Created attachment 156215 [details]
Archive of layout-test-results from gce-cr-linux-08

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-08  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[W. James MacLean]

Created attachment 156387 [details]
Patch

[W. James MacLean]

Revised to provide a guaranteed method of cleaning stale pointers out of the GraphicsLayerChromium tree. Also, commented out three lines of test code (left in for review purposes) so as not to fail on mouse button layout tests.

[James Robinson]

Comment on attachment 156387 [details]
Patch

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()->enclosingLayer();

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(touchEventLocation, 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?

[W. James MacLean]

I'm going to address comments, then start working on revising the CL as best I can. If any of my comments seem off-base, please let me know ASAP before I go down any undesirable paths. There is one question below regarding detecting when the target invalidates itself; I would appreciate feedback on how it is preferred I should do that.

(In reply to comment #49)
> (From update of attachment 156387 [details])
> 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.

I will file a bug to do this ... it will need to be in a separate CL.

> 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?

It's not a case I've run into before, although I had planned on addressing this in a follow-on CL. I had originally thought this case very unlikely, and had though putting the necessary logic for testing into LinkHighlight::paint() would be overly expensive. If it's not deemed overly expensive, I'm happy to move it in 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?

There should never be more than one link highlight. That being said, walking the GLC tree is relatively cheap so I thought it would be safer to do the complete walk. I'll remove this.

> > 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

Yes, it is a bug in this implementation (I mentioned this in comments embedded in the CL). I think the way to stop the flickering is to *not* invalidate the link highlight layer every time GLC::paint() is called. Instead, we need to check the position of the target node every time GLC::paint() is called, but we should only invalidate the highlight layer *if the target has moved. That was the whole reason I suggested using a LinkHighlightClient* so we could instead get the LinkHighlight to update it's position, and only invalidate if necessary. All this, of course, avoids the issue of calling invalidate() in paint(), which I address below.

In our VC you suggested a strong preference that GLC would hold a plain WebLayer* to the link highlight layer (as opposed to a LinkHighlightClient*), hence the current 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.

"at the same time as the layer is invalidated" - which layer are you referring to here?

I agree that invalidating during paint is wrong, but I thought it's what you recommended I do during our VC. Obviously I'd like to invalidate at the same time as the target node, but I am unaware of how to be notified of its invalidation. I've looked in GLC/RenderLayer/etc. but I'm not sure how this can be done. I had thought you were recommending using GLC::paint() as a way of knowing invalidation has occurred.

> > 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?

I asked other compositor folk about how to apply clipping, and using a separate clip layer was the recommendation. And, this seems more logical, because making the contentLayer the full size of the clip region is going to result in a texture that is (potentially) larger than it needs to be. (Correct me if I'm wrong.)

> > 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?

Yes it could. However, I figure we would just create the path once then re-use it, so I didn't see this as a problem. I will change this.

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

Unit testing.

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

Left over reminder to myself ... shouldn't have been in CL and will be removed.

> > 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.

I thought that re-computing the Path ever paint() might be too expensive (especially when we can use something more sophisticated than just a bounding box). I will fix this and re-generate the path on every paint().

> > 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).

OK, I will do my best to revise according to these instructions. I thought I had the coordinate conversions correct, but apparently not.

> 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.

Again, I will do this, but it will require breaking out into different CLs.

> > 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.

OK, will revise according to this description. I found that without accounting for scroll separately the highlight ended up in the wrong place, but this is possibly related to incorrect coordinates (see comments above).

> > 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.

When I was looking for a way to access the GLC tree from WVI, I missed m_rootGraphicsLayer, but did notice mainnFrameImpl() and knew I could get it from there. I'll fix this.
 
> > Source/WebKit/chromium/src/LinkHighlight.cpp:178
> > +        RenderLayer* renderLayer = m_owningWebViewImpl->mainFrameImpl()->frame()->contentRenderer()->enclosingLayer();
> 
> This is fragile and unnecessary, WebViewImpl has the root of the GraphicsLayer tree

See above.

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

Will fix.

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

Will fix.

> > 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

Will fix.

> > 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?

It has been my observation that calling startOneShot(0) doesn't lead to an immediate call to fired(), and that this in turn causes unit tests to fail when they set animationDelay 0 (in hopes of avoiding expensive synchronization mechanisms within the test). Is there some other way to handle this case?

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

Will fix.

> > 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

Will do. Possibly separate CL.
 
> > Source/WebKit/chromium/src/WebViewImpl.cpp:1120
> > +bool highlightConditions(Node *node)
> 
> static, or put this in an anonymous namespace

Will do.

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

Ooops, bad habit of mine, but I'm surprised check-webkit-style didn't pick it up (I always do this just before uploading a CL).

> > Source/WebKit/chromium/src/WebViewImpl.cpp:1137
> > +    m_page->mainFrame()->eventHandler()->bestClickableNodeForTouchPoint(touchEventLocation, 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

Will do.

> > 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!

What's terrifying, GestureTapHighlighter::pathForNodeHighligh crashing (I agree), or using a bounding box? ;-)

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

Will do.

> > 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

OK, will do.

> > 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

OK, will add this.

> > 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?

Because I thought the appropriate clip would be the bounds of the target-enclosing scrollable area. Since there is no reason to believe the bounds of the GLC we attach the link highlight will be the same as those of the target's layer (e.g. when we have to attach to the non-composited content host GLC). I will investigate and revise.

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

Will fix.

> > 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?

We discussed this in IM. If the non-composited content host is the only GLC that draws content, then finding the nearest enclosing GLC for the target node leads to a GLC for a RenderView that does not draw contents, and thus never gets paint() calls. I specifically asked if defaulting to the NCCH was acceptable in this case, and you said 'yes'. I have created a test for this that I was going to upload in the next CL, but it will be better in a layout test and so will do it there.

> > 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

Will do. This entire section of code the comment lives in will change substantially based on comments above, and I will improve the code-comments as part of that.

> > 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.

This will presumably be redundant once I figure out how to properly trigger invalidations and can remove the scroll-specific code in LinkHighlight.

> Where are the tests for this case?

Again, I have been working on them, but since we're going the LayoutTest route will re-formulate them there.
 
> > 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?

See above re improving code-comments.

[W. James MacLean]

(In reply to comment #49)
> 
> 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.
> 

Sorry for the e-mail spam, but I forgot to mention that I do actually have a number of these test ... they're manual though and have not been converted to unit/layout tests yet. I haven't done anything with CSS transforms, but certainly have iframes in scrolling divs, etc.

[W. James MacLean]

Created attachment 156701 [details]
Patch

[W. James MacLean]

This patch addresses the structural concerns with the previous patch, and does so in (I think) a principled way. Major changes are:

* the logic for determining the enclosing compositing layer has been moved out of WebViewImpl and into LinkHighlight
  - this allows us to switch GLCs to follow the target node as needed
* the target position w.r.t. the enclosing compositing layer is recomputed on every invalidation from the GLC, picking up information about software scrolls and layout changes
* the highlight Path is regenerated on every invalidation generated by the GLC
* coordinate transforms are now agnostic with respect to
  - any form of scroll information
  - the structure of nested frames, divs, etc. that the link resides in
* it now uses a LinkHighlightClient interface to avoid re-painting the Path on every invalidation generated by the GLC; instead LinkHighlight::paintContents() is only called if the size of the highlight rect changes - purely positional changes do not require a repaint.

Of course all of this broke the existing unit tests, and I am re-factoring those now. But I am interested in early feedback on this patch while I'm still working on the unit tests.

[W. James MacLean]

Forgot ... clipping is also disabled in this patch, but will be re-enabled shortly. Again, sorry for the e-mail spam.

[James Robinson]

Comment on attachment 156701 [details]
Patch

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?

[W. James MacLean]

Created attachment 157214 [details]
Patch

[W. James MacLean]

New patch, including some initial layout tests (please let me know if the general style of the tests looks OK, I'm continuing to create more for inclusion in the next version.)

Specific answers to review comments to follow.

[W. James MacLean]

(In reply to comment #55)
> (From update of attachment 156701 [details])
> 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.

Will post these shortly.
 
> > 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.

OK - removed. I couldn't reproduce a case that required it, but I wasn't sure.

> > 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.

Done.

> > 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.

Fixed.

> > 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()

Fixed.

> > 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!

No problem!

> > 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.

Will add a test in next iteration of the CL.

> > 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?

Out of date comment - removed.

> 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.

Thanks for the explanation. I didn't know about this ordering. Your instructions seem fairly clear, and I think I have incorporated the revised timing in the present version.

> > 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.

It used to be, but isn't any more. Have converted this to an OwnPtr.

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

Fixed.

> 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?

TapDown always goes to the main thread so far as I know, as it is only used to trigger link highlights. Scrolling, on the other hand, can happen in either thread. This piece of code only affects link highlights that have not yet started to animate, but there is a plan for the future to put the delay before showing the highlight into the gesture recognizer, at which time this logic can disappear.

[James Robinson]

Comment on attachment 157214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=157214&action=review

I think this is on the right track.  I particular want to see tests that involve crossing iframe boundaries and tests with transforms at different places.

> Source/WebKit/chromium/src/LinkHighlight.cpp:81
> +    m_contentLayer.setDrawsContent(true);
> +
> +    // We don't want to show the highlight until startAnimation is called, even though the highlight
> +    // layer is added to the tree immediately.
> +    m_contentLayer.setOpacity(0);
> +    m_contentLayer.setAnimationDelegate(this);

Why not set drawsContent to false until you're ready to show it?

> Source/WebKit/chromium/src/WebViewImpl.cpp:71
> +#include "GestureTapHighlighter.h"

is this actually being used?

> Source/WebKit/chromium/src/WebViewImpl.cpp:75
> +#include "GraphicsLayerChromium.h"

I don't believe you are using this include

> Source/WebKit/chromium/src/WebViewImpl.cpp:741
> +#if PLATFORM(CHROMIUM) && OS(LINUX)

I don't understand either compile guard - this file is in /chromium/, so we're definitely chromium-specific, and I can't think of anything linux-specific about this behavior. Do we even get GestureTapDowns on other platforms?

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:34
> +#include "LayerChromium.h"

I don't you you need this include

> Source/WebKit/chromium/tests/LinkHighlightTest.cpp:58
> +    WebCompositor::initialize(0);

If you call WebCompositor::initialize() you have to call WebCompositor::shutdown() or you'll make tests that run after you crash

[W. James MacLean]

(In reply to comment #59)
> (From update of attachment 157214 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157214&action=review
> 
> I think this is on the right track.  I particular want to see tests that involve crossing iframe boundaries and tests with transforms at different places.

I just found out yesterday from rjkroege@ that, at present, GestureTapDown isn't plumbed across iframe boundaries. I can write tests, but we can't run them without this, or without some temporary hack that bypasses this limitation. I have some manual test cases (attached) that work, if combined with the middle-mouse-button testing hack.

Can you expand on "transforms at different places"? I have tests for transforms (scale & rotation) in the main frame. Did you want these replicated in nested overflow-divs and frames?

> > Source/WebKit/chromium/src/LinkHighlight.cpp:81
> > +    m_contentLayer.setDrawsContent(true);
> > +
> > +    // We don't want to show the highlight until startAnimation is called, even though the highlight
> > +    // layer is added to the tree immediately.
> > +    m_contentLayer.setOpacity(0);
> > +    m_contentLayer.setAnimationDelegate(this);
> 
> Why not set drawsContent to false until you're ready to show it?

No reason ... done.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:71
> > +#include "GestureTapHighlighter.h"
> 
> is this actually being used?

Apparently not - removed. I had thought it was required for bestClickableNodeForTouchPoint() but it seems my memory was faulty.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:75
> > +#include "GraphicsLayerChromium.h"
> 
> I don't believe you are using this include

Correct.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:741
> > +#if PLATFORM(CHROMIUM) && OS(LINUX)
> 
> I don't understand either compile guard - this file is in /chromium/, so we're definitely chromium-specific, and I can't think of anything linux-specific about this behavior. Do we even get GestureTapDowns on other platforms?

Apparently touch with gestures is enabled on Windows, but we don't need PLATFORM(Chromium) - removed.

> > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:34
> > +#include "LayerChromium.h"
> 
> I don't you you need this include

Removed. Actually, there were a couple of other stale includes there, also removed.

> > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:58
> > +    WebCompositor::initialize(0);
> 
> If you call WebCompositor::initialize() you have to call WebCompositor::shutdown() or you'll make tests that run after you crash

Fixed.

[W. James MacLean]

Created attachment 157468 [details]
Some test cases with overflow divs and iframes, composited and non-composited.

[Tien-Ren Chen]

(In reply to comment #60)
> (In reply to comment #59)
> > (From update of attachment 157214 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=157214&action=review
> > 
> > I think this is on the right track.  I particular want to see tests that involve crossing iframe boundaries and tests with transforms at different places.
> 
> I just found out yesterday from rjkroege@ that, at present, GestureTapDown isn't plumbed across iframe boundaries. I can write tests, but we can't run them without this, or without some temporary hack that bypasses this limitation. I have some manual test cases (attached) that work, if combined with the middle-mouse-button testing hack.

That's quite surprising to me. As far as I know, when EventHandler::hitTestResultAtPoint finds out the result node is a RenderWidget that owns a FrameView, it will make a recursive call to do another hit test in the frame.

I'm working on the touch handling for on-demand zoom right now. If I come across with this bug I'll need to fix it too.

> Can you expand on "transforms at different places"? I have tests for transforms (scale & rotation) in the main frame. Did you want these replicated in nested overflow-divs and frames?

I think it's okay to leave some problem with transformation at this time. We're going to revisit them in #84936 anyway.

> > > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:58
> > > +    WebCompositor::initialize(0);
> > 
> > If you call WebCompositor::initialize() you have to call WebCompositor::shutdown() or you'll make tests that run after you crash
> 
> Fixed.

It's off-topic here, but is there a reason why we can't make this RAII?

[W. James MacLean]

(In reply to comment #62)
> (In reply to comment #60)
> > (In reply to comment #59)
> > > (From update of attachment 157214 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=157214&action=review
> > > 
> > > I think this is on the right track.  I particular want to see tests that involve crossing iframe boundaries and tests with transforms at different places.
> > 
> > I just found out yesterday from rjkroege@ that, at present, GestureTapDown isn't plumbed across iframe boundaries. I can write tests, but we can't run them without this, or without some temporary hack that bypasses this limitation. I have some manual test cases (attached) that work, if combined with the middle-mouse-button testing hack.
> 
> That's quite surprising to me. As far as I know, when EventHandler::hitTestResultAtPoint finds out the result node is a RenderWidget that owns a FrameView, it will make a recursive call to do another hit test in the frame.

I'm not sure of the full story here, and I'm experimenting with it right now myself.

> I'm working on the touch handling for on-demand zoom right now. If I come across with this bug I'll need to fix it too.
> 
> > Can you expand on "transforms at different places"? I have tests for transforms (scale & rotation) in the main frame. Did you want these replicated in nested overflow-divs and frames?
> 
> I think it's okay to leave some problem with transformation at this time. We're going to revisit them in #84936 anyway.

We're just going to create tests and file bugs ... I don't imagine all the transform issues will be fixed right away.

[W. James MacLean]

Created attachment 158027 [details]
Patch

[W. James MacLean]

(In reply to comment #64)
> Created an attachment (id=158027) [details]
> Patch

This patch adds a fair number of layout tests. Of course, coverage can be expanded further, but I think this is a good start, and includes tests expected to fail/be missing on account of unfinished features (clipping, transform support ... bug numbers will be filled in just before landing).

If there are specific test cases that need to be added, please let me know.

I had to re-work the code that *detects* when a layer switch is required, because in the case a new layer is added, the old layer doesn't always receive the required invalidations to prompt the switch (presumably when the new layer draws overtop?). I've modified the code in updateLayerIsDrawable() to handle this case, as well as adding code to handle when a layer is removed.

I have tested this manually and it seems to work (toggling translateZ(0) manually in Inspector while a highlight is active), though the layer switch doesn't seem to trigger when the layout tests are run in DRT (e.g. gesture-tapHighlight-1-overflow-div-scrolled-late-noncomposite.html, gesture-tapHighlight-1-overflow-div-scrolled-late-composite.html). I couldn't spot any other examples of triggering compositor layer changes in layout tests via JavaScript, so I'd appreciate thoughts here on how to guarantee they get triggered.

[WebKit Review Bot]

Comment on attachment 158027 [details]
Patch

Attachment 158027 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13487355

[W. James MacLean]

Created attachment 158077 [details]
Patch

[W. James MacLean]

Fixed compile error in unit test.

[WebKit Review Bot]

Comment on attachment 158077 [details]
Patch

Attachment 158077 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13493269

New failing tests:
platform/chromium/fast/loader/create-view-target-blank.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-scaledY.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-rotated.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-simple-scaledX.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-frame-scrolled-clipped.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-scroll-clip.html
fast/events/mouse-click-events.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-inner-clipped.html
scrollbars/scrollbar-middleclick-nopaste.html
fast/events/fire-mousedown-while-pressing-mouse-button.html
platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-outer-clipped.html

[WebKit Review Bot]

Created attachment 158098 [details]
Archive of layout-test-results from gce-cr-linux-06

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-06  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[James Robinson]

Comment on attachment 158077 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=158077&action=review

Thanks for adding the tests, although it seems like a number of them do not pass.  I'm confused by the static-ness of m_linkHighlightGlobal.

I'm also not quite sure by what you mean by "triggering compositor layer changes in layout tests via JavaScript" - do you mean that when the final display is made the layer tree doesn't reflect what you expect it to?  You can try doing a document.body.offsetTop before calling layoutTestController.display() if you are forcing manual painting to flush pending layout.  We should do a layout at the end of the test currently.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:83
> +LinkHighlightClient* GraphicsLayerChromium::m_linkHighlightGlobal = 0;

statics are named s_foo, since they aren't member variables.  This is quite gross - are you sure we need it?  I understand that the link highlight is global per view, but is it also global across multiple WebViews?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:177
> +    static LinkHighlightClient* m_linkHighlightGlobal;
> +    bool m_ownsLinkHighlight;

Could you explain why there's this setup instead of having a member variable m_linkHighlight that is null when this GLC does not have ownership of the link highlight?

> Source/WebKit/chromium/src/WebViewImpl.cpp:531
> +    // The following block is development code only! DO NOT COMMIT!!
> +    if (event.button == WebMouseEvent::ButtonMiddle) {
> +        enableTouchHighlight(IntPoint(event.x, event.y));
> +        return;
> +    }

Remove this

> Source/WebKit/chromium/src/WebViewImpl.cpp:742
> +#if OS(LINUX)

OS(LINUX) is almost certainly wrong here - are there even any other platforms that send GestureTapDown right now?

> Source/WebKit/chromium/src/WebViewImpl.cpp:749
> +        // Preempt any unstarted highlight animations, then fall through to regular handler.

This also looks wrong - if we start a scroll on the compositor thread then the main thread will never receive a GestureScrollBegin and the highlight will stay active, won't it?

> LayoutTests/platform/chromium/TestExpectations:3484
> +BUGWKxxxx1 LINUX : gesture-tapHighlight-1-overflow-div-scroll-clip.html = IMAGE

why are you marking these failing on linux? where are your baselines from?

you need to specify the full path if you want to actually suppress image diffs on them

[W. James MacLean]

(In reply to comment #71)
> (From update of attachment 158077 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=158077&action=review
> 
> Thanks for adding the tests, although it seems like a number of them do not pass.  I'm confused by the static-ness of m_linkHighlightGlobal.

It seems that layers other than the layer that owns the link highlight need to be able to invalidate it. Specifically, when a new layer is created, it may be the case that the link highlight will need to jump to it. But the old layer isn't guaranteed to receive the necessary invalidations (e.g. the new layer is a promoted chunk of the old layer), so it is up to the new layer to invalidate the link highlight, leading to it switching layers. Complicating this, the new layer may not be in the GLC tree at the time of the invalidation, thus the static is required. (I don't want every layer sending invalidations, so I use updateLayerIsDrawable() to detect the new layer becoming available, but it doesn't seem to have been added to the tree at the time this is called.) 
 
> I'm also not quite sure by what you mean by "triggering compositor layer changes in layout tests via JavaScript" - do you mean that when the final display is made the layer tree doesn't reflect what you expect it to?  You can try doing a document.body.offsetTop before calling layoutTestController.display() if you are forcing manual painting to flush pending layout.  We should do a layout at the end of the test currently.

Correct. I mean that my tests to verify layer switching happens correctly are ineffective (but not failing) because it seems as if the layer additions/deletions aren't actually happening during the layout test. I have manually verified that the layer switches do work via inspector and printing out diagnostic messages in the layer switch code, but I have been unable to replicate this output (sent to stderr) during the layout test (I have verified I should see the stderr output if I highlight a different link than the expected output is expecting, thus forcing nrwt to display the results.)

I have tried adding

document.body.offsetTop;
testRunner.display();

both between triggering the highlight and doing the layer promotion/demotion, and then again after the layer change, but I still can't seem to trigger the output that verified a layer change has occurred.

> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:83
> > +LinkHighlightClient* GraphicsLayerChromium::m_linkHighlightGlobal = 0;
> 
> statics are named s_foo, since they aren't member variables.  This is quite gross - are you sure we need it?  I understand that the link highlight is global per view, but is it also global across multiple WebViews?

So far, yes, I think we need it. updateLayerIsDrawable() seems like the right place to force invalidation of the highlight (since this layer is now a potential candidate to attach to), but as mentioned earlier I can't walk the tree looking for the highlight since the new layer may not be in the tree yet.

> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:177
> > +    static LinkHighlightClient* m_linkHighlightGlobal;
> > +    bool m_ownsLinkHighlight;
> 
> Could you explain why there's this setup instead of having a member variable m_linkHighlight that is null when this GLC does not have ownership of the link highlight?

I will add a comment explaining what I have described above (I'll try and make it shorter!)

> > Source/WebKit/chromium/src/WebViewImpl.cpp:531
> > +    // The following block is development code only! DO NOT COMMIT!!
> > +    if (event.button == WebMouseEvent::ButtonMiddle) {
> > +        enableTouchHighlight(IntPoint(event.x, event.y));
> > +        return;
> > +    }
> 
> Remove this

Certainly ... embarrassed to have forgotten it yet again.

> > Source/WebKit/chromium/src/WebViewImpl.cpp:742
> > +#if OS(LINUX)
> 
> OS(LINUX) is almost certainly wrong here - are there even any other platforms that send GestureTapDown right now?

rbyers@ tells me that gestures are currently being generated for Windows also (in preparation for Win8).

> > Source/WebKit/chromium/src/WebViewImpl.cpp:749
> > +        // Preempt any unstarted highlight animations, then fall through to regular handler.
> 
> This also looks wrong - if we start a scroll on the compositor thread then the main thread will never receive a GestureScrollBegin and the highlight will stay active, won't it?

I'm going to defer to rjkroege@ on this, but yes, your comment is valid.

> > LayoutTests/platform/chromium/TestExpectations:3484
> > +BUGWKxxxx1 LINUX : gesture-tapHighlight-1-overflow-div-scroll-clip.html = IMAGE
> 
> why are you marking these failing on linux?

These are the tests we had agreed would be included (and marked as failing) to remind us of the remaining work: (1) improved handling of transforms, and (ii) better clipping support.

> where are your baselines from?

There are no baselines for the transform case (marked as MISSING), and the clipping baselines were done manually, by commenting out the code that triggered the highlight then running the test.

> you need to specify the full path if you want to actually suppress image diffs on them

I missed that ... fixed.

[James Robinson]

(In reply to comment #72)
> (In reply to comment #71)
> > (From update of attachment 158077 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158077&action=review
> > 
> > Thanks for adding the tests, although it seems like a number of them do not pass.  I'm confused by the static-ness of m_linkHighlightGlobal.
> 
> It seems that layers other than the layer that owns the link highlight need to be able to invalidate it. Specifically, when a new layer is created, it may be the case that the link highlight will need to jump to it. But the old layer isn't guaranteed to receive the necessary invalidations (e.g. the new layer is a promoted chunk of the old layer), so it is up to the new layer to invalidate the link highlight, leading to it switching layers. Complicating this, the new layer may not be in the GLC tree at the time of the invalidation, thus the static is required. (I don't want every layer sending invalidations, so I use updateLayerIsDrawable() to detect the new layer becoming available, but it doesn't seem to have been added to the tree at the time this is called.) 
> 

Which test(s) are you seeing this behavior on?  I'd like to take a closer look at what's going on here.

> > I'm also not quite sure by what you mean by "triggering compositor layer changes in layout tests via JavaScript" - do you mean that when the final display is made the layer tree doesn't reflect what you expect it to?  You can try doing a document.body.offsetTop before calling layoutTestController.display() if you are forcing manual painting to flush pending layout.  We should do a layout at the end of the test currently.
> 
> Correct. I mean that my tests to verify layer switching happens correctly are ineffective (but not failing) because it seems as if the layer additions/deletions aren't actually happening during the layout test. I have manually verified that the layer switches do work via inspector and printing out diagnostic messages in the layer switch code, but I have been unable to replicate this output (sent to stderr) during the layout test (I have verified I should see the stderr output if I highlight a different link than the expected output is expecting, thus forcing nrwt to display the results.)
> 
> I have tried adding
> 
> document.body.offsetTop;
> testRunner.display();
> 
> both between triggering the highlight and doing the layer promotion/demotion, and then again after the layer change, but I still can't seem to trigger the output that verified a layer change has occurred.

Which test(s) are you seeing this behavior on?
 
> > > Source/WebKit/chromium/src/WebViewImpl.cpp:742
> > > +#if OS(LINUX)
> > 
> > OS(LINUX) is almost certainly wrong here - are there even any other platforms that send GestureTapDown right now?
> 
> rbyers@ tells me that gestures are currently being generated for Windows also (in preparation for Win8).

Yes, and I would expect you would want the same behavior there - right?  I would expect the same gesture on Chrome should behave the same everywhere.

[W. James MacLean]

(In reply to comment #73)
> 
> Which test(s) are you seeing this behavior on?  I'd like to take a closer look at what's going on here.

Actually, no need. I tracked it down ... it was a combination of (1) requiring testRunner.display() and (2) assigning to .style.webkitTransformStyle when I should have used .style.webkitTransform instead.

Seems to be working now.
 
> > > > Source/WebKit/chromium/src/WebViewImpl.cpp:742
> > > > +#if OS(LINUX)
> > > 
> > > OS(LINUX) is almost certainly wrong here - are there even any other platforms that send GestureTapDown right now?
> > 
> > rbyers@ tells me that gestures are currently being generated for Windows also (in preparation for Win8).
> 
> Yes, and I would expect you would want the same behavior there - right?  I would expect the same gesture on Chrome should behave the same everywhere.

Ultimately, yes. But the plan had been to *first* enable and test just on Linuz, and then expand the scope to include Windows once we have a bit more experience with it on Linux.

If you prefer I can remove the flag and we'll enable it both places at once. Please let me know asap, as I am ready upload a revised patch anytime now.

[W. James MacLean]

Created attachment 158576 [details]
Patch

[W. James MacLean]

Revised patch that fixes some layout tests (layering changes were not being invoked properly before, they are now), makes the deletion of a LinkHighlight safer (previously required the LinkHighlight to be destructed before WebViewImpl's GraphicsLayer tree was torn down, now works regardless), and removes LinkHighlightTimer (the delay functionality it provided is being moved to the Gesture recognizer by rjkroege@).

[James Robinson]

(In reply to comment #72)
> (In reply to comment #71)
> > (From update of attachment 158077 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=158077&action=review
> > 
> > Thanks for adding the tests, although it seems like a number of them do not pass.  I'm confused by the static-ness of m_linkHighlightGlobal.
> 
> It seems that layers other than the layer that owns the link highlight need to be able to invalidate it. Specifically, when a new layer is created, it may be the case that the link highlight will need to jump to it. But the old layer isn't guaranteed to receive the necessary invalidations (e.g. the new layer is a promoted chunk of the old layer), so it is up to the new layer to invalidate the link highlight, leading to it switching layers.

Please provide a test case showing this happening - my understanding is that if some content on composited layer A is promoted to composited layer B, both A and B will receive invalidations (B because it is new, A because some of the content previously on it are now on a different composited layer).  I would be really surprised if this wasn't the case since we would see fairly obvious rendering issues.

[W. James MacLean]

Created attachment 158585 [details]
Patch

[W. James MacLean]

Comment on attachment 158585 [details]
Patch

Ooops, more to fix than test expectations merge.

[W. James MacLean]

(In reply to comment #77)
> 
> Please provide a test case showing this happening - my understanding is that if some content on composited layer A is promoted to composited layer B, both A and B will receive invalidations (B because it is new, A because some of the content previously on it are now on a different composited layer).  I would be really surprised if this wasn't the case since we would see fairly obvious rendering issues.

Sorry for the delay, but ran into a confounding issue where the manual tests fail without the global invalidation (from the new GLC), but layout tests didn't.

**Short summary**

When I ran the tests this time, there *are* invalidations from the old GLC, but they happen before the new GLC is created, and *they get handled via the WebViewImpl animation callback* before the new GLC is created, meaning the highlight can't jump yet (although it does seem to alter the coordinate computations enough to move the highlight to the wrong place, so maybe the Node* has moved before the new GLC is ready?

DRT seems not to handle the callback as fast, so the new GLC *is* ready and the layer switch occurs.

So we still seem to need the global invalidation.

*** End of Short Summary ***

I don't know if this counts as a "test", but it is real output from GLC & LH objects. In GLC, I've generated stderr logging in *every* place in GLC that m_layer.invalidate() is called, regardless of wether there is an active layer. '//' comments are my manual annotations.

Case #1a: No global notification; manual test

wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f54f1b17c80  // starts with single drawable GLC (NCCH)
wjm: LinkHighlight::invalidate() this = 0x7f54e3a1fe00                     // Highlight link in overflow div
wjm: LinkHighlight::updateGeometry() this = 0x7f54e3a1fe00
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7f54e3a1fe00
wjm: Adding LH=0x7f54e3a1fe00 to GLC=0x7f54f1b17c80
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f54f1b17c80  // Promote overflow div via "translateZ(0)" using Inspector
wjm: LinkHighlight::invalidate() this = 0x7f54e3a1fe00
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f54f1b17c80
wjm: LinkHighlight::invalidate() this = 0x7f54e3a1fe00
wjm: LinkHighlight::updateGeometry() this = 0x7f54e3a1fe00
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7f54e3a1fe00
wjm: GraphicsLayerChromium::setSize() this = 0x7f54f1b17500                // New GLC created *after* LH has responded to invalidate from old
wjm: GraphicsLayerChromium::setSize() this = 0x7f54e4cbd780
wjm: GraphicsLayerChromium::updateLayerIsDrawable() this = 0x7f54e4cbd780
wjm: LinkHighlight::clearGraphicsLayerHighlightLinkPointer() this = 0x7f54e3a1fe00  // Highlight removes itself from layer, never switches
// Observation: highlight displays incorrectly since not attached to newly promoted layer, but target *is* in new layer
// Observation: if we scroll overflow div, then highlight switches to correct layer as scrollbars are still in old GLC
//              and this forces required invalidation.


Case#2a: No global notification; layout test via DRT ()

wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7fe86440e180 // starts with single drawable GLC (NCCH)
wjm: LinkHighlight::invalidate() this = 0x7fe858d7dcb0                    // Highlight link in overflow div
wjm: Adding LH=0x7fe858d7dcb0 to GLC=0x7fe86440e180
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7fe86440e180
wjm: LinkHighlight::invalidate() this = 0x7fe858d7dcb0
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7fe86440e180
wjm: LinkHighlight::invalidate() this = 0x7fe858d7dcb0
wjm: GraphicsLayerChromium::setSize() this = 0x7fe855262780
wjm: GraphicsLayerChromium::setSize() this = 0x7fe855263180
wjm: GraphicsLayerChromium::updateLayerIsDrawable() this = 0x7fe855263180
wjm: LinkHighlight::clearGraphicsLayerHighlightLinkPointer() this = 0x7fe858d7dcb0
wjm: LayerSwitch 0x7fe86440e180 -> 0x7fe855263180
wjm: Adding LH=0x7fe858d7dcb0 to GLC=0x7fe855263180

wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7fc40933c180 // starts with single drawable GLC (NCCH)
wjm: LinkHighlight::invalidate() this = 0x7fc3fdcabcb0                    // Highlight link in overflow div
wjm: LinkHighlight::updateGeometry() this = 0x7fc3fdcabcb0
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7fc3fdcabcb0
wjm: Adding LH=0x7fc3fdcabcb0 to GLC=0x7fc40933c180                       // Link added to NCCH
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7fc40933c180 // Promote overflow div via "translateZ(0)" using Javascript
wjm: LinkHighlight::invalidate() this = 0x7fc3fdcabcb0
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7fc40933c180
wjm: LinkHighlight::invalidate() this = 0x7fc3fdcabcb0
wjm: GraphicsLayerChromium::setSize() this = 0x7fc3fa190780                // New GLC created *after* LH has responded to invalidate from old
wjm: GraphicsLayerChromium::setSize() this = 0x7fc3fa191180
wjm: GraphicsLayerChromium::updateLayerIsDrawable() this = 0x7fc3fa191180
wjm: LinkHighlight::updateGeometry() this = 0x7fc3fdcabcb0                 // This is from the invalidate sent *before* the new GLC existed!
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7fc3fdcabcb0 // Why does this take so long in DRT
wjm: LinkHighlight::clearGraphicsLayerHighlightLinkPointer() this = 0x7fc3fdcabcb0
wjm: LayerSwitch 0x7fc40933c180 -> 0x7fc3fa191180
wjm: Adding LH=0x7fc3fdcabcb0 to GLC=0x7fc3fa191180

Case #1b: Global notification; manual test

wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f74b6b06c80 // As in Case #1a ...
wjm: LinkHighlight::invalidate() this = 0x7f74a9c7ccb0
wjm: LinkHighlight::updateGeometry() this = 0x7f74a9c7ccb0
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7f74a9c7ccb0
wjm: Adding LH=0x7f74a9c7ccb0 to GLC=0x7f74b6b06c80
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f74b6b06c80
wjm: LinkHighlight::invalidate() this = 0x7f74a9c7ccb0
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f74b6b06c80
wjm: LinkHighlight::invalidate() this = 0x7f74a9c7ccb0
wjm: LinkHighlight::updateGeometry() this = 0x7f74a9c7ccb0
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7f74a9c7ccb0
wjm: GraphicsLayerChromium::setSize() this = 0x7f74b6b06500
wjm: GraphicsLayerChromium::setSize() this = 0x7f74a9cab780
wjm: GraphicsLayerChromium::updateLayerIsDrawable() this = 0x7f74a9cab780
wjm: LinkHighlight::invalidate() this = 0x7f74a9c7ccb0                       // ... but now we get a validation *after* the new GLC created
wjm: LinkHighlight::updateGeometry() this = 0x7f74a9c7ccb0
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7f74a9c7ccb0
wjm: LinkHighlight::clearGraphicsLayerHighlightLinkPointer() this = 0x7f74a9c7ccb0
wjm: LayerSwitch 0x7f74b6b06c80 -> 0x7f74a9cab780                            // Here's the layer switch we were looking for.
wjm: Adding LH=0x7f74a9c7ccb0 to GLC=0x7f74a9cab780
wjm: LinkHighlight::clearGraphicsLayerHighlightLinkPointer() this = 0x7f74a9c7ccb0

Case#2b: No global notification; layout test via DRT ()

wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f33c6f7b180 // As in Case #2a ...
wjm: LinkHighlight::invalidate() this = 0x7f33bb8eacb0
wjm: LinkHighlight::updateGeometry() this = 0x7f33bb8eacb0
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7f33bb8eacb0
wjm: Adding LH=0x7f33bb8eacb0 to GLC=0x7f33c6f7b180
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f33c6f7b180
wjm: LinkHighlight::invalidate() this = 0x7f33bb8eacb0
wjm: GraphicsLayerChromium::setNeedsDisplayInRect() this = 0x7f33c6f7b180
wjm: LinkHighlight::invalidate() this = 0x7f33bb8eacb0
wjm: GraphicsLayerChromium::setSize() this = 0x7f33b7dbf780
wjm: GraphicsLayerChromium::setSize() this = 0x7f33b7dc0180
wjm: GraphicsLayerChromium::updateLayerIsDrawable() this = 0x7f33b7dc0180
wjm: LinkHighlight::invalidate() this = 0x7f33bb8eacb0                    // Invalidate from afetr creation of new GLC.
wjm: LinkHighlight::updateGeometry() this = 0x7f33bb8eacb0                // Again, why so long to service?
wjm: LinkHighlight::computeEnclosingCompositingLayer(), this =0x7f33bb8eacb0
wjm: LinkHighlight::clearGraphicsLayerHighlightLinkPointer() this = 0x7f33bb8eacb0
wjm: LayerSwitch 0x7f33c6f7b180 -> 0x7f33b7dc0180
wjm: Adding LH=0x7f33bb8eacb0 to GLC=0x7f33b7dc0180

[W. James MacLean]

As a follow up to the previous comment, I was curious as to why the invalidation of the old layer is divorced from the creation of the new layer. Doing a backtrace (below) from the invalidation of the old layer shows that this invalidation (at least for the non-composited content host) seems to be generated independently from the call to RenderLayerCompositor::rebuildCompositingLayerTree() that introduces the new layer. This makes me wonder if the difference between the DumpRenderTree case and the manual test case (using Inspector) are caused by a race condition.

#0  WebKit::LinkHighlight::invalidate (this=0x7fffdcb7ce70) at ../../third_party/WebKit/Source/WebKit/chromium/src/LinkHighlight.cpp:258
#1  0x00007fffef0a95b0 in WebCore::GraphicsLayerChromium::setNeedsDisplayInRect (this=0x7fffdfb29a00, rect=...) at ../../third_party/WebKit/Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:478
#2  0x00007fffedfd5cf6 in WebKit::NonCompositedContentHost::invalidateRect (this=0x7fffdddf1140, rect=...) at ../../third_party/WebKit/Source/WebKit/chromium/src/NonCompositedContentHost.cpp:161
#3  0x00007fffee094cf5 in WebKit::WebViewImpl::invalidateRootLayerRect (this=0x7fffdfb01380, rect=...) at ../../third_party/WebKit/Source/WebKit/chromium/src/WebViewImpl.cpp:3625
#4  0x00007fffedfa1b29 in WebKit::ChromeClientImpl::invalidateContentsAndRootView (this=0x7fffdfb01400, updateRect=...) at ../../third_party/WebKit/Source/WebKit/chromium/src/Chr
omeClientImpl.cpp:515
#5  0x00007fffef7861f6 in WebCore::Chrome::invalidateContentsAndRootView (this=0x7fffdfb68760, updateRect=..., immediate=false) at ../../third_party/WebKit/Source/WebCore/page/Chrome.cpp:87
#6  0x00007fffeeff5859 in WebCore::ScrollView::repaintContentRectangle (this=0x7fffdfb01000, rect=..., now=false) at ../../third_party/WebKit/Source/WebCore/platform/ScrollView.cpp:950
#7  0x00007fffef7e39c3 in WebCore::FrameView::doDeferredRepaints (this=0x7fffdfb01000) at
 ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:1974
#8  0x00007fffef7e3786 in WebCore::FrameView::endDeferredRepaints (this=0x7fffdfb01000) at ../../third_party/WebKit/Source/WebCore/page/FrameView.cpp:1922
#9  0x00007fffeeaf5881 in WebCore::Document::recalcStyle (this=0x7fffdcf2a000, change=WebCore::Node::NoChange) at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:1874
#10 0x00007fffeeaf5a4e in WebCore::Document::updateStyleIfNeeded (this=0x7fffdcf2a000) at
 ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:1897
#11 0x00007fffeeaf52cb in WebCore::Document::styleRecalcTimerFired (this=0x7fffdcf2a000)
at ../../third_party/WebKit/Source/WebCore/dom/Document.cpp:1782

[W. James MacLean]

Created attachment 158846 [details]
Patch

[James Robinson]

I've spent a while thinking about this and pondering your logs.  I'll keep thinking about this tomorrow.  What is the exact test page you are testing on and what flags are you passing in to chrome?  Please post *exact* steps (including what you are loading, what you are toggling in dev tools, etc) so I can see exactly what you are seeing.

It shouldn't really matter when invalidations come in, what's important is when the layer tree is rebuilt vs when you look for the pointer. Perhaps at this time we haven't triggered layout for painting yet - you could see if things get better if you hook in WebViewImpl::layout() instead of animate().

A static pointer seems like it couldn't be right since you can have multiple views loaded in the same process handling touch events - how could that work?

[W. James MacLean]

Spoiler: To avoid reading this entire comment, I'll repeat here that the invalidation from the GLC losing the target node + calling from WVI::layout() instead of WVI::updateAnimations() appears to work, although there could be cases I'm unaware of that wouldn't (do you know of any?). On this basis, I'm happy to go back to the individually-owned-pointer case and not use the static pointer, so long as we don't know of any such cases up front.

(In reply to comment #83)
> I've spent a while thinking about this and pondering your logs.  I'll keep thinking about this tomorrow.  
>What is the exact test page you are testing on and what flags are you passing in to chrome?  

platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-scrolled-late-noncomposite.html

>Please post *exact* steps (including what you are loading, what you are toggling in dev tools, etc) so I can see exactly what you are seeing.

1) Apply the patch to repo & build plain chrome (no compiler flags required, although I enabled component build). 

To see the incorrect behaviour, make the follow modification in GLC::updateLayerIsDrawable():

- if (s_linkHighlight)
+ if (ownsLingHighlight())

To see the correct behaviour, leave unchanged.

Useful mods:

a) It's helpful to extend the LinkHighlight animation duration to about 8 seconds () so the highlight stays visible long enough to do the manual steps below. 

b) Also, make sure you enable the hack to trigger highlights from the middle mouse button if you don't have a touchscreen.

c) Add whatever logging calls/printfs to various functions (good ones are LH constructor, destructor, invalidate, updateGeometry, and any GLC function that invokes m_layer.invalidate().)

2) My command line is:

out/Debug/chrome --user-data-dir=/tmp --force-compositing-mode --enable-threaded-compositing --disable-seccomp-sandbox out/Debug/chrome --user-data-dir=/tmp --force-compositing-mode --enable-threaded-compositing --disable-seccomp-sandbox

The only default flag I have set (via about:flags) is --show-compositing-layer-borders (works the same with or without this, as expected).
 
3) Once the page comes up, the procedure is as follows:

i) launch Inspector in a separate window (I use a separate window to make it quicker to turn the layer promotion on/off via the mouse).

ii) uncheck the -webkit-transform property on "divToControlCompositedLayer" (we'll test layer promotion, as demotion works fine via pathway in GLC::willBeDestroyed()).

iii) Middle-click on a link in the scrollable dive - the highlight should appear.

iv) Before the highlight fades, click on the check box to re-enable the webkit-transform for divToControlCompositedLayer.

v) Look at logging output.

> It shouldn't really matter when invalidations come in, what's important is when the layer tree is rebuilt vs when you look for the pointer.

Exactly, although at present invalidations from the old layer seem to be processed by LH too early to be useful, so in a way it does matter.

> Perhaps at this time we haven't triggered layout for painting yet - you could see if things get better if you hook in WebViewImpl::layout() instead of animate().

WebViewImpl::layout() does seem to work better ... here the invalidations get handled after the tree is rebuilt it seems (are there corner cases though?).
 
> A static pointer seems like it couldn't be right since you can have multiple views loaded in the same process handling touch events - how could that work?

Since, as far as the UI experience is concerned, the link highlighter is sytem global, it's fine so long as we can make sure the multiple views don't get into contention over the shared resource. Perhaps the LinkHighlight pointer shoudl be static in WebViewImpl? (OK, I know that's the last thing you want to hear, though it makes sense!)

[W. James MacLean]

Created attachment 159155 [details]
Patch

[W. James MacLean]

(In reply to comment #85)
> Created an attachment (id=159155) [details]
> Patch

This patch reverts to using member pointer vars in GLC, and triggers geometry updates from WVI::layout().

Also, we no longer walk the tree to remove old link highlights (a problem if the owning GLC is no longer in the tree: now we have the owning GLC sign    al if it is being destroyed, and otherwise use the m_currentGraphicsLayer pointer in LH to remove.

Finally, have created bugs numbers for follow-on work and included these in TestExpectations.

[W. James MacLean]

Grrrr ... stupid TestExpectations!

[James Robinson]

Comment on attachment 159155 [details]
Patch

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?

[W. James MacLean]

Created attachment 159477 [details]
Patch

[W. James MacLean]

(In reply to comment #88)
> (From update of attachment 159155 [details])
> 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

Fixed.

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

Fixed.

> > 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()?

Renamed to clearCurrentGraphicsLayer().

> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:124
> > +    bool ownsLinkHighlight();
> 
> I can't find any callers of this - dead?

Dead. I kept it around during re-factoring thinking I was going to use it, then never did.

> > 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?

Dead. This one I just missed deleting.

> > 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

Dead.

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

Fixed.

> > 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

Guards removed.

> > 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

Yeah, it used to be true, then it got missed in the re-factoring cleanup.

> > 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

Ooops, did not know. Fixed.

> > Source/WebKit/chromium/src/LinkHighlight.cpp:219
> > +    invalidate();
> 
> if you patch invalidate() as suggested (see below), remember to schedule an animation explicitly here

Done.

> > 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.

True, removed.

> 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

Done.

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

Fixed.

> > Source/WebKit/chromium/src/WebViewImpl.h:44
> > +#if ENABLE(GESTURE_EVENTS)
> > +#endif
> 
> looks like whatever you had here isn't need any more

Removed.

> > Source/WebKit/chromium/tests/LinkHighlightTest.cpp:40
> > +#include <webkit/support/webkit_support.h>
> 
> it looks like you don't need this any more

Removed.

> > 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

Revised for all 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)

:-) Removed all frameset-based tests.

> > 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?

Ooops, we had some duplicate expectations. This one (and a few others) were out of date - removed.

[James Robinson]

Comment on attachment 159477 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=159477&action=review

R=me

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:126
> +        // If we have an active link highlight, it may need to transfer to a new GraphicsLayer,
> +        // so we need to give it a chance to update its layer affiliation.
> +        m_linkHighlight->invalidate();

this comment is misleading - the linkHighlight doesn't update its layer affiliation in the invalidate() call.

can you just make clearCurrent...() set m_geometryNeedsUpdate on the LinkHighlight? if the GraphicsLayerChromium the highlight is attached to goes away then it definitely needs to find a new owner

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:800
> +        // This is the only place where the global copy of the LinkHighlightClient pointer
> +        // is dereferenced. GraphicsLayerChromium::updateLayerIsDrawable() may be called before
> +        // this layer is added into the tree, so we need a way to tell the link highlight to
> +        // re-check its layer affiliation.

this comment is still out of date. we don't have a global copy of m_linkHighlight, we have one per WebViewImpl.

I think putting this code in updateLayerIsDrawable() is super sketchy - you're relying on this being called whenever we enter the tree which is true currently, but not necessarily always going to be the case. I think with the other fixes you can get rid of this now.

> Source/WebKit/chromium/tests/data/test_touch_link_highlight.html:1
> +<html>

<!DOCTYPE html> first, please. all our tests should be in standards mode unless they are testing quirks behavior

> LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-inner.html:7
> +<iframe id="nestingFrame" src="resources/1-nested-frame-noncomposited.html" style="position : relative; left : 10px; top : 10px; width : 450px; height : 150px;"></iframe>

nit: no space between the property name and the :. i.e. "position: relative" not "position : relative"

> LayoutTests/platform/chromium-linux/compositing/gestures/resources/1-frame-noncomposited.html:1
> +<html>

<!DOCTYPE html> on these too

> LayoutTests/platform/chromium/TestExpectations:3510
> +BUGWK94357 LINUX : platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-scroll-clip.html = IMAGE

why are these marked as expected failures? you've added baselines that look approximately correct to me. we won't be able to tell if they regress if they are marked as expected failures.

if they aren't right, why are you checking in baselines at all?

[W. James MacLean]

Created attachment 159949 [details]
Patch for landing

[W. James MacLean]

(In reply to comment #91)
> (From update of attachment 159477 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=159477&action=review
> 
> R=me
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:126
> > +        // If we have an active link highlight, it may need to transfer to a new GraphicsLayer,
> > +        // so we need to give it a chance to update its layer affiliation.
> > +        m_linkHighlight->invalidate();
> 
> this comment is misleading - the linkHighlight doesn't update its layer affiliation in the invalidate() call.

True, removed.

> can you just make clearCurrent...() set m_geometryNeedsUpdate on the LinkHighlight? if the GraphicsLayerChromium the highlight is attached to goes away then it definitely needs to find a new owner

Done.

> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:800
> > +        // This is the only place where the global copy of the LinkHighlightClient pointer
> > +        // is dereferenced. GraphicsLayerChromium::updateLayerIsDrawable() may be called before
> > +        // this layer is added into the tree, so we need a way to tell the link highlight to
> > +        // re-check its layer affiliation.
> 
> this comment is still out of date. we don't have a global copy of m_linkHighlight, we have one per WebViewImpl.

Removed.

> I think putting this code in updateLayerIsDrawable() is super sketchy - you're relying on this being called whenever we enter the tree which is true currently, but not necessarily always going to be the case. I think with the other fixes you can get rid of this now.

You're right that we no longer need to rely on an invalidation from updateLayerIsDrawable() to detect when a new layer is created during promotion, but our strategy has also been that anytime m_layer().invalidate() is called in GraphicsLayerChromium, we also invalidate the link highlight should there be one.

In previous version of this CL (the ones with the static pointer to the link highlight), this case was special in that it was the only one to invalidate the highlight even if this layer was not the owner, and that notion is certainly gone now.

I'm going to leave this in for now, but if we can determine that the m_layer.invalidate() that occurs here never has relevance to the highlight, we can remove this highlight invalidation later.

> > Source/WebKit/chromium/tests/data/test_touch_link_highlight.html:1
> > +<html>
> 
> <!DOCTYPE html> first, please. all our tests should be in standards mode unless they are testing quirks behavior

Done.

> > LayoutTests/platform/chromium-linux/compositing/gestures/gesture-tapHighlight-2-iframe-scrolled-inner.html:7
> > +<iframe id="nestingFrame" src="resources/1-nested-frame-noncomposited.html" style="position : relative; left : 10px; top : 10px; width : 450px; height : 150px;"></iframe>
> 
> nit: no space between the property name and the :. i.e. "position: relative" not "position : relative"

Done.

> > LayoutTests/platform/chromium-linux/compositing/gestures/resources/1-frame-noncomposited.html:1
> > +<html>
> 
> <!DOCTYPE html> on these too

Done.

> > LayoutTests/platform/chromium/TestExpectations:3510
> > +BUGWK94357 LINUX : platform/chromium-linux/compositing/gestures/gesture-tapHighlight-1-overflow-div-scroll-clip.html = IMAGE
> 
> why are these marked as expected failures? you've added baselines that look approximately correct to me. we won't be able to tell if they regress if they are marked as expected failures.
> 
> if they aren't right, why are you checking in baselines at all?

These are marked as expected failures because, although the baseline results are correct, the current LinkHighlight code does not produce the expected result in this case. The idea is based on your earlier suggestion that we have tests for the cases we know we aren't able to handle yet, and have them linked to bugs.

It is my intention that there will quickly be two follow-on CLs, one for each of the LinkHIghlight bugs listed in TestExpectations, and that part of checking in those CLs will be to remove the corresponding lines from TestExpectations.

For now, I'll leave out any tests that have TextExpectation entries, and we'll just make them added-tests when the corresponding CL lands.

[WebKit Review Bot]

Comment on attachment 159949 [details]
Patch for landing

Clearing flags on attachment: 159949

Committed r126319: <http://trac.webkit.org/changeset/126319>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kenneth Russell]

Reverted r126319 for reason:

Broke Chromium Mac build

Committed r126323: <http://trac.webkit.org/changeset/126323>

[Kenneth Russell]

Example build break:

http://build.chromium.org/p/chromium.webkit/builders/Webkit%20Mac%20Builder/builds/29857

I suspect it would break any Clang based build (e.g. Chromium Linux too).

[W. James MacLean]

Created attachment 159992 [details]
Patch

[W. James MacLean]

Comment on attachment 159992 [details]
Patch

Fixed Clang compile error, plus suppressed errors for missing platform/chromium-linux tests on MAC WIN

[WebKit Review Bot]

Comment on attachment 159992 [details]
Patch

Clearing flags on attachment: 159992

Committed r126345: <http://trac.webkit.org/changeset/126345>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.