Bug 74196 – [chromium] Delegate scroll events to the main thread when needed

• Status: RESOLVED
• Resolution: DUPLICATE
• Component: UI Events

[Sami Kyostila]

Register a scrollable region for each LayerChromium and delegate scroll events inside the region to the main thread for processing.

Note that this essentially splits out the Chromium-only changes from bug 71385.

[Sami Kyostila]

Created attachment 118606 [details]
Patch

[Sami Kyostila]

Created attachment 119033 [details]
Patch

- Rebased after latest changes to bug 73350.

[Adrienne Walker]

Comment on attachment 119033 [details]
Patch

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

Other than one small thing, this is looking good to me.  Thanks for the tests!

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:341
> +    IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint));
> +    // The content point for non-root layers must be scaled with the page scale. Note that the
> +    // page scale delta is already embedded in each layer's screen space transform.
> +    if (layerImpl != m_scrollLayerImpl)
> +        contentPoint.scale(m_pageScale, m_pageScale);

I know you're just moving code here, but I don't follow this logic and why the scroll layer behaves differently.  Would you mind explaining why this is? Don't all layers (root or no) have the page scale applied to them?

Also, do you need to apply the page scale delta? Maybe I'd feel more comfortable if there were more tests for this function that covered page scale.

[Sami Kyostila]

Comment on attachment 119033 [details]
Patch

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

Setting as review- until the preceding patch layer scrolling is updated.

>> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:341

> 
> I know you're just moving code here, but I don't follow this logic and why the scroll layer behaves differently.  Would you mind explaining why this is? Don't all layers (root or no) have the page scale applied to them?
> 
> Also, do you need to apply the page scale delta? Maybe I'd feel more comfortable if there were more tests for this function that covered page scale.

Turns out the code here is not entirely correct, so I'll rework it in the preceding patch. Actually there is no need to differentiate between layers or scale the content point, but instead visibleLayerRect() must be scaled with the inverse of the page scale to get unscaled css coordinates that match the coordinate system of contentPoint.

The reason why page scale delta is not needed is that while we are zooming, visibleLayerRect() is recomputed in terms of the original page scale. It never includes m_pageScaleDelta so there is no need to undo that here. I'm not really sure whether this is intended, though.

[Sami Kyostila]

Created attachment 119287 [details]
Patch

[James Robinson]

This seems to have quite a few merge conflicts on ToT, does it depend on https://bugs.webkit.org/show_bug.cgi?id=73350 ?

[Sami Kyostila]

(In reply to comment #6)
> This seems to have quite a few merge conflicts on ToT, does it depend on https://bugs.webkit.org/show_bug.cgi?id=73350 ?

Yes, this patch should be applied after the one you linked. Sorry, I should have mentioned that here.

[Sami Kyostila]

Created attachment 119684 [details]
Patch

- Rebased after changes to bug 73350 (no functional changes)

[James Robinson]

Comment on attachment 119684 [details]
Patch

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

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

if this layer's screen space transform is not invertible, you need to return false here - just calling .inverse().mapPoint() will map the point through an identity transform

[Sami Kyostila]

Created attachment 120064 [details]
Patch

 - Updating according to layer walking order changes in bug 73350.

[Sami Kyostila]

Created attachment 120068 [details]
Patch

 - Rebased.

[James Robinson]

Comment on attachment 120068 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:480
> +    if (!layerImpl)
> +        return false;

when can this parameter be null?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:489
> +    if (!layerImpl->screenSpaceTransform().isInvertible())
> +        return false;
> +
> +    IntPoint contentPoint(layerImpl->screenSpaceTransform().inverse().mapPoint(viewportPoint));
> +    if (layerImpl->drawsContent() && !isContentPointWithinLayer(layerImpl, contentPoint))
> +        return false;
> +
> +    if (layerImpl->isInsideScrollableRegion(contentPoint))

there's not a whole lot (if anything) going on here that the CCLayerTreeHostImpl can do and the layer itself can't - all of the logic here is based on member functions of layerImpl. should this whole thing just be a member function of CCLayerImpl?

> Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:526
> +    if (!layerImpl->drawsContent() && (layerImpl->scrollable() || !layerImpl->scrollableRegion().isEmpty()))

at this point, this probably deserves at least a comment explaining why a given layer should or shouldn't be in the assembled list since it's no longer a list of non-drawable scrollable layers

[James Robinson]

*** This bug has been marked as a duplicate of bug 79155 ***