Bug 71385 – Threaded compositor should delegate overflow div/iframe scroll events to WebKit

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

[Sami Kyostila]

When an input scroll event is received by the threaded compositor, it should check whether the event targets an overflow div or iframe and forward it to WebKit if that is the case.

[Sami Kyostila]

Created attachment 116256 [details]
Patch for hit testing input events

The attached patch adds metadata to GraphicsLayers about scrollable elements within them. When the compositor receives an input event, it is hit tested against these elements. If the test succeeds, the event is delegated to the main thread instead of being processed by the compositor.

The patch also modifies layerTreeAsText() to accept a bitmask specifying the data to output in a similar way to renderTreeAsText().

[WebKit Review Bot]

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

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/comp..." exit_code: 1

Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:88:  The parameter name "r" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHostImpl.cpp:305:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 2 in 31 files


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

[WebKit Review Bot]

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

[Sam Weinig]

I don't think it makes sense for GraphicsLayers to know anything about scrolling.

[Sam Weinig]

Since this touches a lot of stuff outside of the Chromium directories, I am removing the [Chromium] prefix.

[Vangelis Kokkevis]

Comment on attachment 116256 [details]
Patch for hit testing input events

I would think that in order to effectively be able to scroll on the thread, we'll first need to promote the scrolling element into a composited layer.  Then it will be a matter of marking an entire GraphicsLayer as scrollable.  The thread won't be able to repaint/scroll composited RLs that contain more than just the scrolling element.

[Sami Kyostila]

(In reply to comment #6)
> (From update of attachment 116256 [details])
> I would think that in order to effectively be able to scroll on the thread, we'll first need to promote the scrolling element into a composited layer.  Then it will be a matter of marking an entire GraphicsLayer as scrollable.  The thread won't be able to repaint/scroll composited RLs that contain more than just the scrolling element.

I agree that for efficient scrolling we want to do exactly that. However, a concern I have is that are we really able to promote _every_ scrollable element to a GraphicsLayer? That could have significant memory and fillrate overhead. Also, I understood there is an issue with automatic promotion of overflow divs into layers due to the lack of a stacking context.

The patch I've attached enables the "slow path" (main thread) scrolling for elements that do not have their own layers. The next step is to make scrolling work for fully composited layers.

Do you think this approach makes sense, or is it safe to assume that all scrollable elements can be promoted to layers?

[James Robinson]

Can you please separate the chromium-specific pieces from the cross-platform code changes?

I agree that we should keep scrolling concerns separate from GraphicsLayer in the cross-platform logic.

[Simon Fraser (smfr)]

Comment on attachment 116256 [details]
Patch for hit testing input events

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

> Source/WebCore/platform/graphics/GraphicsLayer.h:307
> +    // Set coordinates of scrollable elements inside the layer. These are given
> +    // in scaled document coordinates.
> +    const Vector<IntRect>& scrollableRects() const { return m_scrollableRects; }
> +    virtual void setScrollableRects(const Vector<IntRect>& r) { m_scrollableRects = r; }

It's not clear what "scrollable rects" are for. Why aren't the child scrollable things in their own GraphicsLayers?

> Source/WebCore/rendering/RenderLayer.cpp:3884
> +Vector<IntRect> RenderLayer::scrollableChildren() const

This isn't returning a list of children, it's returning a list of rects, so the name is wrong.

[Sami Kyostila]

(In reply to comment #8)
> Can you please separate the chromium-specific pieces from the cross-platform code changes?

Good idea, done: https://bugs.webkit.org/show_bug.cgi?id=73345
 
> I agree that we should keep scrolling concerns separate from GraphicsLayer in the cross-platform logic.

Fair enough. I'll reformulate the patch without adding anything to GraphicsLayer.

[Sami Kyostila]

(In reply to comment #9)
> (From update of attachment 116256 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=116256&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsLayer.h:307
> > +    // Set coordinates of scrollable elements inside the layer. These are given
> > +    // in scaled document coordinates.
> > +    const Vector<IntRect>& scrollableRects() const { return m_scrollableRects; }
> > +    virtual void setScrollableRects(const Vector<IntRect>& r) { m_scrollableRects = r; }
> 
> It's not clear what "scrollable rects" are for. Why aren't the child scrollable things in their own GraphicsLayers?

Scrollable rectangles inside a layer signify areas where the chromium compositor thread should not attempt to process scroll events by itself (by translating the layer coordinates), but instead it should forward those events to the WebKit thread. These areas are created for things like iframes and elements with overflow scrolling which do not have a RenderLayerBacking of their own.

Initially I wasn't sure whether it is too much of an overhead to promote scrollable elements into composited layers -- especially if there are no graphical reasons to do so -- so I went with this approach of adding metadata  to existing GraphicsLayers.

It sounds like it would be better instead to create GraphicsLayers for those elements but still render the elements themselves into the same enclosing layer as before. That is, the GraphicsLayers would only serve as blank placeholders for compositor hit testing. Is this closer to what you had in mind?

> > Source/WebCore/rendering/RenderLayer.cpp:3884
> > +Vector<IntRect> RenderLayer::scrollableChildren() const
> 
> This isn't returning a list of children, it's returning a list of rects, so the name is wrong.

Agreed, I'll fix this.

[Simon Fraser (smfr)]

(In reply to comment #11)
> (In reply to comment #9)
> > (From update of attachment 116256 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=116256&action=review
> > 
> > > Source/WebCore/platform/graphics/GraphicsLayer.h:307
> > > +    // Set coordinates of scrollable elements inside the layer. These are given
> > > +    // in scaled document coordinates.
> > > +    const Vector<IntRect>& scrollableRects() const { return m_scrollableRects; }
> > > +    virtual void setScrollableRects(const Vector<IntRect>& r) { m_scrollableRects = r; }
> > 
> > It's not clear what "scrollable rects" are for. Why aren't the child scrollable things in their own GraphicsLayers?
> 
> Scrollable rectangles inside a layer signify areas where the chromium compositor thread should not attempt to process scroll events by itself (by translating the layer coordinates), but instead it should forward those events to the WebKit thread. These areas are created for things like iframes and elements with overflow scrolling which do not have a RenderLayerBacking of their own.

This sounds like information that should be kept in the chromium layer, not pushed down to GraphicsLayer.

> It sounds like it would be better instead to create GraphicsLayers for those elements but still render the elements themselves into the same enclosing layer as before. That is, the GraphicsLayers would only serve as blank placeholders for compositor hit testing. Is this closer to what you had in mind?

No. I don't want to deviate from a GraphicsLayer representing some kind of composited surface. It sounds like Chromium's scrolling needs are pretty platform-specific, so you should keep them in the Chromium layer.

[Vangelis Kokkevis]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #9)
> > > (From update of attachment 116256 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=116256&action=review
> > > 
> > > > Source/WebCore/platform/graphics/GraphicsLayer.h:307
> > > > +    // Set coordinates of scrollable elements inside the layer. These are given
> > > > +    // in scaled document coordinates.
> > > > +    const Vector<IntRect>& scrollableRects() const { return m_scrollableRects; }
> > > > +    virtual void setScrollableRects(const Vector<IntRect>& r) { m_scrollableRects = r; }
> > > 
> > > It's not clear what "scrollable rects" are for. Why aren't the child scrollable things in their own GraphicsLayers?
> > 
> > Scrollable rectangles inside a layer signify areas where the chromium compositor thread should not attempt to process scroll events by itself (by translating the layer coordinates), but instead it should forward those events to the WebKit thread. These areas are created for things like iframes and elements with overflow scrolling which do not have a RenderLayerBacking of their own.
> 
> This sounds like information that should be kept in the chromium layer, not pushed down to GraphicsLayer.

Simon: This issue seems to keep coming up a lot lately.  The background here (in case it wasn't clear) is that we're working on getting compositing to happen on a thread separate from where the rest of the webkit logic runs. We want to allow that thread to handle basic user input events (mostly scroll for now) on its own whenever possible to improve responsiveness.  In order to effectively do that, we need the composited layer tree nodes to carry more information than they currently do.  Specifically we've had the need for:
1. Tagging fixed position layers so that the compositor can position them while scrolling
2. Tagging overflow layers as scrollable 
3. Tagging scroll areas within a layer in order to avoid scrolling the whole page when we should be scrolling an element (this patch)

With enough contortions we could call out from RLC and RLB to the ChromeClient and set those properties directly to the Chromium layers but it feels that these properties could be generally useful for other ports and should be stored directly into the GraphicsLayer's .  Thoughts?

[Simon Fraser (smfr)]

(In reply to comment #13)

> Simon: This issue seems to keep coming up a lot lately.  The background here (in case it wasn't clear) is that we're working on getting compositing to happen on a thread separate from where the rest of the webkit logic runs. We want to allow that thread to handle basic user input events (mostly scroll for now) on its own whenever possible to improve responsiveness.  In order to effectively do that, we need the composited layer tree nodes to carry more information than they currently do.  Specifically we've had the need for:
> 1. Tagging fixed position layers so that the compositor can position them while scrolling
> 2. Tagging overflow layers as scrollable 
> 3. Tagging scroll areas within a layer in order to avoid scrolling the whole page when we should be scrolling an element (this patch)
> 
> With enough contortions we could call out from RLC and RLB to the ChromeClient and set those properties directly to the Chromium layers but it feels that these properties could be generally useful for other ports and should be stored directly into the GraphicsLayer's .  Thoughts?

The GraphicsLayers tree is an output-only construct, and I think jury-rigging it to contain additional meta-information about the compositing tree may not be the best approach.

What we really need is:
* a data structure that describes the transformations that can happen to the compositing tree via scrolling, that is accessible from another thread/process
* a way to update the (platform-specific) compositing tree on scrolling, from another thread/process
* a way to notify WebCore that scrolling happened, so it can sync up with the new scroll positions

I'm concerned about trying to use the GraphicsLayer tree for event handling, also. The bounds of GraphicsLayers often do not correspond with the bounds of the element, so simply using the bounds of some GraphicsLayer to determine you should treat an event as a scroll will often be wrong. You really have to hit-test through WebCore to get the correct answer, or be willing to push a lot of information into another data structure.

[Vangelis Kokkevis]

(In reply to comment #14)
> (In reply to comment #13)
> 
> > Simon: This issue seems to keep coming up a lot lately.  The background here (in case it wasn't clear) is that we're working on getting compositing to happen on a thread separate from where the rest of the webkit logic runs. We want to allow that thread to handle basic user input events (mostly scroll for now) on its own whenever possible to improve responsiveness.  In order to effectively do that, we need the composited layer tree nodes to carry more information than they currently do.  Specifically we've had the need for:
> > 1. Tagging fixed position layers so that the compositor can position them while scrolling
> > 2. Tagging overflow layers as scrollable 
> > 3. Tagging scroll areas within a layer in order to avoid scrolling the whole page when we should be scrolling an element (this patch)
> > 
> > With enough contortions we could call out from RLC and RLB to the ChromeClient and set those properties directly to the Chromium layers but it feels that these properties could be generally useful for other ports and should be stored directly into the GraphicsLayer's .  Thoughts?
> 
> The GraphicsLayers tree is an output-only construct, and I think jury-rigging it to contain additional meta-information about the compositing tree may not be the best approach.
> 
> What we really need is:
> * a data structure that describes the transformations that can happen to the compositing tree via scrolling, that is accessible from another thread/process
> * a way to update the (platform-specific) compositing tree on scrolling, from another thread/process
> * a way to notify WebCore that scrolling happened, so it can sync up with the new scroll positions
> 
> I'm concerned about trying to use the GraphicsLayer tree for event handling, also. The bounds of GraphicsLayers often do not correspond with the bounds of the element, so simply using the bounds of some GraphicsLayer to determine you should treat an event as a scroll will often be wrong. You really have to hit-test through WebCore to get the correct answer, or be willing to push a lot of information into another data structure.

What we're after is simply expanding the set of properties stored on the GraphicsLayers to include those necessary for handling scrolling and basic layer layout on the PlatformLayer tree. The flow of information will remain fundamentally one way: From the RenderLayers to the PlatformLayers via the GraphicsLayer tree.  The WebKit thread will always hold the ground truth for all the properties and the GraphicsLayer tree will remain an output only construct.

The model is very similar to what already exists today for transform/opacity animations that are handled by the compositor.  The compositor may be computing new transform and opacity values over time but it doesn't (AFAIK) feed them back to the main tree.  The only updates that we see computed on the compositor are additional scroll offsets that need to be reconciled with WebKit's scrolls but they won't be feeding back through the GraphicsLayer tree.

[Adam Barth]

Comment on attachment 116256 [details]
Patch for hit testing input events

This patch seems really old.  I'm clearing the review flag.  If this patch should still be up for review, please feel free to re-nominate.

[Sami Kyöstilä]

This particular piece of functionality was already done in bug 79155. The remaining items for scrollable sublayers are being implemented as part of bugs 78862 and 91117.

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