Bug 110323 – Accelerated overflow scrolling for Coordinated Graphics.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Luiz Agostini]

The objective is to expose an API that will make it possible to scroll layers on UI process before notifying the web process. The user will then be able to scroll layers even when the web process is busy.

[Luiz Agostini]

Created attachment 189284 [details]
WIP

This is a WIP. I will not submit it for review but comments are welcome.

[Luiz Agostini]

Comment on attachment 189284 [details]
WIP

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

We are working on a Coordinated Graphics API test infrastructure as well. I plan to add tests for this API as soon as it is ready.
There are some parts of this patch that make changes in code that can only be found in NIX repository. They will of course be removed on the final patch. For now they might help on showing how to use the API.
This patch is very raw. There is a lot to be improved. I will have a better version soon.

> Source/WebCore/rendering/RenderLayer.cpp:522
> +//        && renderer()->frame()->page()->settings()->acceleratedCompositingForOverflowScrollEnabled();

please ignore this code. It is just for testing.

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.cpp:864
> +void CoordinatedLayerTreeHost::scrollingLayerWasAdded(uint32_t layerId)
> +{
> +    m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::ScrollingLayerWasAdded(layerId));
> +}
> +
> +void CoordinatedLayerTreeHost::scrollingLayerWasRemoved(uint32_t layerId)
> +{
> +    m_webPage->send(Messages::CoordinatedLayerTreeHostProxy::ScrollingLayerWasRemoved(layerId));
> +}

To use the CoordinatedLayerInfo to pass this information would be a lot more efficient. It would remove a lot of code as well.

[Antonio Gomes]

Comment on attachment 189284 [details]
WIP

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:619
> +template<class SearchCondition> TextureMapperLayer* TextureMapperLayer::hitTestSearch(const FloatPoint& point, SearchCondition condition)

could this HitTest be non-texturemapper dependent? so others can use it?

[Noam Rosenthal]

(In reply to comment #3)
> (From update of attachment 189284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189284&action=review
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:619
> > +template<class SearchCondition> TextureMapperLayer* TextureMapperLayer::hitTestSearch(const FloatPoint& point, SearchCondition condition)
> 
> could this HitTest be non-texturemapper dependent? so others can use it?
This hit-test is for UI-side of coordinated graphics, where do you propose we put it?

[Noam Rosenthal]

Comment on attachment 189284 [details]
WIP

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

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:160
> +    virtual void graphicsLayerWillBeRemoved(GraphicsLayer*) { }

I don't think this is necessary.
If instead of a HashMap we kept a ScrollableArea pointer inside CoordinatedGraphicsLayer, this would be implicit and a lot of the code in this patch which deals with maintaing that map wouldn't be needed...

[Antonio Gomes]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 189284 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=189284&action=review
> > 
> > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:619
> > > +template<class SearchCondition> TextureMapperLayer* TextureMapperLayer::hitTestSearch(const FloatPoint& point, SearchCondition condition)
> > 
> > could this HitTest be non-texturemapper dependent? so others can use it?
> This hit-test is for UI-side of coordinated graphics, where do you propose we put it?

- We have touch targets cached for hit testing speed up.
- BlackBerry port has design as having WebKitThread its UIThread layer counterpart, and so does chromium. I had a project of doing the same kind of caching for LayerCompositingThread (as it is called in the BlackBerry land), so hit-testing it could be speed up by a lot, but left RIM before had a chance to got to it.

That said, I believe this case, could benefit this could benefit others and live somewhere in the three not necessarily being webkit2/coordinated graphics specific.

Please, correct if I am wrong.

[Dongseong Hwang]

Comment on attachment 189284 [details]
WIP

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

I really appreciate your effort, but I have some concern. No offence :)

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:68
> +    void scrollBy(const WebCore::IntSize&);

I think that it is not good that TextureMapperLayer deals with scroll and scrollClient. Those stuffs are only for root layer.
I think we can do the same thing as CoordinatedGraphicsScene just manage the position of root layer.
In addition, I want GraphicsLayerTransform not to know scrollPosition too.

I think hitTest is fine but I want that we share related code with other ports if possible. hitTest would be separated in other patch..

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h:108
> +    virtual void scrollLayerBy(uint32_t layerID, const WebCore::IntSize& offset);

I'm not sure if it is natural that CoordinatedLayerTreeHostProxy has scroll api.
I think we should scroll using WebPage and WebPage will call FrameView::setScrollPosition instead of ScrollableArea::notifyScrollPositionChanged.
In addition, if CoordinatedLayerTreeHostProxy has scrollBy, why CoordinatedLayerTreeHostProxy does not has scaling, wheel, key event, mouse move or something.

> Tools/MiniBrowser/nix/main.cpp:493
> +        WKCGScrollBy(m_scrollingLayer, WKSizeMake(m_scrollPoint.x - event.x, m_scrollPoint.y - event.y));

I'm not sure why we make new interface instead of improving existing interface: WebPageProxy::scrollBy
IMHO this api expose our implementation detail: CoordinatedGraphicsScene..
We sometime scroll using PageViewportController and somtime scroll using CoordinatedGraphicsScene. I think it is a little confusing..

In my opinion, we should use common interface for scrolling and we should find the way to accelerate scroll on all ports that use coordinated graphics.

[Luiz Agostini]

Created attachment 189438 [details]
WIP

Same stuff, hopefully a bit simpler.

[Luiz Agostini]

Thank you very much for looking at the code!

(In reply to comment #7)
> (From update of attachment 189284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189284&action=review
> 
> I really appreciate your effort, but I have some concern. No offence :)
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:68
> > +    void scrollBy(const WebCore::IntSize&);
> 
> I think that it is not good that TextureMapperLayer deals with scroll and scrollClient. Those stuffs are only for root layer.
> I think we can do the same thing as CoordinatedGraphicsScene just manage the position of root layer.
> In addition, I want GraphicsLayerTransform not to know scrollPosition too.

I am not sure I was clear in my description but the point here is to scroll layers independently. It is about scrolling each layer on UI side, not the whole page. As the scroll is specific for each layer it looks natural that the layer has scroll information.

> 
> I think hitTest is fine but I want that we share related code with other ports if possible. hitTest would be separated in other patch..

This hittest is only about coordinated graphics layers. It is here because I need to be able to identify the layers that are scrollable on UI side. I would not have a hittest on this patch if it was not necessary. This patch does not work without it.

It is not clear for me how a hittest that just checks coordinated graphics layers could be used without coordinated graphics.

> 
> > Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.h:108
> > +    virtual void scrollLayerBy(uint32_t layerID, const WebCore::IntSize& offset);
> 
> I'm not sure if it is natural that CoordinatedLayerTreeHostProxy has scroll api.
> I think we should scroll using WebPage and WebPage will call FrameView::setScrollPosition instead of ScrollableArea::notifyScrollPositionChanged.

As I said it is about scrolling individual layers, not the whole page. What this patch does is to give a ScrollableArea to all the scrollable layers at web process. That scrollable area is then used to scroll that layer independently of the other layers or the page. 

> In addition, if CoordinatedLayerTreeHostProxy has scrollBy, why CoordinatedLayerTreeHostProxy does not has scaling, wheel, key event, mouse move or something.

CoordinatedLayerTreeHostProxy does not have scrollBy, it has scrollLayerBy (it is about individual layers :). It only has this method because CoordinatedLayerTreeHostProxy is who have the m_drawingAreaProxy->page()->process() to send messages to web process.

> 
> > Tools/MiniBrowser/nix/main.cpp:493
> > +        WKCGScrollBy(m_scrollingLayer, WKSizeMake(m_scrollPoint.x - event.x, m_scrollPoint.y - event.y));
> 
> I'm not sure why we make new interface instead of improving existing interface: WebPageProxy::scrollBy

to be able to scroll layers independently of the rest of the page.

> IMHO this api expose our implementation detail: CoordinatedGraphicsScene..

To expose some API for Coordinated Graphics is the objective of this patch. It should be done with care not to expose the actual implementation.

> We sometime scroll using PageViewportController and somtime scroll using CoordinatedGraphicsScene. I think it is a little confusing..

The idea is to have one api that could be used to scroll both, the page and individual layers. To scroll the page is to scroll the content of the root layer.

> 
> In my opinion, we should use common interface for scrolling and we should find the way to accelerate scroll on all ports that use coordinated graphics.

I fully agree. That is why the API was based on Coordinated Graphics alone. There is some nix code but it should be seen simply as an usage example and of course it will not be part of the final patch.

I hope the latest patch is a bit simpler.

[Luiz Agostini]

(In reply to comment #6)
> (In reply to comment #4)
> > (In reply to comment #3)
> > > (From update of attachment 189284 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=189284&action=review
> > > 
> > > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:619
> > > > +template<class SearchCondition> TextureMapperLayer* TextureMapperLayer::hitTestSearch(const FloatPoint& point, SearchCondition condition)
> > > 
> > > could this HitTest be non-texturemapper dependent? so others can use it?
> > This hit-test is for UI-side of coordinated graphics, where do you propose we put it?
> 
> - We have touch targets cached for hit testing speed up.
> - BlackBerry port has design as having WebKitThread its UIThread layer counterpart, and so does chromium. I had a project of doing the same kind of caching for LayerCompositingThread (as it is called in the BlackBerry land), so hit-testing it could be speed up by a lot, but left RIM before had a chance to got to it.
> 
> That said, I believe this case, could benefit this could benefit others and live somewhere in the three not necessarily being webkit2/coordinated graphics specific.
> 
> Please, correct if I am wrong.

Hi Antonio,

yes, it might be possible to have a hittest that is based on GraphicsLayers instead if TextureMapperLayers. I am not sure how much code would be shared though, because the hittest itself quite simple and small code. It is just a depth first search.

I will consider it. This patch is WIP anyway. And it will probably be splitted before landing.

[Luiz Agostini]

(In reply to comment #5)
> (From update of attachment 189284 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189284&action=review
> 
> > Source/WebCore/page/scrolling/ScrollingCoordinator.h:160
> > +    virtual void graphicsLayerWillBeRemoved(GraphicsLayer*) { }
> 
> I don't think this is necessary.
> If instead of a HashMap we kept a ScrollableArea pointer inside CoordinatedGraphicsLayer, this would be implicit and a lot of the code in this patch which deals with maintaing that map wouldn't be needed...

You are right. I added a ScrollableArea to CoordinatedGraphicsLayer and used its presence as a flag to identify scrollable layers. And added a flag to CoordinatedLayerInfo instead of having specific messages for that. I think that it improved a lot the code.

Thanks!

[Luiz Agostini]

Comment on attachment 189438 [details]
WIP

This code is hopefully better than the previous, but it is still WIP.

Coordinated Graphics test infrastructure is been cooked and this API will have tests as soon as we have it. Please consider the NIX code simply as a usage example, it will not be in the final patch.

[Dongseong Hwang]

Comment on attachment 189438 [details]
WIP

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

> Source/WebCore/platform/graphics/GraphicsLayerTransform.cpp:114
> +{

you may need to add m_dirty = true;

> Source/WebCore/platform/graphics/GraphicsLayerTransform.cpp:119
> +{

ditto.

> Tools/MiniBrowser/nix/main.cpp:488
> +        WKCGScrollBy(m_scrollingLayer, WKSizeMake(m_scrollPoint.x - event.x, m_scrollPoint.y - event.y));

I understand now you want to handle div with scrollbar like http://people.opera.com/pepelsbey/experiments/bga/ It's awesome.
I'm sorry for previous insane comment :) Forget that plz.

However, I still hope coordinated graphics deal with scrollable element scrolling as well as whole page scrolling..
I'm not sure it is good that Minibrowser queries scrollable layer and control the scroll position of each layer..

How about extract scroll and scale code from PageViewportController,
and the code belongs to Coordinated Graphics,
and EFL, Nix, and Qt, by alphabetical order :), use the common code?

btw, how does Nix deal with viewport meta tag in UI Process? I mean how to decide initial scale, and how to scroll page, and how to make WebPage know the view size, and how to deal with deviceScaleFactor?

[Luiz Agostini]

(In reply to comment #13)
> (From update of attachment 189438 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189438&action=review
> 
> > Source/WebCore/platform/graphics/GraphicsLayerTransform.cpp:114
> > +{
> 
> you may need to add m_dirty = true;
> 
> > Source/WebCore/platform/graphics/GraphicsLayerTransform.cpp:119
> > +{
> 
> ditto.

Thanks! The changes in GraphicsLayerTransform are indeed some of the parts that still need to be improved in this patch.

> 
> > Tools/MiniBrowser/nix/main.cpp:488
> > +        WKCGScrollBy(m_scrollingLayer, WKSizeMake(m_scrollPoint.x - event.x, m_scrollPoint.y - event.y));
> 
> I understand now you want to handle div with scrollbar like http://people.opera.com/pepelsbey/experiments/bga/ It's awesome.
> I'm sorry for previous insane comment :) Forget that plz.

Don't worry about it. Your comments are always welcome, :)

> 
> However, I still hope coordinated graphics deal with scrollable element scrolling as well as whole page scrolling..

That is the same for me.

> I'm not sure it is good that Minibrowser queries scrollable layer and control the scroll position of each layer..

Again it was just a usage example. It made it easy for me to test. But I think that it might be a valid use of this API. It would make it possible for example to interpret gestures outside of webkit and at some point start scrolling a certain layer.

> 
> How about extract scroll and scale code from PageViewportController,
> and the code belongs to Coordinated Graphics,
> and EFL, Nix, and Qt, by alphabetical order :), use the common code?

I understand that each port wants to take those decisions by their own but coordinated graphics should have its core functionality independently of the ports. The idea is to identify the core functionality and create APIs, and then implement port independent tests for those APIs. If each port will use the APIs or use coordinated graphics internals is a port decision, I hope that the API will be good enough for them to consider using it. But having those APIs and having tests for them will guarantee that the core functionality is the same for all ports and avoid regressions.

> 
> btw, how does Nix deal with viewport meta tag in UI Process? I mean how to decide initial scale, and how to scroll page, and how to make WebPage know the view size, and how to deal with deviceScaleFactor?

My understanding is that NIX avoids implementing any browser behavior and just provide the means to implement that behavior outside of it. It might make the initial use of it a bit more difficult but at the same time it provides more flexibility. That is why I decided to test this patch on NIX, because its view is very simple and it was fairly  easy to test the code without needing to consider complex interactions with other parts of the system.

[Arvid Nilsson]

(In reply to comment #10)
> (In reply to comment #6)
> > (In reply to comment #4)
> > > (In reply to comment #3)
> > > > (From update of attachment 189284 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=189284&action=review
> > > > 
> > > > > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:619
> > > > > +template<class SearchCondition> TextureMapperLayer* TextureMapperLayer::hitTestSearch(const FloatPoint& point, SearchCondition condition)
> > > > 
> > > > could this HitTest be non-texturemapper dependent? so others can use it?
> > > This hit-test is for UI-side of coordinated graphics, where do you propose we put it?
> > 
> > - We have touch targets cached for hit testing speed up.
> > - BlackBerry port has design as having WebKitThread its UIThread layer counterpart, and so does chromium. I had a project of doing the same kind of caching for LayerCompositingThread (as it is called in the BlackBerry land), so hit-testing it could be speed up by a lot, but left RIM before had a chance to got to it.
> > 
> > That said, I believe this case, could benefit this could benefit others and live somewhere in the three not necessarily being webkit2/coordinated graphics specific.
> > 
> > Please, correct if I am wrong.
> 
> Hi Antonio,
> 
> yes, it might be possible to have a hittest that is based on GraphicsLayers instead if TextureMapperLayers. I am not sure how much code would be shared though, because the hittest itself quite simple and small code. It is just a depth first search.
> 
> I will consider it. This patch is WIP anyway. And it will probably be splitted before landing.

Thanks for adding me to the bug Antonio, I'll add Mike too. Mike and I were discussing hit test of the selection overlay the other day. In order to enable correct hit testing for selections in iframes/fixed pos divs/scrollable divs or partly obscured selections, we would want to implement layer-based hit testing there. This is not really related to scrolling (which this bug is about), but there seems to be a hit testing aspect to it, too.

My initial thought was to implement the hit testing in the LayerCompositingThread (our equivalent of TextureMapperLayer). It would be nice to be able to share hit testing code with other ports, but I'm not sure it's possible to abstract out the "hit test layer tree on non-webkit thread" logic... We also need to do a bit more than just rect-based hit testing, since if we end up in the selection overlay, we also need to hit test the actual selection region.

[Allan Sandfeld Jensen]

Wouldn't the API be a ScrollingCoordinator client? 

We already have working api for scrolling on the UI side, what is missing is for the UI side know how the layers are organized and if the events should be send to the WebProcess to be handled. Most of what you need is the same as the ScrollTree used by threaded scrolling.

Though the first step could be just to register areas which needs to be handled by WebCore, and only let UI handle the rest.

[Allan Sandfeld Jensen]

(In reply to comment #16)
> Most of what you need is the same as the ScrollTree used by threaded scrolling.
> 
I had expected we would actually first go through of task of making threaded scrolling work with coordinated graphics, and then try to take that implementation and the experience learned to improve the process separated API.

[Noam Rosenthal]

I think a lot of the confusion in this thread is because of the title; This bug is for *composited overflow scroll*, not for general UI side scroll.

[Allan Sandfeld Jensen]

(In reply to comment #18)
> I think a lot of the confusion in this thread is because of the title; This bug is for *composited overflow scroll*, not for general UI side scroll.

Ah right, composited overflow scroll is how I would implement a general scrolling abstraction. I believe Chromium is using a setting called forceCompositingMode together with acceleratedCompositingForScrollableFramesEnabled which makes frameview scrolling into overflow scrolling. This way you just need to coordinated overflow scrolling, and you have a general solution.

There is still the problem with DOM event handling, and making it possible for JavaScript to prevent scrolling by preventing the default action.

[Luiz Agostini]

Created attachment 190664 [details]
patch

[Noam Rosenthal]

Comment on attachment 190664 [details]
patch

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

Looks really good, but I have a few comments :)

> Source/WebCore/page/scrolling/coordinatedgraphics/CoordinatedScrollClient.h:37
> +class CoordinatedScrollClient {
> +public:
> +    virtual void scrollLayerBy(uint32_t layerID, const FloatSize& offset) = 0;

I would make this an inner class of TextureMapperLayer and call it simply {TextureMapperLayer::]ScrollingClient.

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:407
> +FloatSize GraphicsLayerTextureMapper::resetCommitedScrollPosition()

Committed

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:573
> +    if (m_changeMask & CommitedScrollPosition)
> +        m_layer->updateCommitedScrollPosition(resetCommitedScrollPosition());

I think a better way to write this would be to have a commitScrollPosition function that internally resets and updates the TextureMapperLayer.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:680
> +    TextureMapperLayer* result = 0;
> +    for (int i = m_children.size() - 1; !result && i >= 0; --i)

You should return early here if visible/contentsVisible is false.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:62
> +    TextureMapperLayer* getTopmostLayerAt(const FloatPoint& pos);

I would leave this for a different patch

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:682
> +TextureMapperLayer* CoordinatedGraphicsScene::getScrollableContentLayerAt(const FloatPoint& point)

Content -> Contents

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.h:81
> +    CoordinatedGraphicsScene(CoordinatedGraphicsSceneClient*, CoordinatedScrollClient*);

I think the scene needs to be the scrolling client, and internally dispatch the web process update on the main thread.

> Source/WebKit2/ChangeLog:16
> +        (WKCoordinatedGetScrollableContentLayerAt):
> +        (WKCoordinatedGetLayerID):
> +        (WKCoordinatedScrollBy):

I would spell out WKCoordinatedScene

> Source/WebKit2/UIProcess/CoordinatedGraphics/CoordinatedLayerTreeHostProxy.cpp:44
> +    , m_scene(adoptRef(new CoordinatedGraphicsScene(this, this)))

This is not thread safe...
In Qt at least scrollLayerBy would be called in the wrong thread.

[Noam Rosenthal]

Comment on attachment 190664 [details]
patch

btw I would add a word in the Changelog about testing. If we're adding new testing infrastructure for this, we should mention the bug IDs for that in the changelog.

[Luiz Agostini]

Created attachment 190884 [details]
patch

[Luiz Agostini]

The plan is to start running tests in Efl soon. See bug #111140.

[Luiz Agostini]

Created attachment 190897 [details]
patch

[Helder Correia]

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:157
> +        CommittedScrollPosition =    (1L << 28),

Extra space before '('.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:702
> +static inline void round(float value, int& integerPart, float& fracPart)

What about getRoundedFloatParts(float, int*, float*) ?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:705
> +    integerPart = static_cast<int>(value >= 0 ? value + .5 : value - .5);
> +    fracPart = value - integerPart;

This will return a negative fracPart when a float has a frac between .5 and .9. Is that what you intended?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:708
> +IntSize TextureMapperLayer::getIntegerScrollOffset(FloatSize offset)

const FloatSize&

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:721
> +FloatSize TextureMapperLayer::calculateEffectiveScrollOffset(FloatSize offset)

const FloatSize&

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:735
> +    m_UISideScrollOffset += scrolloffset;

Not sure, but maybe m_uiSideScrollOffset looks more style-familiar.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:58
> +        , m_isScrollableLayer(false)

I tend to think that 'Layer' is used redundantly in a lot of places in TextureMapperLayer, GraphicsLayerTextureMapper, and CoordinatedGraphicsLayer.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:64
> +    void setLayerID(uint32_t id) { m_layerID = id; }
> +    uint32_t layerID() { return m_layerID; }

Like here ^.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:595
> +    m_layerInfo.isScrollableLayer = isScrollable();

And here ^.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.h:96
> +    WebCore::TextureMapperLayer* getScrollableContentsLayerAt(const WebCore::FloatPoint&);

Can it be declared const?

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:186
> +    typedef HashMap<WebCore::CoordinatedLayerID, WebCore::CoordinatedGraphicsLayer*> RegisteredLayersMap;

I think CoordinatedLayerMap or simply LayerMap is a more generic name for a type.

[Luiz Agostini]

Thanks!

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:702
> > +static inline void round(float value, int& integerPart, float& fracPart)
> 
> What about getRoundedFloatParts(float, int*, float*) ?

humm. I don't like it. I did something else. It is probably better.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:705
> > +    integerPart = static_cast<int>(value >= 0 ? value + .5 : value - .5);
> > +    fracPart = value - integerPart;
> 
> This will return a negative fracPart when a float has a frac between .5 and .9. Is that what you intended?

Yes.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:735
> > +    m_UISideScrollOffset += scrolloffset;
> 
> Not sure, but maybe m_uiSideScrollOffset looks more style-familiar.

uppercase UI is quite common in the code.

> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.h:96
> > +    WebCore::TextureMapperLayer* getScrollableContentsLayerAt(const WebCore::FloatPoint&);
> 
> Can it be declared const?

Unfortunately not without changing code that has not been introduced by this patch. I prefer not to do it here.

[Luiz Agostini]

Created attachment 190908 [details]
patch

[Helder Correia]

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:731
> +    FloatSize scrolloffset = calculateEffectiveScrollOffset(offset);

FloatSize scrollOffset

> Source/WebKit2/WebProcess/WebPage/CoordinatedGraphics/CoordinatedLayerTreeHost.h:187
> +    typedef HashMap<WebCore::CoordinatedLayerID, WebCore::CoordinatedGraphicsLayer*> LayersMap;
> +    LayersMap m_registeredLayers;

LayerMap would be more correct.

[Noam Rosenthal]

Comment on attachment 190908 [details]
patch

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

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:158
> +        CommittedScrollOffset =     (1L << 28),
> +        IsScrollable =              (1L << 29)

CommitedScrollOffsetChange, IsScrollableChange

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:690
> +    if (!layer->isScrollable() || !layer->m_parent || !layer->m_parent->m_parent)

Please explain the "parent" stuff with a comment (I kind of get it but it's not trivial when reading the code).

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:702
> +static inline int round(float value)

I think you already have this in MathExtras.h. Do we really need a new round function? If so, please comment what this one does differently.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:707
> +IntSize TextureMapperLayer::getIntegerScrollOffset(const FloatSize& offset)

It's strange to have side effects in a "get" function... Looks like what you're doing is adjusting the scroll to integer, and then setting the adjustment to m_pendingScrollOffset, and then you have an unadjusted and an adjusted offset. Anyway the code needs to be more explicit about that...

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:722
> +    m_currentTransform.combined().inverse().map(0, 0, zeroX, zeroY);
> +    m_currentTransform.combined().inverse().map(offset.width(), offset.height(), offsetX, offsetY);

We probably want to inverse the transform only once, as it's an expensive operation.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:64
> +    void setLayerID(uint32_t id) { m_layerID = id; }
> +    uint32_t layerID() { return m_layerID; }

I think setID / id() is fine.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:245
> +    FloatSize m_UISideScrollOffset;
> +    FloatSize m_pendingScrollOffset;

Why don't we call this m_temporaryScrollOffset? I don't really get the different between "pending" and "UISide". Is it just about the integer rounding?
Maybe it should be m_requestedScrollOffset, and m_pendingAdjustedScrollOffset.

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.h:144
> +

Not needed

[Kenneth Rohde Christiansen]

Comment on attachment 190908 [details]
patch

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

> Source/WebCore/ChangeLog:9
> +
> +        Implementing accelerated overflow scrolling for coordinated graphics.
> +        The new API WKCoordinatedScene may be used to scroll layers in the UI process.

Maybe you should explain the strategy taken here

> Source/WebCore/ChangeLog:24
> +        * page/scrolling/coordinatedgraphics/ScrollingCoordinatorCoordinatedGraphics.h:
> +        (ScrollingCoordinatorCoordinatedGraphics):
> +        * platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:
> +        (WebCore::GraphicsLayerTextureMapper::GraphicsLayerTextureMapper):
> +        (WebCore::GraphicsLayerTextureMapper::commitScrollOffset):
> +        (WebCore):
> +        (WebCore::GraphicsLayerTextureMapper::setIsScrollableLayer):
> +        (WebCore::GraphicsLayerTextureMapper::commitLayerChanges):
> +        (WebCore::GraphicsLayerTextureMapper::commitLayerScrollOffset):
> +        * platform/graphics/texmap/GraphicsLayerTextureMapper.h:

This section would need more comments to individual changes

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:410
> +{
> +    if (offset.isZero())
> +        return;

Is this supposed to happen? or should we assert as well?

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:578
> +void GraphicsLayerTextureMapper::commitLayerScrollOffset()
> +{
> +    m_layer->commitScrollOffset(m_committedScrollOffset);
> +    m_committedScrollOffset = IntSize();
> +}

this is a function which commits something called "committed". Weird

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:116
>  
> +    void commitLayerScrollOffset();
> +

So this is GraphicsLAYERtexturemapper which has a commitLAYERScrollOffset and a commitScrollOffset. The difference is not obvious

> Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.h:181
> +    IntSize m_committedScrollOffset;

pending?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:676
> +template<class SearchCondition> TextureMapperLayer* TextureMapperLayer::hitTestSearch(const FloatPoint& point, SearchCondition condition)

So it is a "test" and a "search". Is this terminology used elsewhere?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:683
> +
> +    TextureMapperLayer* result = 0;
> +    for (int i = m_children.size() - 1; !result && i >= 0; --i)
> +        result = m_children[i]->hitTestSearch(point, condition);

why not check the size first?

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:707
>> +IntSize TextureMapperLayer::getIntegerScrollOffset(const FloatSize& offset)
> 
> It's strange to have side effects in a "get" function... Looks like what you're doing is adjusting the scroll to integer, and then setting the adjustment to m_pendingScrollOffset, and then you have an unadjusted and an adjusted offset. Anyway the code needs to be more explicit about that...

integer?

pixelAlignedScrollOffset? or what are you trying to accomplish here

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:732
> +    m_UISideScrollOffset += scrolloffset;

scrollOffset.

I dont kike the UISideSCrollOffset... also it should be m_uiScrollOffset if anything

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:245
>> +    FloatSize m_pendingScrollOffset;
> 
> Why don't we call this m_temporaryScrollOffset? I don't really get the different between "pending" and "UISide". Is it just about the integer rounding?
> Maybe it should be m_requestedScrollOffset, and m_pendingAdjustedScrollOffset.

current?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:323
> +    toGraphicsLayerTextureMapper(layer)->commitScrollOffset(layerInfo.committedScrollOffset);
> +    toGraphicsLayerTextureMapper(layer)->setIsScrollable(layerInfo.isScrollable);

shouldnt you set isScrollable first?

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:619
>  
> +void CoordinatedGraphicsScene::doScrollLayerBy(uint32_t layerID, const IntSize& offset)
> +{

do* functions are not really used in webkit, I believe

> Source/WebKit2/UIProcess/API/CoordinatedGraphics/WKCoordinatedScene.h:42
> +
> +WK_EXPORT WKCoordinatedSceneLayer WKCoordinatedSceneGetScrollableContentsLayerAt(WKCoordinatedScene, WKPoint);
> +WK_EXPORT uint32_t WKCoordinatedSceneGetLayerID(WKCoordinatedSceneLayer);
> +WK_EXPORT void WKCoordinatedSceneScrollBy(WKCoordinatedSceneLayer, WKSize);

So long term, how is this going to work? Are we going to know which area of the scene subscribes to wheel and touch events? so we know when to do the fast path (ui side scrolling) or whether we need to send events to the web process first (slow path) ?

[Luiz Agostini]

> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:707
> > +IntSize TextureMapperLayer::getIntegerScrollOffset(const FloatSize& offset)
> 
> It's strange to have side effects in a "get" function... Looks like what you're doing is adjusting the scroll to integer, and then setting the adjustment to m_pendingScrollOffset, and then you have an unadjusted and an adjusted offset. Anyway the code needs to be more explicit about that...

I agree. I have made some renaming and a small change in this specific part of the code. I hope it is better.

> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:245
> > +    FloatSize m_UISideScrollOffset;
> > +    FloatSize m_pendingScrollOffset;
> 
> Why don't we call this m_temporaryScrollOffset? I don't really get the different between "pending" and "UISide". Is it just about the integer rounding?
> Maybe it should be m_requestedScrollOffset, and m_pendingAdjustedScrollOffset.

UI here is a reference to UI process itself. It is the offset that is been applied on UI process but that web process does not yet know about.
pending is the fractionary difference between m_UISideScrollOffset and what has already been notified to web process (pending because it has not already been notified). Yes, just about rounding.

[Luiz Agostini]

> > Source/WebCore/platform/graphics/texmap/GraphicsLayerTextureMapper.cpp:410
> > +{
> > +    if (offset.isZero())
> > +        return;
> 
> Is this supposed to happen? or should we assert as well?

Yes, it may happen. We should not assert. A layer sync message may have no relation to scroll.
> 
> > Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:683
> > +
> > +    TextureMapperLayer* result = 0;
> > +    for (int i = m_children.size() - 1; !result && i >= 0; --i)
> > +        result = m_children[i]->hitTestSearch(point, condition);
> 
> why not check the size first?

Sorry. I think I did not get the question.
> 
> > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsScene.cpp:323
> > +    toGraphicsLayerTextureMapper(layer)->commitScrollOffset(layerInfo.committedScrollOffset);
> > +    toGraphicsLayerTextureMapper(layer)->setIsScrollable(layerInfo.isScrollable);
> 
> shouldnt you set isScrollable first?

I think that it would make no practical difference. But done anyway.

I have made some renaming all over the code that I hope will make it a bit more clear.

> 
> > Source/WebKit2/UIProcess/API/CoordinatedGraphics/WKCoordinatedScene.h:42
> > +
> > +WK_EXPORT WKCoordinatedSceneLayer WKCoordinatedSceneGetScrollableContentsLayerAt(WKCoordinatedScene, WKPoint);
> > +WK_EXPORT uint32_t WKCoordinatedSceneGetLayerID(WKCoordinatedSceneLayer);
> > +WK_EXPORT void WKCoordinatedSceneScrollBy(WKCoordinatedSceneLayer, WKSize);
> 
> So long term, how is this going to work? Are we going to know which area of the scene subscribes to wheel and touch events? so we know when to do the fast path (ui side scrolling) or whether we need to send events to the web process first (slow path) ?

Yes, the plan is to have more information on UI side to be able to take this kind of decision. For now it is just about scrolling itself, but there is some more stuff in the queue.

[Luiz Agostini]

Created attachment 191173 [details]
patch

[Luiz Agostini]

Created attachment 191181 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 191181 [details]
patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:66
> +    TextureMapperLayer* getScrollableContentsLayerAt(const FloatPoint& pos);

If it can be null, I would call it "find" instead of "get"

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.h:186
> +    FloatSize mapScrollOffset(const FloatSize&);

Sometimes we use map, sometimes convertTo and sometimes to... please check what is more prevalent in webkit source

[Luiz Agostini]

Created attachment 191409 [details]
patch

[Kenneth Rohde Christiansen]

Comment on attachment 191409 [details]
patch

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

> Source/WebKit2/UIProcess/API/CoordinatedGraphics/WKCoordinatedScene.h:42
> +WK_EXPORT WKCoordinatedSceneLayer WKCoordinatedSceneFindScrollableContentsLayerAt(WKCoordinatedScene, WKPoint);
> +WK_EXPORT uint32_t WKCoordinatedSceneGetLayerID(WKCoordinatedSceneLayer);
> +WK_EXPORT void WKCoordinatedSceneScrollBy(WKCoordinatedSceneLayer, WKSize);

There is some confusion here about Scene and SceneLayer... functions working on SceneLayer are called Scene only

What does "Coordinated" bring to the user? Why not just WKLayerScrollBy or WKContentLayerScrollBy ?

[Kenneth Rohde Christiansen]

Comment on attachment 191409 [details]
patch

Looks OK now you have explained me the next steps... can you get an owner to take a look?

[Noam Rosenthal]

(In reply to comment #38)
> (From update of attachment 191409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191409&action=review
> 
> > Source/WebKit2/UIProcess/API/CoordinatedGraphics/WKCoordinatedScene.h:42
> > +WK_EXPORT WKCoordinatedSceneLayer WKCoordinatedSceneFindScrollableContentsLayerAt(WKCoordinatedScene, WKPoint);
> > +WK_EXPORT uint32_t WKCoordinatedSceneGetLayerID(WKCoordinatedSceneLayer);
> > +WK_EXPORT void WKCoordinatedSceneScrollBy(WKCoordinatedSceneLayer, WKSize);
> 
> There is some confusion here about Scene and SceneLayer... functions working on SceneLayer are called Scene only
> 
> What does "Coordinated" bring to the user? Why not just WKLayerScrollBy or WKContentLayerScrollBy ?

It's a namespacing thing. There are too many layers around; WKLayer might be too ambiguous.

[Allan Sandfeld Jensen]

Comment on attachment 191409 [details]
patch

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

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:684
> +        result = m_children[i]->hitTest(point, condition);

Why continue after the first hit?

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:686
> +    return result ? result : condition(this, point) ? this : 0;

That is little hard to read. I think I would prefer only a single ?: in an expression.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:697
> +    return parentLayer->m_currentTransform.combined().mapRect(parentLayer->m_state.contentsRect).contains(point);

If the transformation is not rectilinear, mapRect will just give you the bounding box. You need to use mapQuad.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:725
> +    // m_accumulatedScrollOffsetFractionalPart holds the fractional part of the user scroll offset that
> +    // has not yet been synced with the web process because the web process expects an IntSize.
> +    m_accumulatedScrollOffsetFractionalPart = FloatSize(fullOffset.width() - intWidth, fullOffset.height() - intHeight);
> +
> +    m_scrollClient->commitScrollOffset(m_id, IntSize(intWidth, intHeight));

Why not just send the FloatSize across?

> Source/WebKit2/UIProcess/API/CoordinatedGraphics/WKCoordinatedScene.cpp:39
> +WK_EXPORT WKCoordinatedSceneLayer WKCoordinatedSceneFindScrollableContentsLayerAt(WKCoordinatedScene scene, WKPoint point)
> +{
> +    return toAPI(toImpl(scene)->findScrollableContentsLayerAt(WebCore::FloatPoint(point.x, point.y)));
> +}

Is this hittest code path in use yet? I can't seem to find where it is called.

[Simon Fraser (smfr)]

Comment on attachment 191409 [details]
patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2327
> +    settings->setAcceleratedCompositingForOverflowScrollEnabled(store.getBoolValueForKey(WebPreferencesKey::acceleratedCompositingForOverflowScrollEnabledKey()));

Looks like you're enabling this for everyone here, which is wrong.

[Simon Fraser (smfr)]

Comment on attachment 191409 [details]
patch

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

The WebKit2 bits look OK to me.

>> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:2327
>> +    settings->setAcceleratedCompositingForOverflowScrollEnabled(store.getBoolValueForKey(WebPreferencesKey::acceleratedCompositingForOverflowScrollEnabledKey()));
> 
> Looks like you're enabling this for everyone here, which is wrong.

Never mind.

[Luiz Agostini]

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

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:684
>> +        result = m_children[i]->hitTest(point, condition);
> 
> Why continue after the first hit?

It does not. It breaks the for if the result is not zero.

>> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:725
>> +    m_scrollClient->commitScrollOffset(m_id, IntSize(intWidth, intHeight));
> 
> Why not just send the FloatSize across?

An IntSize is expected because of ScrollableArea::notifyScrollPositionChanged.

>> Source/WebKit2/UIProcess/API/CoordinatedGraphics/WKCoordinatedScene.cpp:39
>> +}
> 
> Is this hittest code path in use yet? I can't seem to find where it is called.

This is API to be used outside of webkit. Tests are comming in bug #111140.

[Allan Sandfeld Jensen]

(In reply to comment #44)
> >> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:684
> >> +        result = m_children[i]->hitTest(point, condition);
> > 
> > Why continue after the first hit?
> 
> It does not. It breaks the for if the result is not zero.
Of course, my mistake.

> > Is this hittest code path in use yet? I can't seem to find where it is called.
> This is API to be used outside of webkit. Tests are comming in bug #111140.

Is the API a temporary one to be able to test the current state of the code, or is it the actual API you plan to export? It seems to me it might not be ideal once we finish more of the implementation of accelerated overflow scrolling. For one thing a layer might not always be scrollable, or might be scrollable but not in the UIProcess.

[Noam Rosenthal]

(In reply to comment #45)

> Is the API a temporary one to be able to test the current state of the code, or is it the actual API you plan to export? 
How this is exported should be port specific. But we want a way to test some of the underlying features without dealing with port-specific APIs.

> For one thing a layer might not always be scrollable, or might be scrollable but not in the UIProcess.
With composited overflow scrolling and coordinated graphics enabled, the layer is in the UI process.
When it's not scrollable, setting its scroll position should have no effect.
This is no different from RenderLayer being a subclass of ScrollableArea.

[Luiz Agostini]

> > > Is this hittest code path in use yet? I can't seem to find where it is called.
> > This is API to be used outside of webkit. Tests are comming in bug #111140.
> 
> Is the API a temporary one to be able to test the current state of the code, or is it the actual API you plan to export? It seems to me it might not be ideal once we finish more of the implementation of accelerated overflow scrolling.

I consider this is part of the actual API. There are missing parts though. For example we are working now on passing to the UI side information about the regions that are touch event targets for it to be used together with this patch. There is more stuff in the queue.

The idea is to have an API that is port independent (that depends only on coordinated graphics), properly implemented and tested. More stuff will probably be added in future. Of course the API might change a bit in future, if needed.

> For one thing a layer might not always be scrollable, or might be scrollable but not in the UIProcess.

Yes, indeed. WKCoordinatedSceneFindScrollableContentsLayerAt will return 0 if there is no scrollable layer at the given point. The information about if the layer is scrollable is been kept always up to date in UI side.

I am preparing a patch that addresses your other comments. Thanks!

[Allan Sandfeld Jensen]

(In reply to comment #46)
>With composited overflow scrolling and coordinated graphics enabled, the layer is in the UI process.
> When it's not scrollable, setting its scroll position should have no effect.
> This is no different from RenderLayer being a subclass of ScrollableArea.

It is more complicated than that. If you have a scrollable layer that is 10 px from the top and you send mouse wheel for 30px upwards; the layer scrolls 10px up and its parent layer scrolls 20px up.

Second if the layer is not fast scrollable, it needs to be scrolled in webprocess, and this means either blocking, having an asynchronous API or simply giving up and telling the caller to scroll another way.

[Luiz Agostini]

(In reply to comment #46)
> (In reply to comment #45)
> 
> > Is the API a temporary one to be able to test the current state of the code, or is it the actual API you plan to export? 
> How this is exported should be port specific. But we want a way to test some of the underlying features without dealing with port-specific APIs.

Of course each port may decide about using this API or not. I hope that it is good enough for the ports to consider using it.

[Luiz Agostini]

(In reply to comment #48)
> (In reply to comment #46)
> >With composited overflow scrolling and coordinated graphics enabled, the layer is in the UI process.
> > When it's not scrollable, setting its scroll position should have no effect.
> > This is no different from RenderLayer being a subclass of ScrollableArea.
> 
> It is more complicated than that. If you have a scrollable layer that is 10 px from the top and you send mouse wheel for 30px upwards; the layer scrolls 10px up and its parent layer scrolls 20px up.

The idea is to provide the information for the user of the API to take these decisions but create as few restrictions as possible. In your example, the user of the API should have the information about the contents size and viweport size and decide what to do, but if he decides to scroll 30px then we scroll.

As I said the API is not complete. I would like to add the files and create first tests so that our people could continue working on it.

> 
> Second if the layer is not fast scrollable, it needs to be scrolled in webprocess, and this means either blocking, having an asynchronous API or simply giving up and telling the caller to scroll another way.

What we did is pretty similar to 'giving up and telling the caller to scroll another way' because no scrollable layer will be found on UI side.

[Allan Sandfeld Jensen]

(In reply to comment #47)
> Yes, indeed. WKCoordinatedSceneFindScrollableContentsLayerAt will return 0 if there is no scrollable layer at the given point. The information about if the layer is scrollable is been kept always up to date in UI side.
> 
My point was more that what scrollable layer should be returned might depend on what direction you need to scroll in, what event is triggering it and what the scrolling constraints are on the layer. So the hitTest API and scrollBy methods may need to look different. Besides my split scroll example, an easier example is a layer scrolled to all the way to one of its constraint, so it is no longer scrollable in that direction but its parent layer is. Or the bad case with overflow scroll touch, where it is scrollable beyond the constraint but only by touch.

> I am preparing a patch that addresses your other comments. Thanks!

Cool :)

[Luiz Agostini]

(In reply to comment #51)
> (In reply to comment #47)
> > Yes, indeed. WKCoordinatedSceneFindScrollableContentsLayerAt will return 0 if there is no scrollable layer at the given point. The information about if the layer is scrollable is been kept always up to date in UI side.
> > 
> My point was more that what scrollable layer should be returned might depend on what direction you need to scroll in, what event is triggering it and what the scrolling constraints are on the layer. So the hitTest API and scrollBy methods may need to look different. Besides my split scroll example, an easier example is a layer scrolled to all the way to one of its constraint, so it is no longer scrollable in that direction but its parent layer is. Or the bad case with overflow scroll touch, where it is scrollable beyond the constraint but only by touch.

I understand.

I see this API as lower level. I would like it to provide the means to implement the behavior that you described, like sizes, parents, positions, etc, but I don't think that it should implement the behavior itself.

I think that issues like how to scroll, which layer to scroll depending on the kind of event, which scroll constraint to apply in each case, etc, might be even more than port specific, they might be application specific.

[Luiz Agostini]

Created attachment 191654 [details]
patch

[Noam Rosenthal]

Comment on attachment 191654 [details]
patch

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

Apart from the contentsRect issue I'm ok with this.
Allan, I believe that some of your concerns/suggestions can be addressed later on top of this, they're really more about advanced cases like wheel events and scrolling layer inside scrolling layer. Like we do with scrolling of the main page, I would like to first support the common use cases and then iterate towards more advanced cases. If you don't have more objections I would like to move forward.

> Source/WebCore/platform/graphics/texmap/TextureMapperLayer.cpp:701
> +    return parentLayer->m_currentTransform.combined().mapQuad(parentLayer->m_state.contentsRect).containsPoint(point);

Actually, it's wrong to use contentsRect here.
We should use the entire layerRect() if there is a m_backingStore, and contentsRect if there is an m_contentsLayer.

[Allan Sandfeld Jensen]

(In reply to comment #54)
> (From update of attachment 191654 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191654&action=review
> 
> Apart from the contentsRect issue I'm ok with this.
> Allan, I believe that some of your concerns/suggestions can be addressed later on top of this, they're really more about advanced cases like wheel events and scrolling layer inside scrolling layer. Like we do with scrolling of the main page, I would like to first support the common use cases and then iterate towards more advanced cases. If you don't have more objections I would like to move forward.
> 
I am okay with that. I was only going to suggest making scrollby return how much it has scrolled and a method to get the parent scrollable layer, so you could ask scrollable layers one by one to scroll, but we can improve that later.

[Luiz Agostini]

Created attachment 191702 [details]
patch

[Luiz Agostini]

Created attachment 191707 [details]
patch

[WebKit Review Bot]

Comment on attachment 191707 [details]
patch

Clearing flags on attachment: 191707

Committed r144907: <http://trac.webkit.org/changeset/144907>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.