[chromium] Allow scrolling non-root layers in the compositor thread

• Component: UI Events
• Status: RESOLVED FIXED

[Sami Kyostila]

We should allow scrolling non-root layers directly in the compositor thread to avoid having to fall back to the main thread.

[Sami Kyostila]

Created attachment 117024 [details]
Work-in-progress patch

Added work-in-progress patch that implements scrolling non-root layers. Note that this requires: https://bugs.webkit.org/show_bug.cgi?id=73345

Notable TODOs:
 - Communicate layer scroll geometry to the compositor without going through GraphicsLayer.
 - Make it possible to look up layers by id in WebViewImpl without having to walk the entire tree.
 - Process page scale changes properly.

[Sami Kyostila]

Created attachment 117245 [details]
Work-in-progress patch

Changes:
 - Scrollable layers are now registered through ChromeClient and nothing needs to be added to GraphicsLayer.
 - WebViewImpl is now also informed about scrollable layers, so it can efficiently apply the scroll commits.

TODO:
 - Split patch into Chromium and WebCore part.
 - Page scale support -- I'll revisit this in a different patch.

[Sami Kyostila]

Created attachment 117425 [details]
WebCore patch

[Sami Kyostila]

Created attachment 117426 [details]
Chromium patch

[Adrienne Walker]

Comment on attachment 117426 [details]
Chromium patch

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

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:302
> +    layer->setMaxScrollPosition(m_maxScrollPosition);

How does this interact with CCLayerTreeHostImpl::updateMaxScrollPosition calculating and overwriting the max scroll independently on the impl thread, taking page scale into account? This patch probably needs a test where page scale delta != 1.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:313
> +    if (!layerImpl->bounds().isEmpty()) {
> +        IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(windowPoint));
> +        if (!IntRect(IntPoint::zero(), layerImpl->bounds()).contains(contentPoint))
> +            return 0;
> +    }

Maybe I'm misreading this, but if a layer has valid bounds but the point is not contained, then all of its children are skipped? The layer tree is not a bounding box hierarchy, so I think you still need to check the children.  Can you add a test for this case?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:350
> +        // Reset the pending delta completely to zero if the layer was able to move in that direction. This is
> +        // to ensure it is possible to scroll exactly to the beginning or end of the scroll area regardless of
> +        // the scroll step.

Does this cause any hitch when flinging?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:440
> -    bool didMove = m_scrollLayerImpl && (!m_scrollLayerImpl->scrollDelta().isZero() || m_pageScaleDelta != 1.0f);
> -    if (!didMove || m_pinchGestureActive) {
> +    if (m_pinchGestureActive || !collectScrollDeltas(scrollInfo.get(), m_rootLayerImpl.get())) {

Can you explain why you dropped the m_pageScaleDelta check here? I think that's needed so that pinch zooms are sent back even when there are no scrolls.

[Sami Kyostila]

Comment on attachment 117426 [details]
Chromium patch

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

>> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:302
>> +    layer->setMaxScrollPosition(m_maxScrollPosition);
> 
> How does this interact with CCLayerTreeHostImpl::updateMaxScrollPosition calculating and overwriting the max scroll independently on the impl thread, taking page scale into account? This patch probably needs a test where page scale delta != 1.

Agreed, the coverage for page scaling isn't nearly as good as I'd like it to be, mainly because of the difficulty in testing that feature interactively. I considered adding some rudimentary UI for page scaling for this purpose, but did not get that far yet.

I think I'll need to do another pass with a focus on page scaling interactions. I'll keep your comment in mind while doing that.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:313
>> +    }
> 
> Maybe I'm misreading this, but if a layer has valid bounds but the point is not contained, then all of its children are skipped? The layer tree is not a bounding box hierarchy, so I think you still need to check the children.  Can you add a test for this case?

Good catch. I thought that layers were bounded by their parents but clearly that isn't the case. I've changed this to always recurse through the children and check the content point against layerImpl->visibleLayerRect() since that also takes scrolling into account. I'll make sure this is tested as well.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:350
>> +        // the scroll step.
> 
> Does this cause any hitch when flinging?

I doesn't, because the fling range is limited so that the flung element comes to rest exactly at the edge of its scroll area. The fling should not affect the parent element -- unless the element was already flush against the scroll area edge when the gesture began, in which case we fling one of its parents instead.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:440
>> +    if (m_pinchGestureActive || !collectScrollDeltas(scrollInfo.get(), m_rootLayerImpl.get())) {
> 
> Can you explain why you dropped the m_pageScaleDelta check here? I think that's needed so that pinch zooms are sent back even when there are no scrolls.

Well spotted, that was my mistake. I'll make sure there's a test for this as well.

[Sami Kyostila]

Created attachment 118602 [details]
Chromium patch

- Added support and tests for page scaling.

[WebKit Review Bot]

Please wait for approval from [e-mail] before submitting because this patch contains changes to the Chromium public API.

[Darin Fisher (:fishd, Google)]

Comment on attachment 118602 [details]
Chromium patch

I defer to jamesr for the WebKit API change.

[James Robinson]

Comment on attachment 118602 [details]
Chromium patch

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

I haven't looked through everything in detail, but have found some issues.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:170
>      void scrollBy(const IntSize& scroll);
> +    void scrollBy(const FloatSize& scroll);

it's very rarely a good idea to override a function like this with such similar types when the implementations do fairly different things. what's the story?

do we actually intend to support scrolling by non-integer amounts?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:245
> +    // Tracks if this layer or a descendant was scrolled since the last sync.
> +    bool m_subtreeWasScrolled;

if i understand correctly, this is an optimization to avoid having to walk the full layer tree every commit to check for scrolls on sublayers, correct? would this be better done as a bit on the LTHI? this seems somewhat overdone for a small win

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:262
> +    FloatSize m_scrollDeltaResidue;

what is the 'scroll delta residue'?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:485
> +    for (size_t i = 0; i < info.scrolls.size(); ++i) {
> +        if (m_rootScrollLayer && info.scrolls[i].layerId == m_rootScrollLayer->id())
> +            m_client->applyRootLayerScrollAndScale(info.scrolls[i].scrollDelta, info.pageScaleDelta);

this logic makes me wonder if trying to be generic about scroll on the root layer is a mistake and if we should instead keep the root scroll + page scale delta separate from scrolls on sublayers

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:335
> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingInnermostLayerAtPoint(CCLayerImpl* layerImpl, const IntPoint& windowPoint)

what does "window" mean in "window point"? we don't have a defined notion of a window in the compositor today. viewport, perhaps? is this scaled or unscaled?

> Source/WebKit/chromium/public/platform/WebLayerTreeViewClient.h:48
> +    // Applies a scroll delta to a non-root layer. This is triggered by events
> +    // sent to the compositor thread through the WebCompositor interface.
> +    virtual void applyLayerScroll(int layerId, const WebSize& scrollDelta) = 0;

I don't think this makes sense on this interface - why would you talk about a sublayer through the WebLayerTreeViewClient instead of via that layer? How would you expect this API to be used?

We also do not (and should not) expose layer IDs via the public API.

[Alexandre Elias]

I think instead of introducing m_scrollDeltaResidue, we should simply change m_scrollDelta and m_sentScrollDelta to be FloatSize, and make all operations on it likewise float-based.  I was also considering doing this, and this would also slightly improve the accuracy of pinch gestures.  We can round to int when sending down to the non-Impl side.

[Sami Kyostila]

I'd also be in favor of making the scroll offset float. It might lead to some small snapping artifacts when we sync with to the main thread and get integer offsets back, but all in all it would simplify this code quite a bit.

[Sami Kyostila]

Comment on attachment 118602 [details]
Chromium patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:170
>> +    void scrollBy(const FloatSize& scroll);
> 
> it's very rarely a good idea to override a function like this with such similar types when the implementations do fairly different things. what's the story?
> 
> do we actually intend to support scrolling by non-integer amounts?

Based on offline discussion I'll change the layer scroll coordinates to be floating point instead.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:245
>> +    bool m_subtreeWasScrolled;
> 
> if i understand correctly, this is an optimization to avoid having to walk the full layer tree every commit to check for scrolls on sublayers, correct? would this be better done as a bit on the LTHI? this seems somewhat overdone for a small win

Yes, that's the idea. The problem with having the bit in the LTHI is that CCLayerImpl doesn't have a reference back to it (perhaps rightly so), so scrollBy() cannot go an flip that bit.

Perhaps this is a premature optimization, so I'll drop it for now and scan the whole layer tree instead.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:262
>> +    FloatSize m_scrollDeltaResidue;
> 
> what is the 'scroll delta residue'?

It is the fractional part of the scroll offset, but with floating point coordinates I can get rid of it.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:485
>> +            m_client->applyRootLayerScrollAndScale(info.scrolls[i].scrollDelta, info.pageScaleDelta);
> 
> this logic makes me wonder if trying to be generic about scroll on the root layer is a mistake and if we should instead keep the root scroll + page scale delta separate from scrolls on sublayers

I see what you mean. For the root layer the scroll offset and the page scale delta are tightly coupled, whereas the sublayers only care about scroll offset. I'll try to separate the two concepts.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:335
>> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingInnermostLayerAtPoint(CCLayerImpl* layerImpl, const IntPoint& windowPoint)
> 
> what does "window" mean in "window point"? we don't have a defined notion of a window in the compositor today. viewport, perhaps? is this scaled or unscaled?

It refers to the target space of CCLAyerImpl::screenSpaceTransform(). View coordinate sounds good to me. The page scale does not affect view coordinates, but you have to apply the inverse of the page scale when going from view to content coordinates.

>> Source/WebKit/chromium/public/platform/WebLayerTreeViewClient.h:48
>> +    virtual void applyLayerScroll(int layerId, const WebSize& scrollDelta) = 0;
> 
> I don't think this makes sense on this interface - why would you talk about a sublayer through the WebLayerTreeViewClient instead of via that layer? How would you expect this API to be used?
> 
> We also do not (and should not) expose layer IDs via the public API.

Ah, I guess I picked the wrong path to feed back the scroll offsets to the main thread. I'll rework this use the individual layers instead.

[Sami Kyostila]

Created attachment 119032 [details]
Chromium patch

- CCLayerImpl scroll delta is now floating point.
- Separated root and sublayer scroll deltas.
- Don't route sublayer scroll deltas back through WebViewImpl.
- Use viewport coordinates instead of window coordinates.

[WebKit Review Bot]

Attachment 119032 [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/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:980:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1028:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:132:  The parameter name "layerImpl" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:426:  Semicolon defining empty statement for this loop. Use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:486:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 5 in 15 files


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

[Sami Kyostila]

Created attachment 119036 [details]
Chromium patch

- Fixed style issues.

[WebKit Review Bot]

Attachment 119036 [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/WebKit/chromium/tests/CCLayerTreeHostTest.cpp:1027:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 15 files


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

[James Robinson]

Are y'all using webkit-patch upload? That will tell you about style errors before you upload and save a lot of round-trips.

[Sami Kyostila]

Created attachment 119050 [details]
Patch

[Sami Kyostila]

Ah, I somehow missed the fact that webkit-patch also works with git. Sorry for the noise.

[Sami Kyostila]

Created attachment 119065 [details]
Patch

aelias pointed out that the scrolling logic can be simplified now that the coordinates are floating point.

[Alexandre Elias]

Looking good, thanks.  One final concern: we should make sure that m_currentlyScrollingLayerImpl doesn't get into a bad state.

1. I'd suggest adding m_currentlyScrollingLayerImpl.clear(); to the beginning of scrollBegin().  That way we're more robust to bugs in the caller like forgetting to call ScrollEnd.
2. It could be that m_currentlyScrollingLayerImpl goes away during a commit.  I guess we could validate that it's still present in the tree in commitComplete()?

[Sami Kyostila]

Created attachment 119285 [details]
Patch

- Check that the currently scrolling layer still exists after a commit.
- Always clear the currently scrolling layer in scrollBegin().
- Apply the page scale to the layer visible rect instead of the content point.
- Don't collect scroll deltas if pinch zooming or a page scale animation is in progress.

[James Robinson]

Comment on attachment 119285 [details]
Patch

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

R- for several layering violations. I think a good place to start for this is to think about where the interface boundaries lie between the compositor and the thing that's scrolling, then go from there to figure out the proper interface boundaries.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:39
> +#include "Node.h"

no no no, you can't depend on Node in here. This is in WebCore/platform so it can't depend on anything in WebCore outside of WebCore/platform.

are you sure you want to be talking about Nodes here anyway? there are things that aren't Nodes that need to scroll. are you perhaps looking for ScrollableArea? the fact that you are taking an actual reference here really scares me

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:32
> +#include "RenderBox.h"

you can't use code from WebCore/rendering/ here (nor do i think you want to)

[Sami Kyostila]

Created attachment 119683 [details]
Patch

- Reworked scroll delta feedback by extending CCLayerDelegate.

[James Robinson]

Comment on attachment 119683 [details]
Patch

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

High level design looks good to me. Still going through the details, but here's some initial easy-to-resolve feedback.

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:699
> +        m_scrollableArea->scroll(scrollDelta.width() > 0 ? ScrollRight : ScrollLeft, ScrollByPixel, abs(scrollDelta.width()));
> +    if (scrollDelta.height())
> +        m_scrollableArea->scroll(scrollDelta.height() > 0 ? ScrollDown : ScrollUp, ScrollByPixel, abs(scrollDelta.height()));

I'm pretty sure you need to use the ..WithoutAnimation() variants here or WebKit will attempt to apply animation here, which doesn't make sense for scrolls initiated from the impl thread (which we've presumably already animated in the appropriate way)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:32
> +#include "RenderBox.h"

you don't actually need this include any more, do you? it's a layering violation

> Source/WebKit/chromium/src/ChromeClientImpl.h:36
> +#include "GraphicsLayer.h"

can GraphicsLayer be forward declared?

> Source/WebKit/chromium/src/ChromeClientImpl.h:44
> +class Node;

don't think you need this

[Sami Kyostila]

(In reply to comment #26)
> (From update of attachment 119683 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119683&action=review
> 
> High level design looks good to me. Still going through the details, but here's some initial easy-to-resolve feedback.
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.cpp:699
> > +        m_scrollableArea->scroll(scrollDelta.width() > 0 ? ScrollRight : ScrollLeft, ScrollByPixel, abs(scrollDelta.width()));
> > +    if (scrollDelta.height())
> > +        m_scrollableArea->scroll(scrollDelta.height() > 0 ? ScrollDown : ScrollUp, ScrollByPixel, abs(scrollDelta.height()));
> 
> I'm pretty sure you need to use the ..WithoutAnimation() variants here or WebKit will attempt to apply animation here, which doesn't make sense for scrolls initiated from the impl thread (which we've presumably already animated in the appropriate way)

Good point, scrollToOffsetWithoutAnimation() sounds like the right thing to do here.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:32
> > +#include "RenderBox.h"
> 
> you don't actually need this include any more, do you? it's a layering violation

No I don't. This was left over by mistake.
 
> > Source/WebKit/chromium/src/ChromeClientImpl.h:36
> > +#include "GraphicsLayer.h"
> 
> can GraphicsLayer be forward declared?

Indeed it can. I was using PlatformLayer here before which is a typedef and cannot be forward declared.

> > Source/WebKit/chromium/src/ChromeClientImpl.h:44
> > +class Node;
> 
> don't think you need this

Done.

[Sami Kyostila]

Created attachment 119898 [details]
Patch

- Use scrollToOffsetWithout animation to avoid WebKit-side scroll animations.
- Remove useless includes.

[James Robinson]

Comment on attachment 119898 [details]
Patch

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

R=me, but I'd appreciate it if you let aelias@ take a pass over the page scale apply/unapply logic to make sure it's all consistent.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:626
> +    bool didMove = m_scrollLayerImpl && (didScrollSubtree(m_scrollLayerImpl.get()) || m_pageScaleDelta != 1.0f);

webkit style nit: the comparison should be written as "m_pageScaleDelta != 1", without the ".0f" qualifier.

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:68
> +#include "RenderLayer.h"

pretty sure you don't need this #include any more now either (it isn't wrong in terms of code layering, just unnecessary)

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:172
> +    RefPtr<CCLayerImpl> root = CCLayerImpl::create(0);
> +    root->setScrollable(false);
> +    RefPtr<CCLayerImpl> child = CCLayerImpl::create(0);

nit: i think it's generally a bad idea to construct CCLayerImpls with overlapping IDs in tests since we maintain the invariant that we never reuse IDs in the normal code.

[James Robinson]

Comment on attachment 119898 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:477
> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingInnermostLayerAtPoint(CCLayerImpl* layerImpl, const IntPoint& viewportPoint)

sorry to reverse myself earlier here but now that I think about this more I don't think this is right. just finding the first most-nested layer is not going to find what the user really wants, especially if the tree order doesn't match the z-order very well

instead of iterating through the layer tree in tree order, you really need to walk in z-order here by going through the CCRenderSurface list

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:488
> +    IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint));

if the layer's screenSpaceTransform is not invertible, inverse() will return an identity transform which will probably _not_ do what you expect here. if the screenSpaceTransform is not invertible we probably do not want to let it scroll, since that means that some dimension got mapped to zero and the layer isn't actually visible

also do you need to consider layer backfaces here?

[Alexandre Elias]

LGTM on the page scale logic.

[Sami Kyostila]

Created attachment 120063 [details]
Patch

- Find scrollable layer in reverse painting order.

[James Robinson]

Lots of merge conflicts on this, would you mind rebasing?

[Sami Kyostila]

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

Thanks for taking a look. The layer walking order should indeed be corrected since the user may get some unexpected results. The latest version of the patch fixes this.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:477
>> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingInnermostLayerAtPoint(CCLayerImpl* layerImpl, const IntPoint& viewportPoint)
> 
> sorry to reverse myself earlier here but now that I think about this more I don't think this is right. just finding the first most-nested layer is not going to find what the user really wants, especially if the tree order doesn't match the z-order very well
> 
> instead of iterating through the layer tree in tree order, you really need to walk in z-order here by going through the CCRenderSurface list

Good catch. I'll rework this to walk the layers in reverse z-order so we scroll what the user actually sees.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:488
>> +    IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint));
> 
> if the layer's screenSpaceTransform is not invertible, inverse() will return an identity transform which will probably _not_ do what you expect here. if the screenSpaceTransform is not invertible we probably do not want to let it scroll, since that means that some dimension got mapped to zero and the layer isn't actually visible
> 
> also do you need to consider layer backfaces here?

I've added a check against non-invertible transforms. The latest revision of the patch is walking the list of render surface layers which does not include single sided backfacing layers. Double sided backfacing layers I think we should just scroll normally.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:626
>> +    bool didMove = m_scrollLayerImpl && (didScrollSubtree(m_scrollLayerImpl.get()) || m_pageScaleDelta != 1.0f);
> 
> webkit style nit: the comparison should be written as "m_pageScaleDelta != 1", without the ".0f" qualifier.

Done.

>> Source/WebKit/chromium/src/ChromeClientImpl.cpp:68
>> +#include "RenderLayer.h"
> 
> pretty sure you don't need this #include any more now either (it isn't wrong in terms of code layering, just unnecessary)

Done.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:172
>> +    RefPtr<CCLayerImpl> child = CCLayerImpl::create(0);
> 
> nit: i think it's generally a bad idea to construct CCLayerImpls with overlapping IDs in tests since we maintain the invariant that we never reuse IDs in the normal code.

Looks like I confused the ID with LayerChromium's delegate pointer. I'll replace these with unique numbers.

[Sami Kyostila]

Created attachment 120067 [details]
Patch

 - Rebased.

[James Robinson]

Comment on attachment 120067 [details]
Patch

Looks good, R=me.

This looks like a really good use case for the layer tree iterators underway here: https://bugs.webkit.org/show_bug.cgi?id=74203.  Sami, when you get a chance could you add some unit tests for the hit testing order in various cases (no render surfaces, render surfaces that overlap, backface of single-sided, etc) so that we can transition the collect*Layers() logic over to using iterators with confidence that we aren't regressing behavior?

Set cq? when this is ready to land and myself or any committer can mark it cq+.  I'm not sure if there are conflicts or overlaps with other patches, so I'm not setting it now.

[James Robinson]

+cc Dana as this looks like some code that could use these iterators: https://bugs.webkit.org/show_bug.cgi?id=74203

[Sami Kyostila]

Created attachment 121696 [details]
Patch

Thanks James. Here's a rebased patch with the following additional changes:
- Move added methods in ChromeClientImpl.h inside USE(ACCELERATED_COMPOSITING)
- LayerChromium::setMaxScrollPosition() now triggers commit
- Reset currently scrolling layer if the root layer goes away in CCLayerTreeHostImpl::setRootLayer()

I'll follow up with more test coverage for the cases you mentioned in a separate bug.

[WebKit Review Bot]

Comment on attachment 121696 [details]
Patch

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

[Sami Kyostila]

Created attachment 121861 [details]
Patch

 - Fixed EWS build failure.

[Sami Kyostila]

Created attachment 122041 [details]
Patch

 - Rebased.

[Sami Kyostila]

Created attachment 122229 [details]
Patch

 - Rebased.
 - Check whether scrollable layer still exists in CCLayerTreeHostImpl::setRootLayer() instead as walking up CCLayerImpl::parent() pointers is not safe.

[James Robinson]

Comment on attachment 122229 [details]
Patch

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

Overall I think this is nearly ready to go. I think we need to wait for https://bugs.webkit.org/show_bug.cgi?id=76663 to land to clean up the delegate stuff, and I'm hoping that the layer iterators can clean up some of the iteration logic. Otherwise we should be good to go.

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:63
> +    virtual void scrollBy(const IntSize& scrollDelta) = 0;

I'd really prefer that you not grow the interface required for every LayerChromium.  In https://bugs.webkit.org/show_bug.cgi?id=76663 i'm getting rid of CCLayerDelegate and moving the paintContents() down to ContentLayerChromium. I think you can do the same thing with scrollBy(), the only layers that we actually support scrolling are GraphicsLayerChromium::m_layer's, right? That way you won't have to provide empty implementations in so many classes (like WebLayerImpl) where scrollBy() makes no sense

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:141
> +    static void collectLayersInPaintOrder(CCLayerList&, CCRenderSurface*);
> +    static void collectNonDrawingScrollableLayers(CCLayerList&, CCLayerImpl*);
> +    static void collectAllLayers(CCLayerList&, CCLayerImpl*);

now that layer iterators have landed (http://trac.webkit.org/changeset/104626) can we use those instead of adding a set of new utility functions?

if we do need these, please separate the static functions from the member functions and add some documentation of what they do

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:877
> +    PlatformLayer* platformLayer = layer->platformLayer();

since we're in chromium specific code here the PlatformLayer typedef is unnecessary abstraction here - just use LayerChromium here

> Source/WebKit/chromium/src/ChromeClientImpl.cpp:888
> +    PlatformLayer* platformLayer = layer->platformLayer();

same here re: using LayerChromium

[Sami Kyostila]

Created attachment 124395 [details]
Patch

 - Remove custom iterators in favor of layer iterators.
 - Move scrollBy() from LayerChromium to ContentLayerChromium.
 - Use LayerChromium instead of PlatformLayer in ChromeClientImpl.
 - The inverse screen space transform for a non-composited content layer does not introduce page scale, so no need to undo it in isContentPointWithinLayer().
 - Add new tests for hit testing against a backfacing layer and a normal or non-composited layer with page scaling.

[Sami Kyostila]

Created attachment 125653 [details]
Patch

- Rebased.
- Scroll non-composited content layer via the outer scroll layer like before. The latter is not drawn so the iterator was skipping it. Added test case.

[Sami Kyostila]

Created attachment 126307 [details]
Patch

- Cosmetic changes based on downstream review -- mainly renamed ContentLayerDelegate::scrollBy() => wasScrolled()

[James Robinson]

Comment on attachment 126307 [details]
Patch

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

I like this patch and would like to see it move forward, but I think we'll want to adjust the ScrollableArea-related hookups a bit (see https://bugs.webkit.org/show_bug.cgi?id=78401).  Would you mind splitting those bits out of this patch?

Also it seems that we're going to need to propagate more information about the scroll down through the CCInputHandler if we want to be maximally efficient - for instance, if one layer in the page listens to wheel events we need to know that we can't accept scrolls if they are initiated by mouse wheel events *and* we hit test in that layer.  We can address this separately if we need to.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:50
> +    virtual void wasScrolled(const IntSize&) = 0;

our naming convention for these sorts of calls is either "willXXX" or "didXXX" - think this is a "didXXX"

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:622
> +            static_cast<ContentLayerChromium*>(layer)->scrollBy(info.scrolls[i].scrollDelta);

rather than downcast can we make this virtual on LayerChromium with a no-op impl for the base class?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:203
> +    // During testing we may not have an active renderer.
> +    const int kDefaultMaxTextureSize = 256;

i would much rather fix tests rather than adding production code to deal with this case. it's very easy to instantiate a real LRC in unit tests today. if that's not an option in this case then we could do something like mock out capabilities

[Sami Kyostila]

(In reply to comment #47)
> (From update of attachment 126307 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=126307&action=review
> 
> I like this patch and would like to see it move forward, but I think we'll want to adjust the ScrollableArea-related hookups a bit (see https://bugs.webkit.org/show_bug.cgi?id=78401).  Would you mind splitting those bits out of this patch?

Yeah, we'll need to integrate that with the scroll coordinator.
 
> Also it seems that we're going to need to propagate more information about the scroll down through the CCInputHandler if we want to be maximally efficient - for instance, if one layer in the page listens to wheel events we need to know that we can't accept scrolls if they are initiated by mouse wheel events *and* we hit test in that layer.  We can address this separately if we need to.

This sounds like it could be folded into 74196.

> > Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:50
> > +    virtual void wasScrolled(const IntSize&) = 0;
> 
> our naming convention for these sorts of calls is either "willXXX" or "didXXX" - think this is a "didXXX"

Done.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:622
> > +            static_cast<ContentLayerChromium*>(layer)->scrollBy(info.scrolls[i].scrollDelta);
> 
> rather than downcast can we make this virtual on LayerChromium with a no-op impl for the base class?

Yes, that's much safer.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:203
> > +    // During testing we may not have an active renderer.
> > +    const int kDefaultMaxTextureSize = 256;
> 
> i would much rather fix tests rather than adding production code to deal with this case. it's very easy to instantiate a real LRC in unit tests today. if that's not an option in this case then we could do something like mock out capabilities

Agreed. Turns out it's not very difficult to have an LRC in all the relevant tests.

[Sami Kyostila]

Created attachment 127318 [details]
Patch

[James Robinson]

Comment on attachment 127318 [details]
Patch

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

I think this looks pretty great.  I think that we might want to tweak the way we sync changes back from LayerChromium->GraphicsLayer->beyond, but I think we should move forward with the rest of this code first.

I would suggest that you first break out the Int->Float change for scrollDeltas into a separate patch, since I think that's pretty separable and would be generally pretty useful, and take out the GraphicsLayerChromium stuff from this patch (just leave a stub for didScroll).

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:62
> +    bool isLayerInDescendants(int layerId) const;

this doesn't appear to consider mask and reflection layers at all. for scrolling, i believe that's right, but as a generic function this might surprised some callers. can you update the function name and/or documentation (ideally both) describing exactly what this does?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.h:58
> +    IntSize rootScrollDelta;

why does this have to be separate and not simply an entry in 'scrolls'?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:191
> +void CCLayerTreeHostImpl::calculateRenderSurfaces(CCLayerList& renderSurfaceLayerList)

this looks like a bad merge - did you mean to change this?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:478
> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingLayer(CCLayerImpl* layerImpl, const IntPoint& viewportPoint)

it feels like most of this belongs on CCLayerImpl and the host can take care of setting m_currentlyScrollingLayerImpl if appropriate.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:507
> +    CCLayerList renderSurfaceLayerList;
> +    calculateRenderSurfaces(renderSurfaceLayerList);

yuck yuck! we should definitely not need to recalculate all our transforms, etc on every scrollBegin. Just use the transforms from the most recent frame.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:513
> +        // A non-composited content layer should be scrolled via the root scroll layer.
> +        if (layerImpl->isNonCompositedContent())

this feels fishy. in general, we need to support scrolling a layer that isn't the content in order to be able to scroll any FrameViews, not just the NCCH.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:538
> +            // Since scrollDelta is in window coordinates, it already has the page scale applied.
> +            // This matches what the root scroll layer expects, but child layers are scrolled using
> +            // unscaled content coordinates instead, so we have to undo the scaling for them. The
> +            // page scale delta needs to be unapplied with both layer types since the scroll
> +            // coordinates do not respect it.

this is really hard to maintain. it's a bit beyond this patch, but can we normalize the scales somewhere else (LayerChromium? GraphicsLayerChromium?) and not have to deal with it throughout the compositor code?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:642
> +    bool didMove = m_scrollLayerImpl && (didScrollSubtree(m_scrollLayerImpl.get()) || m_pageScaleDelta != 1);

we're doing a recursive walk through layers here to check if anything has a scrollDelta...

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:648
> +    collectScrollDeltas(scrollInfo.get(), m_scrollLayerImpl.get());

and then we do another walk here. seems unnecessary - can't we just do one walk and if we don't have any scroll deltas just send an empty scrollInfo over?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:179
> +    initializeLayerRenderer();

would you mind converting the other CCLayerTreeHostImplTest tests that initialize the layer renderer to using this helper as well?

[Sami Kyostila]

Comment on attachment 127318 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.h:62
>> +    bool isLayerInDescendants(int layerId) const;
> 
> this doesn't appear to consider mask and reflection layers at all. for scrolling, i believe that's right, but as a generic function this might surprised some callers. can you update the function name and/or documentation (ideally both) describing exactly what this does?

Right. I've now made the function consider mask and replicated layers, renamed it function and added some documentation.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostCommon.h:58
>> +    IntSize rootScrollDelta;
> 
> why does this have to be separate and not simply an entry in 'scrolls'?

This used to be separate because of how the root scroll and page scale deltas were paired, but now all the scroll deltas are in 'scrolls'.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:191
>> +void CCLayerTreeHostImpl::calculateRenderSurfaces(CCLayerList& renderSurfaceLayerList)
> 
> this looks like a bad merge - did you mean to change this?

It was intentional in the old version of the patch where scrollBegin() recalculated the layer geometry, but it's not needed any longer.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:478
>> +CCInputHandlerClient::ScrollStatus CCLayerTreeHostImpl::beginScrollingLayer(CCLayerImpl* layerImpl, const IntPoint& viewportPoint)
> 
> it feels like most of this belongs on CCLayerImpl and the host can take care of setting m_currentlyScrollingLayerImpl if appropriate.

Agreed, I've now moved this logic there.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:507
>> +    calculateRenderSurfaces(renderSurfaceLayerList);
> 
> yuck yuck! we should definitely not need to recalculate all our transforms, etc on every scrollBegin. Just use the transforms from the most recent frame.

The reason I opted for this approach earlier was that otherwise you couldn't scroll without having rendered first. From the user's point of view this makes sense since they need to see what they are pointing at, but from a scene graph point of view it felt like a weird restriction.

I've now changed the code to save a list of visible layers from the most recent frame and use that for hit testing. There's a subtle edge case where we can get input events after tree synchronization but before prepareToDraw() was called. For this reason the visible layer list uses soft references to be able to persist across tree synchronizations.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:513
>> +        if (layerImpl->isNonCompositedContent())
> 
> this feels fishy. in general, we need to support scrolling a layer that isn't the content in order to be able to scroll any FrameViews, not just the NCCH.

Yeah, I've now replaced this with a generic helper function that should cover all needed scroll/content layer configurations.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:538
>> +            // coordinates do not respect it.
> 
> this is really hard to maintain. it's a bit beyond this patch, but can we normalize the scales somewhere else (LayerChromium? GraphicsLayerChromium?) and not have to deal with it throughout the compositor code?

Yeah, this kind of code gives me nightmares. The latest revision now expects the ScrollingCoordinator interfacing code (TBD) to normalize the scroll positions and ranges so that every layer uses scaled coordinates. I've also dropped the scale fudging for layer hit detection and nonFastScrollableRegion(), so that stuff will also need to be fixed when the data is pushed to the impl side. So, the patch is no longer correct in terms of page scale, but we'll fix that in further installments.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:648
>> +    collectScrollDeltas(scrollInfo.get(), m_scrollLayerImpl.get());
> 
> and then we do another walk here. seems unnecessary - can't we just do one walk and if we don't have any scroll deltas just send an empty scrollInfo over?

I think I did it this way because collecting the scroll deltas also reset the sent deltas and that interfered with the pinch and scale animations. In the latest version we only need to do one walk since the pinch and scale animations synthesize their own scroll events.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:179
>> +    initializeLayerRenderer();
> 
> would you mind converting the other CCLayerTreeHostImplTest tests that initialize the layer renderer to using this helper as well?

This helper is now initializeLayerRendererAndDrawFrame(), and I've changed existing tests to use it where it made sense.

[Sami Kyostila]

Created attachment 134303 [details]
Patch

- Removed page scaling hacks from this patch; let's normalize the coordinates further upstream instead.
- Avoid recalculating layer transforms on scrollBegin().
- Removed some assumptions about scroll layer hierarchy.
- Avoid traversing tree twice when calculating scroll deltas.
- Removed ScrollableArea bits.
- Added several tests.

[Dana Jansens]

Comment on attachment 134303 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:628
> +    if (layer->id() == id)
> +        return layer;
> +
> +    for (size_t i = 0; i < layer->children().size(); ++i) {
> +        LayerChromium* found = findLayerById(layer->children()[i].get(), id);
> +        if (found)
> +            return found;
> +    }
> +
> +    return 0;

bikeshed: Why not just return layer->findLayerInSubtree(id)? Is this to avoid the masks/replicas of the topmost layer in the search? Or just call layer->findLayerInSubtree() directly, with an earlyout if m_rootLayer == NULL?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:656
> +    for (size_t i = 0; i < info.scrolls.size(); ++i) {
> +        LayerChromium* layer = findLayerById(m_rootLayer.get(), info.scrolls[i].layerId);
> +        if (!layer)
> +            continue;

If m_rootLayer == null, this will loop through the whole array continuing each time?

[Sami Kyostila]

Comment on attachment 134303 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:628
>> +    return 0;
> 
> bikeshed: Why not just return layer->findLayerInSubtree(id)? Is this to avoid the masks/replicas of the topmost layer in the search? Or just call layer->findLayerInSubtree() directly, with an earlyout if m_rootLayer == NULL?

findLayerInSubtree() is in CCLayerImpl, while we are searching through the LayerChromium tree here. I suppose I could've added the same helper in LayerChromium too for symmetry, but I didn't do it as this code is used in just one place. Also, the operation is O(N) and people might be more tempted to use if it was part of the class :) I can still do that if you prefer.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:656
>> +            continue;
> 
> If m_rootLayer == null, this will loop through the whole array continuing each time?

That's right. Are you suggesting an early out for that case? Usually when we do not have a root layer we should not see any scroll deltas from the impl thread either.

[Adrienne Walker]

Comment on attachment 134303 [details]
Patch

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

Can you walk me through the data flow here? Scrolling a layer sets its scroll delta.  The scroll delta is used temporarily on the impl thread to affect the position of a layer.  It's then synced back and merged into the scroll position on the main thread which...does what? For the root layer, we call out to FrameView, scroll the frame, and then update layer positions.  I think you're going to have to do the same thing here--scrolling a non-root layer on the main thread actually adjusts the layer position (GraphicsLayer::position).  You're also going to have to figure out how to get the correct maximum scroll for non-root layers and set layers as properly being scrollable.

Also, can you hook this up more so that some non-example page can be tested locally? I feel like that would help to clear this up immensely.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:147
> +void ContentLayerChromium::scrollBy(const IntSize& scrollDelta)

Only content layers get to scroll?

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:151
> +    if (m_delegate)
> +        m_delegate->didScroll(scrollDelta);

This delegate needs to do something real.

> Source/WebCore/platform/graphics/chromium/LayerChromium.cpp:389
> +void LayerChromium::setMaxScrollPosition(const IntSize& maxScrollPosition)
> +{
> +    if (m_maxScrollPosition == maxScrollPosition)
> +        return;
> +    m_maxScrollPosition = maxScrollPosition;
> +    setNeedsCommit();
> +}

You need to set this from somewhere other than tests.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:675
> +        if (layerImpl->tryScroll(viewportPoint, type) == ScrollFailed && false)

...?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:686
> +        // If any layer wants to divert the scroll event to the main thread, abort.
> +        if (status == ScrollFailed)
> +            return ScrollFailed;

I don't follow this.  If I have a scrollable child layer contained within a non-fast scrollable parent layer, this code appears to not scroll the child layer, because the parent layer will bail out? Is this because you don't want to partially scroll one layer and then have to transfer it to a non-fast layer?

[Sami Kyostila]

(In reply to comment #55)
> (From update of attachment 134303 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=134303&action=review
> 
> Can you walk me through the data flow here? Scrolling a layer sets its scroll delta.  The scroll delta is used temporarily on the impl thread to affect the position of a layer.  It's then synced back and merged into the scroll position on the main thread which...does what? For the root layer, we call out to FrameView, scroll the frame, and then update layer positions.  I think you're going to have to do the same thing here--scrolling a non-root layer on the main thread actually adjusts the layer position (GraphicsLayer::position).  You're also going to have to figure out how to get the correct maximum scroll for non-root layers and set layers as properly being scrollable.
> 
> Also, can you hook this up more so that some non-example page can be tested locally? I feel like that would help to clear this up immensely.

I'm sorry, I can see how this can be confusing. This particular patch only implements the necessary bits on the impl side to allow layer scrolling to happen. Setting up the proper layer metadata as well as feeding back the scroll deltas to the objects on the main thread is the subject of an upcoming patch, which is why those parts are stubbed out here.

Unfortunately without those bits you can't really test this stuff in practice. I'm working on the main thread patch and can give you a hacky preview if you'd like.

Comment 50 also touches on this.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:147
>> +void ContentLayerChromium::scrollBy(const IntSize& scrollDelta)
> 
> Only content layers get to scroll?

For now, yes. Please see comment 43. I admit I haven't given much though to scrolling other layer types -- do other layer types even have the concept of overflow? I guess an image layer could, but at least canvas, WebGL and video seem to get squashed to the available content box.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:675
>> +        if (layerImpl->tryScroll(viewportPoint, type) == ScrollFailed && false)
> 
> ...?

*facepalm* Gah, I've fixed this and added a test to actually exercise the condition. Apologies.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:686
>> +            return ScrollFailed;
> 
> I don't follow this.  If I have a scrollable child layer contained within a non-fast scrollable parent layer, this code appears to not scroll the child layer, because the parent layer will bail out? Is this because you don't want to partially scroll one layer and then have to transfer it to a non-fast layer?

Yes, that's right. The code in scrollBy() walks up the layer hierarchy while scrolling, and currently we can't transition from a fast scroll to a slow scroll. Now that we have the nonFastScrollableRegion logic in place, I don't think this case happens often enough to justify the effort in making the fast->slow transition work.

[Sami Kyostila]

Created attachment 134541 [details]
Patch

- Removed debugging code left in by mistake and added test (scrollBlockedByContentLayer)
- Prioritize ScrollFailed over ScrollIgnored in CCLayerImpl::tryScroll.

[Adrienne Walker]

(In reply to comment #56)
> (In reply to comment #55)
> > (From update of attachment 134303 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=134303&action=review
> 
> Unfortunately without those bits you can't really test this stuff in practice. I'm working on the main thread patch and can give you a hacky preview if you'd like.
> 
> Comment 50 also touches on this.

Ack, sorry for the noise.  I read through this giant thread again, but I missed that part.

> >> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:147
> >> +void ContentLayerChromium::scrollBy(const IntSize& scrollDelta)
> > 
> > Only content layers get to scroll?
> 
> For now, yes. Please see comment 43. I admit I haven't given much though to scrolling other layer types -- do other layer types even have the concept of overflow? I guess an image layer could, but at least canvas, WebGL and video seem to get squashed to the available content box.

I see.  This is in the context of GTFO.

> >> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:686
> >> +            return ScrollFailed;
> > 
> > I don't follow this.  If I have a scrollable child layer contained within a non-fast scrollable parent layer, this code appears to not scroll the child layer, because the parent layer will bail out? Is this because you don't want to partially scroll one layer and then have to transfer it to a non-fast layer?
> 
> Yes, that's right. The code in scrollBy() walks up the layer hierarchy while scrolling, and currently we can't transition from a fast scroll to a slow scroll. Now that we have the nonFastScrollableRegion logic in place, I don't think this case happens often enough to justify the effort in making the fast->slow transition work.

Ok, thanks for the explanation.  If it ends up blocking fast scrolls more often than we want, there can always be a follow-up patch.

(In reply to comment #57)
>
> - Prioritize ScrollFailed over ScrollIgnored in CCLayerImpl::tryScroll.

With respect to this change, can you leave some comments about the relationship between scrollable() and shouldScrollOnMainThread() in the header or in tryScroll? It seems like they are independent from looking at the code, but it's not very obvious from their names why shouldScrollOnMainThread() could be true when scrollable() is false.

[Sami Kyostila]

Created attachment 134627 [details]
Patch

- Added comment describing logic in CCLayerImpl::tryScroll.

[James Robinson]

Comment on attachment 134627 [details]
Patch

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

Sorry for the long delay in getting back to this.  Some high-level comments:

Syncing offsets back into WebCore is something to consider, but I don't think we need to do it here and I honestly don't think it'll be that difficult.  We're trying to get back to some scrollable thing.  The GraphicsLayerClient implementations are:

- NonCompositedContentHost - scrolls via WebViewImpl
- RenderLayerCompositor - associated with a FrameView, which is a ScrollableArea
- RenderLayerBacking - associated with a RenderLayer, which is a ScrollableArea

but we can deal with that later.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:614
> +static LayerChromium* findLayerById(LayerChromium* layer, int id)

why does this function skip mask/replica layers? that may make sense in the context of this caller but it's not clear from the name

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:373
> +void CCLayerTreeHostImpl::calculateVisibleLayers(CCLayerTreeHostImpl::FrameData& frame, CCLayerGeometryList& visibleLayers)

I'm not sure I understand why this intermediate representation is useful

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:520
> +    if (m_currentlyScrollingLayerImpl) {
> +        if (!m_rootLayerImpl || !m_rootLayerImpl->findLayerInSubtree(m_currentlyScrollingLayerImpl->id()))

webkit style would prefer a few early returns here instead of nested if()s. the second is a bit hard to read and the extra nesting level isn't helping much

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:721
> +    for (CCLayerImpl* layerImpl = m_currentlyScrollingLayerImpl; layerImpl && !pendingDelta.isZero(); layerImpl = layerImpl->parent()) {

For wheel scrolls we need to stop the scroll as soon as we move the target layer at all.  IOW, if we get a scrollBy(50) and the scroll target has 30px left of scrollable area, then the desired behavior is to scroll that layer by 30px and do nothing else.  This is so you can scroll content to the end of a scrollable area without moving it.

Touch is definitely different, so we need to map the scroll type into this function somehow.  Perhaps we could track the scroll type between the scrollBegin / scrollEnd calls?  The sequence for wheel is always a scrollBegin(), exactly one scrollBy(), then a scrollEnd().

we should also have test coverage for this (currently since only the root is scrollable there's no way to scroll past the end, so it hasn't come up)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:207
> +    // List of visible layers in the most recent frame in front-to-back order.

I don't understand why we are keeping this list instead of retaining the last frame's CCLayerList, since we have to calculate the latter anyway.  Why not just keep that and iterate front-to-back?

[Sami Kyostila]

Comment on attachment 134627 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:614
>> +static LayerChromium* findLayerById(LayerChromium* layer, int id)
> 
> why does this function skip mask/replica layers? that may make sense in the context of this caller but it's not clear from the name

Agreed, I'll have it walk through all the layers in the tree.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:520
>> +        if (!m_rootLayerImpl || !m_rootLayerImpl->findLayerInSubtree(m_currentlyScrollingLayerImpl->id()))
> 
> webkit style would prefer a few early returns here instead of nested if()s. the second is a bit hard to read and the extra nesting level isn't helping much

Sure, I'll clean this up.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:721
>> +    for (CCLayerImpl* layerImpl = m_currentlyScrollingLayerImpl; layerImpl && !pendingDelta.isZero(); layerImpl = layerImpl->parent()) {
> 
> For wheel scrolls we need to stop the scroll as soon as we move the target layer at all.  IOW, if we get a scrollBy(50) and the scroll target has 30px left of scrollable area, then the desired behavior is to scroll that layer by 30px and do nothing else.  This is so you can scroll content to the end of a scrollable area without moving it.
> 
> Touch is definitely different, so we need to map the scroll type into this function somehow.  Perhaps we could track the scroll type between the scrollBegin / scrollEnd calls?  The sequence for wheel is always a scrollBegin(), exactly one scrollBy(), then a scrollEnd().
> 
> we should also have test coverage for this (currently since only the root is scrollable there's no way to scroll past the end, so it hasn't come up)

Right now this function works like you described for wheel events -- assuming the wheel scrolls only on one axis. There's also a test (CCLayerTreeHostImplTest::scrollChildBeyondLimit).

As you said, touch is different in that you might have multiple scrollBys in a single scroll transaction. However, I think the snap-to-end behavior also makes sense for touch events, because it makes it easier to exactly reach the end of a scrollable region both when flinging or finger-dragging. Further calls to scrollBy() will start moving an ancestor layer, if any, so if you keep moving your finger, things will respond as you would expect.

So, I think the current code covers your requirements. I can of course make it more explicit by tracking the scroll type and having scrollBy() call scrollEnd() after moving, but that won't really change anything since a wheel event is always a single scrollBegin/By/End sequence anyway.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.h:207
>> +    // List of visible layers in the most recent frame in front-to-back order.
> 
> I don't understand why we are keeping this list instead of retaining the last frame's CCLayerList, since we have to calculate the latter anyway.  Why not just keep that and iterate front-to-back?

The reason for the list was that sometimes we get an input event right after syncing but before drawing. Syncing removes the layer tree ownership from CCLayerTreeHostImpl, so the previous render surface layer list becomes potentially invalid.

I thought of a couple of ways of solving this:

1. Require that we always draw before handling input events. Bad because of the increase to input event latency.
2. Drop input events to the floor if we haven't drawn yet. Yeah, no.
3. Keep the previous layer tree around for an extra sync cycle. Not really feasible since CCLayerImpl's would need to become refcounted again.
4. Detect this situation in scrollBegin() and calculate the layer geometry on demand. Workable, but it seems weird to have scrollBegin() call prepareToDraw().
5. Keep soft references to the previous layer tree.

I'll rework the patch to do #4 to see how it pans out, but personally I'm also happy with the soft reference approach.

[Sami Kyostila]

Created attachment 136690 [details]
Patch

 - Consider all layer types in findLayerById.
 - Clean up logic in setRootLayer().
 - Save most recent render surface layer list and walk it in scrollBegin().

[Sami Kyostila]

This patch uses the render surface layer list from the previous frame for hit testing. If we didn't draw since the last root layer reset, scrollBy() will recompute this list on demand.

Note that I chose not to reuse the list calculated by scrollBy() in prepareToDraw() to make sure the latter will always return the latest frame data. Depending on the sync/framerate ratio this can increase the amount of computation we do. I tested with a mouse wheel that the previous frame's list is reused most of the time, unless you spin the wheel really fast, causing up to 2 out of 3 input events to trigger render pass calculation. I didn't try with touch inputs.

So, this approach isn't as efficient as the soft references from the earlier patch, but it's most definitely simpler.

[Sami Kyostila]

Created attachment 137869 [details]
Patch

- Rebased.

[James Robinson]

Comment on attachment 137869 [details]
Patch

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

OK, I think we're nearly there.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:67
> +    virtual void scrollBy(const IntSize&);

OVERRIDE

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:116
> +    virtual void didScroll(const IntSize&) { }

OVERRIDE

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:574
> +static LayerChromium* findLayerById(LayerChromium* layer, int id)

why is this structured differently from CCLayerImpl::findLayerById()? why isn't it the same function just templatized?

> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:681
> +    // Scroll event is ignored because the hit layer is not facing the viewer.

shouldn't the hit test just skip the layer in this case? why would the backface of a layer cause us to eat the scroll?

[Sami Kyostila]

Comment on attachment 137869 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:67
>> +    virtual void scrollBy(const IntSize&);
> 
> OVERRIDE

Done.

>> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:116
>> +    virtual void didScroll(const IntSize&) { }
> 
> OVERRIDE

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:574
>> +static LayerChromium* findLayerById(LayerChromium* layer, int id)
> 
> why is this structured differently from CCLayerImpl::findLayerById()? why isn't it the same function just templatized?

Great idea! I've now replaced both functions with a single template.

>> Source/WebKit/chromium/tests/CCLayerTreeHostImplTest.cpp:681
>> +    // Scroll event is ignored because the hit layer is not facing the viewer.
> 
> shouldn't the hit test just skip the layer in this case? why would the backface of a layer cause us to eat the scroll?

You're right, that's what's happening but this comment was just obtuse about it. The backfacing layer will never end up in the list of hit tested layers, and since there are no other scrollable layers in this test, the scroll event is dropped.

I've made the comment more precise.

[Sami Kyostila]

Created attachment 137917 [details]
Patch

[WebKit Review Bot]

Comment on attachment 137917 [details]
Patch

Clearing flags on attachment: 137917

Committed r114651: <http://trac.webkit.org/changeset/114651>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adrienne Walker]

Reverted in http://trac.webkit.org/changeset/1146142.

[Adrienne Walker]

(In reply to comment #70)
> Reverted in http://trac.webkit.org/changeset/1146142.

Make that http://trac.webkit.org/changeset/116142.  Oops.

[Sami Kyostila]

Created attachment 146640 [details]
Patch

- Don't crash if the root layer loses its render surface after the most recent render.
- Fixed a bug in scrollBy() that always scheduled a commit when page scale delta != 1.
- Renamed ScrollFailed to ScrollOnMainThread to make its purpose clearer.
- Added a test to make sure non-scrollable layers don't block scrolling their ancestors.

[Sami Kyostila]

Created attachment 146873 [details]
Patch

- Don't reset sent scroll deltas while a pinch zoom or page scale animation is in progress.
- Added tests for the above.

[James Robinson]

Comment on attachment 146873 [details]
Patch

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

Thanks for adding all the new tests.  I think we need a little more refinement on this before landing, but it's not in bad shape.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:49
> +    virtual void didScroll(const IntSize&) = 0;

After having more time to think about it, I really think that scrolling should be a separate delegate type and hang off of the base LayerChromium class (and be optional).  Even in this patch you can see that you had to modify a number of ContentLayerDelegate implementations that aren't scrollable at all.  It's also a bit wrong that this patch only lets us scroll content layers - other types (like plugins) definitely should be scrollable as well.

> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:70
> +    virtual void scrollBy(const IntSize&) OVERRIDE;

if you move the scroll delegate up to LayerChromium, this should move up as well (and become non-virtual)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:190
> +        TRACE_EVENT("tryScroll Failed shouldScrollOnMainThread", this, 0);

please use:

TRACE_EVENT0("cc", "....");

style trace macros here and everywhere else inside cc so we get a useful component

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:397
> +    m_mostRecentRenderSurfaceLayerList = frame.renderSurfaceLayerList;

Hmmm, a deep copy on every pass?  If we're making the renderSurfaceLayerList something persistent instead of something frame-transient, can we just generate the list into a member variable of CCLayerTreeHostImpl in the first place, instead of pretending it's part of FrameData (since it's no longer isolated to being used only on this frame)?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:545
> +PassOwnPtr<CCLayerImpl> CCLayerTreeHostImpl::releaseRootLayer()
> +{
> +    m_scrollingLayerIdFromPreviousTree = m_currentlyScrollingLayerImpl ? m_currentlyScrollingLayerImpl->id() : -1;
> +    m_currentlyScrollingLayerImpl = 0;

I think it's ugly to make this seemingly normal getter have strange side effects like this.  I assume you're doing this because currently the only caller to releaseRootLayer() is in the TreeSynchronizer call, right? I think it'd be much better to have an explicit call to reset state like this or to rename this function to make it clearer what is going on.

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:714
> +    if (m_mostRecentRenderSurfaceLayerList.size() && m_rootLayerImpl->renderSurface())

I'm not sure I understand both of the checks here - why are we checking both?  If we haven't calculated a renderSurfaceLayerList I can understand that we need to calculate one, but what does the rootLayerImpl having a render surface have to do with it?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:739
> +    // First find out which layer was hit by walking the saved list of visible layers from the most recent frame.
> +    CCLayerImpl* layerImpl = 0;

this looks like a generic hit testing function - could you extract it out into one and provide dedicated tests, etc?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:745
> +    for (CCLayerIteratorType it = CCLayerIteratorType::begin(&m_mostRecentRenderSurfaceLayerList); it != end; ++it) {
> +        CCLayerImpl* renderSurfaceLayerImpl = (*it);

I think we should only care about the case where it.representsItself() == true, right? (see comments at the top of CCLayerIterator.h to see what this means).

[Sami Kyostila]

Comment on attachment 146873 [details]
Patch

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

Thanks for taking a look James.

>> Source/WebCore/platform/graphics/chromium/ContentLayerChromium.h:49
>> +    virtual void didScroll(const IntSize&) = 0;
> 
> After having more time to think about it, I really think that scrolling should be a separate delegate type and hang off of the base LayerChromium class (and be optional).  Even in this patch you can see that you had to modify a number of ContentLayerDelegate implementations that aren't scrollable at all.  It's also a bit wrong that this patch only lets us scroll content layers - other types (like plugins) definitely should be scrollable as well.

Yeah, that seems a little more orthogonal than limiting scrolling to content layers. I'll rework this part.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:190
>> +        TRACE_EVENT("tryScroll Failed shouldScrollOnMainThread", this, 0);
> 
> please use:
> 
> TRACE_EVENT0("cc", "....");
> 
> style trace macros here and everywhere else inside cc so we get a useful component

Done.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:397
>> +    m_mostRecentRenderSurfaceLayerList = frame.renderSurfaceLayerList;
> 
> Hmmm, a deep copy on every pass?  If we're making the renderSurfaceLayerList something persistent instead of something frame-transient, can we just generate the list into a member variable of CCLayerTreeHostImpl in the first place, instead of pretending it's part of FrameData (since it's no longer isolated to being used only on this frame)?

Right, we might as well just have one instance. I also like keeping the list inside CCLayerTreeHostImpl because it already owns the data in it.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:545
>> +    m_currentlyScrollingLayerImpl = 0;
> 
> I think it's ugly to make this seemingly normal getter have strange side effects like this.  I assume you're doing this because currently the only caller to releaseRootLayer() is in the TreeSynchronizer call, right? I think it'd be much better to have an explicit call to reset state like this or to rename this function to make it clearer what is going on.

I wasn't thinking about TreeSynchronizer specifically here, but rather that we need to forget everything we know about the tree once we release it. I think I'll rename this to something more obvious like detachLayerTree().

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:714
>> +    if (m_mostRecentRenderSurfaceLayerList.size() && m_rootLayerImpl->renderSurface())
> 
> I'm not sure I understand both of the checks here - why are we checking both?  If we haven't calculated a renderSurfaceLayerList I can understand that we need to calculate one, but what does the rootLayerImpl having a render surface have to do with it?

This is needed because we can end up in a situation where we have calculated the render surface layer list, but since then the root render surface has been cleared (e.g., by initializeLayerRenderer()). Since CCLayerIterator doesn't work without a root render surface, I check for that here. This was the ChromeOS crash we were seeing from this patch.

Alternatively we could empty the render surface layer list every time the render surfaces are cleared, but since that can also be done outside CCLayerTreeHostImpl I prefer to have a defensive check here.

I also considered fixing CCLayerIterator to fail gracefully, but that would mean we'd silently fail to hit anything if the root render surface is gone. Thinking about this more, I think it would be good for that case to not blow up, so I've opened bug 88886 for it.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:739
>> +    CCLayerImpl* layerImpl = 0;
> 
> this looks like a generic hit testing function - could you extract it out into one and provide dedicated tests, etc?

Good idea. I'll pull this into CCLayerTreeHostCommon.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:745
>> +        CCLayerImpl* renderSurfaceLayerImpl = (*it);
> 
> I think we should only care about the case where it.representsItself() == true, right? (see comments at the top of CCLayerIterator.h to see what this means).

That would eliminate some potentially redundant hit tests, but it would also break some cases that seem like they should work. For example, if there's just a single layer in the tree, it'll be iterated over just once with it.representsItself() == false. Granted, having just a single root layer is really only used when testing, but OTOH it would seem like a strange restriction to have.

[Shawn Singh]

Comment on attachment 146873 [details]
Patch


So I looked through this as much as I could and ended up with only one comment below.

Tonight I will pull out the hit testing loop and add unit tests, and submit it as a separate patch.  That should make Sami's life easier, and he can decide whether to assimilate that code or keep it as a separate patch.




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

>>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:745
>>> +        CCLayerImpl* renderSurfaceLayerImpl = (*it);
>> 
>> I think we should only care about the case where it.representsItself() == true, right? (see comments at the top of CCLayerIterator.h to see what this means).
> 
> That would eliminate some potentially redundant hit tests, but it would also break some cases that seem like they should work. For example, if there's just a single layer in the tree, it'll be iterated over just once with it.representsItself() == false. Granted, having just a single root layer is really only used when testing, but OTOH it would seem like a strange restriction to have.

Spoke with Dana about this.  We both feel that it.representsItself() == true would be much clearer here, also.   if a layer really does iterate only once with it.representsItself()==false, that means it was a renderSurface without drawing any content anyway.

[Shawn Singh]

Comment on attachment 146873 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:203
> +    if (!screenSpaceTransform().isInvertible()) {
> +        TRACE_EVENT("tryScroll Ignored nonInvertibleTransform", this, 0);
> +        return CCInputHandlerClient::ScrollIgnored;
> +    }
> +
> +    IntPoint contentPoint(screenSpaceTransform().inverse().mapPoint(viewportPoint));
> +    if (nonFastScrollableRegion().contains(contentPoint)) {
> +        TRACE_EVENT("tryScroll Failed nonFastScrollableRegion", this, 0);
> +        return CCInputHandlerClient::ScrollOnMainThread;
> +    }

I think sreenSpaceTransform doesn't convert between layer's content space and screen space.  If I remember correctly, it converts between layer's "layout space" (with the origin at the top-left corner) and screen space.   So there needs to be an additional part of the transform that converts from the layer's origin space to the layer's content space.  The code I'll submit for hit testing tonight should have some code that can be assimilated here.

Additionally, after assimilating that hit testing code, it should no longer be necessary to check if the screenSpaceTransform is invertible.

Finally, I think its going to be a far more common case (and far more lightweight) to check if the layer is not scrollable.  Would it be equally correct to re-order these if-statements?  it would change semantics of which return values get returned, i'm not sure if that may change the correctness of this code here.

[Shawn Singh]

Hit testing patch at https://bugs.webkit.org/show_bug.cgi?id=88972.

[Dana Jansens]

Comment on attachment 146873 [details]
Patch

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

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:203
>> +    }
> 
> I think sreenSpaceTransform doesn't convert between layer's content space and screen space.  If I remember correctly, it converts between layer's "layout space" (with the origin at the top-left corner) and screen space.   So there needs to be an additional part of the transform that converts from the layer's origin space to the layer's content space.  The code I'll submit for hit testing tonight should have some code that can be assimilated here.
> 
> Additionally, after assimilating that hit testing code, it should no longer be necessary to check if the screenSpaceTransform is invertible.
> 
> Finally, I think its going to be a far more common case (and far more lightweight) to check if the layer is not scrollable.  Would it be equally correct to re-order these if-statements?  it would change semantics of which return values get returned, i'm not sure if that may change the correctness of this code here.

Ya see CCOcclusionTracker::contentToScreenSpaceTransform for stealable code.

[Sami Kyostila]

(In reply to comment #77)
> Finally, I think its going to be a far more common case (and far more lightweight) to check if the layer is not scrollable.  Would it be equally correct to re-order these if-statements?  it would change semantics of which return values get returned, i'm not sure if that may change the correctness of this code here.

There's one major constraint in ordering the checks here: ScrollOnMainThread must always take priority over the other cases. Otherwise we might neglect to send input events to the main thread for layers that expect that to happen.

However, I think we can avoid doing the projection if the nonFastScrollableRegion is empty, which is currently the case for all layers except the root scroll layer.

[Sami Kyostila]

This patch will now depend on the hit testing code from bug 88972.

[Sami Kyostila]

Created attachment 147383 [details]
Patch

This patch incorporates all the review feedback so far *except* for the extracted hit testing function since that is still being finalized.

- Added a dedicated scroll delegate to LayerChromium.
- Use cc component for tracing.
- Only hit test against self-representing layers.
- Renamed releaseRootLayer() to detachLayerTree()
- Instead of separating renderSurfaceLayerList from FrameData, made FrameData refcounted so CCLayerTreeHostImpl can cache the most recent one.

[Shawn Singh]

Hi Sami,

Do you know whether nonFastScrollableRegion was in the layer's "layout space" with the origin at the top-left corner, or if it was in the layer's "content space"?

[James Robinson]

Comment on attachment 147383 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:122
>      // ContentLayerDelegate implementation.
>      virtual void paintContents(GraphicsContext&, const IntRect& clip);
> +    virtual void didScroll(const IntSize&) OVERRIDE { }

this isn't part of ContentLayerDelegate - mind adding another comment block?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:389
> +    m_mostRecentFrameData = frame = FrameData::create();

this isn't what I meant - retaining the FrameData seems more problematic.

You specifically need the renderSurfaceLayerList to persist in order to hit test but the other frame-transient data is not interesting.  My idea was to move the renderSurfaceLayerList out and have FrameData refer to it by reference, but leave the other FrameData fields and FrameData itself transient.  Making all of FrameData refcounted has hugely bloated the patch and makes things more confusing, IMO

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:542
> +PassOwnPtr<CCLayerImpl> CCLayerTreeHostImpl::detachLayerTree()

I think the new name does make this much clearer - thanks!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:737
> +    scrollEnd();

not sure I fully understand this - my understanding of the CCInputHandler interface is that the caller has to make sure to call scrollEnd() after every successful scrollBegin(). are you finding that callers aren't doing this or that the API is overly restrictive? is this call just to update some internal state?

> Source/WebKit/chromium/src/WebContentLayerImpl.h:45
> +    virtual void didScroll(const WebCore::IntSize&);

do you still need this?

if this had an OVERRIDE annotation then the compiler could help tell if it was actually overriding anything or not

[Shawn Singh]

Comment on attachment 147383 [details]
Patch


Hi Sami, to make it a little easier for you - here are three simple steps for updating your existing patch to play nicely with the hit testing patch.

Thanks!


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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:199
> +    if (!screenSpaceTransform().isInvertible()) {
> +        TRACE_EVENT0("cc", "CCLayerImpl::tryScroll: Ignored nonInvertibleTransform");
> +        return CCInputHandlerClient::ScrollIgnored;
> +    }

This code should stay, unlike what I suggested earlier

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:205
> +    IntPoint contentPoint(screenSpaceTransform().inverse().mapPoint(viewportPoint));
> +    if (nonFastScrollableRegion().contains(contentPoint)) {
> +        TRACE_EVENT0("cc", "CCLayerImpl::tryScroll: Failed nonFastScrollableRegion");
> +        return CCInputHandlerClient::ScrollOnMainThread;
> +    }

This code should be replaced with the following:

    bool clipped = false;
    FloatPoint hitTestPointInLocalSpace = CCMathUtil::projectPoint(localSpaceToScreenSpaceTransform.inverse(), FloatPoint(viewportPoint), clipped);
    if (!clipped && nonFastScrollableRegion().contains(hitTestPointInLocalSpace)) {
        TRACE_EVENT0("cc", "CCLayerImpl::tryScroll: Failed nonFastScrollableRegion");
        return CCInputHandlerClient::ScrollOnMainThread;
    }

Similar math has landed in the hit testing patch, you can compare to that for a sanity check =)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:762
> +    // layers from the most recent frame.
> +    CCLayerImpl* layerImpl = 0;
> +
> +    typedef CCLayerIterator<CCLayerImpl, CCLayerList, CCRenderSurface, CCLayerIteratorActions::FrontToBack> CCLayerIteratorType;
> +    CCLayerIteratorType end = CCLayerIteratorType::end(&m_mostRecentFrameData->renderSurfaceLayerList);
> +    for (CCLayerIteratorType it = CCLayerIteratorType::begin(&m_mostRecentFrameData->renderSurfaceLayerList); it != end; ++it) {
> +        if (!it.representsItself())
> +            continue;
> +        CCLayerImpl* renderSurfaceLayerImpl = (*it);
> +        if (!renderSurfaceLayerImpl->screenSpaceTransform().isInvertible())
> +            continue;
> +        IntPoint scrollLayerPoint(renderSurfaceLayerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint));
> +        if (!renderSurfaceLayerImpl->visibleLayerRect().contains(scrollLayerPoint))
> +            continue;
> +        layerImpl = renderSurfaceLayerImpl;
> +        break;
>      }

The hit testing patch has landed; it should work to replace this code with the following:
CCLayerImpl* layerImpl = CCLayerTreeHostCommon::findLayerThatIsHitByPoint(viewportPoint,  yourRenderSurfaceLayerList);

[Sami Kyostila]

(In reply to comment #84)
> (From update of attachment 147383 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=147383&action=review
> 
> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:122
> >      // ContentLayerDelegate implementation.
> >      virtual void paintContents(GraphicsContext&, const IntRect& clip);
> > +    virtual void didScroll(const IntSize&) OVERRIDE { }
> 
> this isn't part of ContentLayerDelegate - mind adding another comment block?

Oops, I left that line in by mistake -- actually there's no need for didScroll() here at all. Oddly gcc didn't complain about the OVERRIDE.

> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:389
> > +    m_mostRecentFrameData = frame = FrameData::create();
> 
> this isn't what I meant - retaining the FrameData seems more problematic.
> 
> You specifically need the renderSurfaceLayerList to persist in order to hit test but the other frame-transient data is not interesting.  My idea was to move the renderSurfaceLayerList out and have FrameData refer to it by reference, but leave the other FrameData fields and FrameData itself transient.  Making all of FrameData refcounted has hugely bloated the patch and makes things more confusing, IMO

Right, gotcha. I just wanted to see if refcounted FrameData would be useful -- I recall reading somewhere that we might start preparing multiple animation frames in advance which seems to require independent FrameData instances. But let's keep things simple for now; I'll implement what you described above.
 
> > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:737
> > +    scrollEnd();
> 
> not sure I fully understand this - my understanding of the CCInputHandler interface is that the caller has to make sure to call scrollEnd() after every successful scrollBegin(). are you finding that callers aren't doing this or that the API is overly restrictive? is this call just to update some internal state?

You're right, that's what the interface requires and that's also what happens. This call is here just to reset everything related to the currently scrolling layer in case we don't find a new layer to scroll. I didn't want to duplicate the code from scrollEnd() so I'm just calling that function here.
 
> > Source/WebKit/chromium/src/WebContentLayerImpl.h:45
> > +    virtual void didScroll(const WebCore::IntSize&);
> 
> do you still need this?
> 
> if this had an OVERRIDE annotation then the compiler could help tell if it was actually overriding anything or not

D'oh, I missed this one too. Thanks for pointing it out.

[Sami Kyostila]

Created attachment 147607 [details]
Patch

- Use new hit testing utilities. (Thanks Shawn!)
- Keep render surface layer list in CCLayerTreeHostImpl.
- Removed two unneeded didScroll() functions.
- Added test for the new scroll delegate.

[James Robinson]

(In reply to comment #86)
> (In reply to comment #84)
> > (From update of attachment 147383 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=147383&action=review
> > 
> > > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:122
> > >      // ContentLayerDelegate implementation.
> > >      virtual void paintContents(GraphicsContext&, const IntRect& clip);
> > > +    virtual void didScroll(const IntSize&) OVERRIDE { }
> > 
> > this isn't part of ContentLayerDelegate - mind adding another comment block?
> 
> Oops, I left that line in by mistake -- actually there's no need for didScroll() here at all. Oddly gcc didn't complain about the OVERRIDE.
> 

gcc doesn't understand OVERRIDE until 4.7 (or so) :/

It's easy to use clang on linux if you'd like - ping me on gchat/email and I can send you some directions for getting it set up with google's setup.

> > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:737
> > > +    scrollEnd();
> > 
> > not sure I fully understand this - my understanding of the CCInputHandler interface is that the caller has to make sure to call scrollEnd() after every successful scrollBegin(). are you finding that callers aren't doing this or that the API is overly restrictive? is this call just to update some internal state?
> 
> You're right, that's what the interface requires and that's also what happens. This call is here just to reset everything related to the currently scrolling layer in case we don't find a new layer to scroll. I didn't want to duplicate the code from scrollEnd() so I'm just calling that function here.

Can we have an internal function that does the state resetting and ASSERT() that we get the expected usage of the exposed APIs?

[Sami Kyostila]

(In reply to comment #88)
> gcc doesn't understand OVERRIDE until 4.7 (or so) :/
> 
> It's easy to use clang on linux if you'd like - ping me on gchat/email and I can send you some directions for getting it set up with google's setup.

Thanks, I'm already on clang with help from our local llvm evangelist :)

> > > > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:737
> > > > +    scrollEnd();
> > > 
> > > not sure I fully understand this - my understanding of the CCInputHandler interface is that the caller has to make sure to call scrollEnd() after every successful scrollBegin(). are you finding that callers aren't doing this or that the API is overly restrictive? is this call just to update some internal state?
> > 
> > You're right, that's what the interface requires and that's also what happens. This call is here just to reset everything related to the currently scrolling layer in case we don't find a new layer to scroll. I didn't want to duplicate the code from scrollEnd() so I'm just calling that function here.
> 
> Can we have an internal function that does the state resetting and ASSERT() that we get the expected usage of the exposed APIs?

Good idea, I'll do that.

[Sami Kyostila]

Created attachment 147615 [details]
Patch

[James Robinson]

Comment on attachment 147615 [details]
Patch

OK, looks good.

[WebKit Review Bot]

Comment on attachment 147615 [details]
Patch

Rejecting attachment 147615 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t 2 lines).
Hunk #7 succeeded at 450 (offset 2 lines).
Hunk #8 succeeded at 496 (offset 2 lines).
Hunk #9 succeeded at 807 (offset 2 lines).
patching file Source/WebKit/chromium/tests/CCLayerTreeHostTest.cpp
Hunk #1 succeeded at 2220 (offset 20 lines).
patching file Source/WebKit/chromium/tests/WebCompositorInputHandlerImplTest.cpp

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'James Robi..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/12967111

[Shawn Singh]

Created attachment 147654 [details]
Exact same patch rebased to tip of tree

It was just an innocent conflict in CCLayerTreeHostImpl.h.  Otherwise this patch should be _exactly_ the same.  I just wanted to push this to land sooner since its getting close to branch. Also double-checked that it still compiles and passes all unit tests after rebase.

[WebKit Review Bot]

Comment on attachment 147654 [details]
Exact same patch rebased to tip of tree

Rejecting attachment 147654 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebKit/chromium/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12953732

[Shawn Singh]

Created attachment 147700 [details]
Exact same patch again

Sorry for the churn, it was completely my mistake, and should be fixed this time. Re-ran debug/release unit tests and release layout tests, no obvious regressions (running debug layout tests now, too).

[Sami Kyostila]

Created attachment 147779 [details]
Patch

Rebased.

[Peter Beverloo]

Comment on attachment 147779 [details]
Patch

Patch has already been reviewed, setting cq+ on request.

[WebKit Review Bot]

Comment on attachment 147779 [details]
Patch

Clearing flags on attachment: 147779

Committed r120444: <http://trac.webkit.org/changeset/120444>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sami Kyostila]

Just one short of a centi-bug :) Thanks to everyone who has helped this patch forward along the way!