Bug 98457 – ScrollingStateNodes should be referenced via IDs on RenderLayerBacking

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

The next step to getting the ScrollingTree into shape where it can take multiple nodes is to associate RenderLayerBackings with ScrollingStateNodes.

[Beth Dakin]

Created attachment 167191 [details]
Patch

[Early Warning System Bot]

Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14152687

[Peter Beverloo (cr-android ews)]

Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14171445

[Gyuyoung Kim]

Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14173327

[Early Warning System Bot]

Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14183158

[Build Bot]

Comment on attachment 167191 [details]
Patch

Attachment 167191 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14176279

[James Robinson]

Do you have to make the caller of ScrollingCoordinator aware of the ScrollingStateNodes?  I thought it was the ScrollingCoordinator's job to deal with these mappings.

[Beth Dakin]

(In reply to comment #7)
> Do you have to make the caller of ScrollingCoordinator aware of the ScrollingStateNodes?  I thought it was the ScrollingCoordinator's job to deal with these mappings.

I am trying to keep it mostly internal to ScrollingCoordinator. The actual ScrollingStateNodes are definitely contained within the class still. Is there something in particular that you think violates that?

[Beth Dakin]

(In reply to comment #7)
> Do you have to make the caller of ScrollingCoordinator aware of the ScrollingStateNodes?  I thought it was the ScrollingCoordinator's job to deal with these mappings.

Maybe you are referring to these functions:

    void detachFromStateTree(RenderLayerBacking*);
    void clearStateTree();

Perhaps it would be more appropriate to re-name those so that they don't reference the state tree.

[Beth Dakin]

Created attachment 167215 [details]
Patch that should build

AKA ERMAHGERD THE ERF DERFS

Maybe there is a better way to get this building, but ScrollingCoordinator already seems like such a mess with the if-defs that I'm not really sure what the best choice is.

[Peter Beverloo (cr-android ews)]

Comment on attachment 167215 [details]
Patch that should build

Attachment 167215 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14175308

[Simon Fraser (smfr)]

Comment on attachment 167215 [details]
Patch that should build

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

> Source/WebCore/dom/Document.cpp:4064
>              v->cacheCurrentScrollPosition();
> -            if (page() && page()->mainFrame() == m_frame)
> +            if (page && page->mainFrame() == m_frame) {
>                  v->resetScrollbarsAndClearContentsSize();
> -            else
> +                if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                    scrollingCoordinator->clearStateTree();
> +            } else

Maybe this should be done in a separate patch? This patch is big enough already.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:255
> +    // ensureRootStateNodeForFrameView() can fail if, for example, the FrameView does not have a
> +    // RenderLayerBacking. If that is the case, then we should not do any more work here.

The comment would be more useful if it said when this might happen (to allow a developer to reproduce the scenario).

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:462
> +static RenderLayerBacking* backingForFrameView(FrameView* frameView)

I'm not sure why SC ever needs to know about RLB.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:503
> +void ScrollingCoordinator::detachFromStateTree(RenderLayerBacking* backing)

Why not just pass the ID in here?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:549
> +    if (!rootLayerID) {
> +        rootLayerID = generateScrollLayerID();
> +        backing->setScrollLayerID(rootLayerID);
> +    }

I think this code should live in RLB.

> Source/WebCore/rendering/RenderLayer.cpp:4462
> +        if (isRootLayer() && compositor()->scrollLayer()) {
> +            if (Frame* frame = renderer()->frame()) {
> +                if (Page* page = frame->page()) {
> +                    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                        scrollingCoordinator->frameViewRootLayerDidChange(frame->view());
> +                }
> +            }
> +        }

Seems like an odd place to put this. I think we should try to keep all of the calls into SC in RLC.

[Simon Fraser (smfr)]

http://trac.webkit.org/changeset/130439

[James Robinson]

ScrollingCoordinator is really kind of a mess.  What would you think about making the interface just a vtable and separating the implementation bits into separate subclasses instead of one implemenation that's all sliced up to hell?

[Beth Dakin]

(In reply to comment #13)
> http://trac.webkit.org/changeset/130439

Simon, did you mean to paste this somewhere else?

[Beth Dakin]

(In reply to comment #14)
> ScrollingCoordinator is really kind of a mess.  What would you think about making the interface just a vtable and separating the implementation bits into separate subclasses instead of one implemenation that's all sliced up to hell?

I definitely agree that ScrollingCoordinator is a huge mess. I didn't realize how bad it was until I wrote this patch. I don't love the idea of virtualization here because we would be adding run-time cost for something that is known at compile-time. This may not actually be a performance-critical area, but I still think it's a best-practice that is worth sticking to.

What could work instead would be to make ScrollingCoordinator.h a very small file that looks something like this:

#if PLATFORM(MAC)
#include "ScrollingCoordinatorMac.h"
typedef ScrollingCoordinatorMac ScrollingCoordinator;
#endif

#if PLATFORM(CHROMIUM)
#include "ScrollingCoordinatorChromium.h"
typedef ScrollingCoordiantorChromium ScrollingCoordinator;
#endif

// Other platforms here.

And then ScrollingCoordinatorMac and ScrollingCoordinatorChromium could inherit from a ScrollingCoordinatorBase that could contain any legitimately shared functionality.

At this point, I do think something like that would be greatly preferable to this rat's nest, but some of the forking is unfortunate. I guess we're already in that state though.

I'm not going to focus on this re-structuring for this patch because it will be a big task on its own. But I do think we should do this.

[Beth Dakin]

(In reply to comment #12)
> (From update of attachment 167215 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167215&action=review
> 
> > Source/WebCore/dom/Document.cpp:4064
> >              v->cacheCurrentScrollPosition();
> > -            if (page() && page()->mainFrame() == m_frame)
> > +            if (page && page->mainFrame() == m_frame) {
> >                  v->resetScrollbarsAndClearContentsSize();
> > -            else
> > +                if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> > +                    scrollingCoordinator->clearStateTree();
> > +            } else
> 
> Maybe this should be done in a separate patch? This patch is big enough already.
> 

This code is required in this patch because this patch also makes it so ScrollingStateTree does not automatically create a root node upon its own creation. With that change in place, this code is required to avoid crashing. Both of these things could technically be done in another patch, but there would have to be some weird temporary code that accounts for the fact that we would always creat root state node that is not ready to be ScrollingCoordinator's HashMap since it hasn't been associated with a FrameView. I don't think it's the right trade-off to do this work in a separate patch, so I'm going to leave it in for now.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:255
> > +    // ensureRootStateNodeForFrameView() can fail if, for example, the FrameView does not have a
> > +    // RenderLayerBacking. If that is the case, then we should not do any more work here.
> 
> The comment would be more useful if it said when this might happen (to allow a developer to reproduce the scenario).
> 

I changed this into an assertion since that is the most accurate comment.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:462
> > +static RenderLayerBacking* backingForFrameView(FrameView* frameView)
> 
> I'm not sure why SC ever needs to know about RLB.
> 

I changed this so that it does not.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:503
> > +void ScrollingCoordinator::detachFromStateTree(RenderLayerBacking* backing)
> 
> Why not just pass the ID in here?
> 

Fixed.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:549
> > +    if (!rootLayerID) {
> > +        rootLayerID = generateScrollLayerID();
> > +        backing->setScrollLayerID(rootLayerID);
> > +    }
> 
> I think this code should live in RLB.
> 

Moved it.

> > Source/WebCore/rendering/RenderLayer.cpp:4462
> > +        if (isRootLayer() && compositor()->scrollLayer()) {
> > +            if (Frame* frame = renderer()->frame()) {
> > +                if (Page* page = frame->page()) {
> > +                    if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> > +                        scrollingCoordinator->frameViewRootLayerDidChange(frame->view());
> > +                }
> > +            }
> > +        }
> 
> Seems like an odd place to put this. I think we should try to keep all of the calls into SC in RLC.

Okay, I moved it back to RLC, and I put it after RLC's call to ensureBacking() so that we can be sure we have a backing.

Simon, I am re-opening this bug because I don't think you meant to close it.

[Beth Dakin]

Created attachment 167487 [details]
Patch

[Gyuyoung Kim]

Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/14205437

[Early Warning System Bot]

Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14204434

[Early Warning System Bot]

Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14205438

[WebKit Review Bot]

Comment on attachment 167487 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14207331

[Build Bot]

Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/14217137

[Peter Beverloo (cr-android ews)]

Comment on attachment 167487 [details]
Patch

Attachment 167487 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14211206

[Beth Dakin]

Created attachment 167496 [details]
Patch

[Build Bot]

Comment on attachment 167496 [details]
Patch

Attachment 167496 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/14204509

[WebKit Review Bot]

Comment on attachment 167496 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 167496 [details]
Patch

Attachment 167496 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14214200

[Beth Dakin]

Created attachment 167570 [details]
Patch

[Peter Beverloo (cr-android ews)]

Comment on attachment 167570 [details]
Patch

Attachment 167570 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14211442

[Beth Dakin]

Created attachment 167577 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167577 [details]
Patch

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

[Peter Beverloo (cr-android ews)]

Comment on attachment 167577 [details]
Patch

Attachment 167577 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/14229209

[Beth Dakin]

Created attachment 167592 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 167592 [details]
Patch

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

> Source/WebCore/loader/HistoryController.cpp:131
> +        Page* page = m_frame->page();
> +        if (page && page->mainFrame() == m_frame) {
> +            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                scrollingCoordinator->frameViewRootLayerDidChange(view);
> +        }

This is a bit mysterious here, and it's a shame to call into ScrollingCoordinator from yet another file. What work does it need to do?

> Source/WebCore/page/FrameView.h:137
> +    uint64_t scrollLayerID() const;

Does this have to be public on FrameView? Can't FrameView pass its own ID into the SC?

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:153
> +

Blanky.

> Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:473
> +    ASSERT(node->isScrollingStateScrollingNode());
> +    return toScrollingStateScrollingNode(node);

toScrollingStateScrollingNode will ASSERT for you; no need to ASSERT here too.

> Source/WebCore/page/scrolling/ScrollingStateNode.cpp:92
> +    size_t size = m_children->size();
> +    for (size_t i = 0; i < size; ++i)
> +        m_children->at(i)->removeChild(node);

Looks like you're iterating the array while mutating it here.

> Source/WebCore/rendering/RenderLayerBacking.cpp:972
> +static uint64_t generateScrollLayerID()
> +{
> +    static uint64_t uniqueScrollLayerID = 1;
> +    return uniqueScrollLayerID++;
> +}

I think the ID generation should be on ScrollingCoordinator. Calling could look like:

m_id = scrollingCoordinator->attach(scrollingCoordinator->uniqueNodeID(), someConstraintsData)

where attach returns the ID passed in, for convenience.

> Source/WebCore/rendering/RenderLayerBacking.cpp:978
> +void RenderLayerBacking::attachToScrollingCoordinator()
> +{
> +    if (!m_scrollLayerID)
> +        m_scrollLayerID = generateScrollLayerID();
> +}

But this isn't attaching, it's just making an ID.

[Beth Dakin]

(In reply to comment #36)
> (From update of attachment 167592 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=167592&action=review
> 
> > Source/WebCore/loader/HistoryController.cpp:131
> > +        Page* page = m_frame->page();
> > +        if (page && page->mainFrame() == m_frame) {
> > +            if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> > +                scrollingCoordinator->frameViewRootLayerDidChange(view);
> > +        }
> 
> This is a bit mysterious here, and it's a shame to call into ScrollingCoordinator from yet another file. What work does it need to do?
> 

This creates a root state node for the cached page. There is none otherwise because the root state node for the previous page was destroyed when its FrameView and backing went away.

> > Source/WebCore/page/FrameView.h:137
> > +    uint64_t scrollLayerID() const;
> 
> Does this have to be public on FrameView? Can't FrameView pass its own ID into the SC?
> 

It definitely does need to be public. ScrollingCoordinator needs to be able to access this ID whenever it has messages to send to the scrolling thread.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:153
> > +
> 
> Blanky.
> 

Fixed.

> > Source/WebCore/page/scrolling/ScrollingCoordinator.cpp:473
> > +    ASSERT(node->isScrollingStateScrollingNode());
> > +    return toScrollingStateScrollingNode(node);
> 
> toScrollingStateScrollingNode will ASSERT for you; no need to ASSERT here too.
> 

Fixed.

> > Source/WebCore/page/scrolling/ScrollingStateNode.cpp:92
> > +    size_t size = m_children->size();
> > +    for (size_t i = 0; i < size; ++i)
> > +        m_children->at(i)->removeChild(node);
> 
> Looks like you're iterating the array while mutating it here.
> 

Hmm, kind of. I don't think this is actually dangerous. We NOT mutating the array we are iterating. We are potentially mutating the array that belongs to one of the objects in the array that we are iterating, if that makes sense.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:972
> > +static uint64_t generateScrollLayerID()
> > +{
> > +    static uint64_t uniqueScrollLayerID = 1;
> > +    return uniqueScrollLayerID++;
> > +}
> 
> I think the ID generation should be on ScrollingCoordinator. Calling could look like:
> 
> m_id = scrollingCoordinator->attach(scrollingCoordinator->uniqueNodeID(), someConstraintsData)
> 
> where attach returns the ID passed in, for convenience.
> 

Okay. Your last comments made me think you wanted this code on RLB, so I move it there. I see what you want now. That is fine.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:978
> > +void RenderLayerBacking::attachToScrollingCoordinator()
> > +{
> > +    if (!m_scrollLayerID)
> > +        m_scrollLayerID = generateScrollLayerID();
> > +}
> 
> But this isn't attaching, it's just making an ID.

Do you have another name suggestion? I was trying to avoid being overly-explict about the state tree since we are trying to keep that internal to ScrollingCoordinator, so I chose attachToScrollingCoordinator(). We could do something like addToScrollLayerHashMap().

[Beth Dakin]

Created attachment 167624 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 167624 [details]
Patch

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

> Source/WebCore/dom/Document.cpp:4063
>              v->cacheCurrentScrollPosition();
> -            if (page() && page()->mainFrame() == m_frame)
> +            if (page && page->mainFrame() == m_frame) {
>                  v->resetScrollbarsAndClearContentsSize();
> -            else
> +                if (ScrollingCoordinator* scrollingCoordinator = page->scrollingCoordinator())
> +                    scrollingCoordinator->clearStateTree();

Let's file a bug to clean up the page cache save/restore and make it more symmetrical.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:144
> +    uint64_t attachToStateTree(uint64_t);

It's still odd that attach() isn't really attaching anything; it's making a node in the tree that will be filled in later. Can we just create the node with a new ID when we "fill in" that node later?

I think we should have a typedef ("ScrollingNodeID") for the uint64_t too.

[Beth Dakin]

Created attachment 167674 [details]
Patch

[WebKit Review Bot]

Comment on attachment 167674 [details]
Patch

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

New failing tests:
ScrollingCoordinatorChromiumTest.wheelEventHandler

[James Robinson]

(In reply to comment #41)
> (From update of attachment 167674 [details])
> Attachment 167674 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/14209754
> 
> New failing tests:
> ScrollingCoordinatorChromiumTest.wheelEventHandler

You plan to fix this before landing, right?

[Beth Dakin]

(In reply to comment #42)
> (In reply to comment #41)
> > (From update of attachment 167674 [details] [details])
> > Attachment 167674 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/14209754
> > 
> > New failing tests:
> > ScrollingCoordinatorChromiumTest.wheelEventHandler
> 
> You plan to fix this before landing, right?

I believe I know how to fix this, yes. And I have that fix in place. I'm going to land and watch the bots.

[Beth Dakin]

http://trac.webkit.org/changeset/130783

[Beth Dakin]

This caused https://bugs.webkit.org/show_bug.cgi?id=98968

[Beth Dakin]

This also caused https://bugs.webkit.org/show_bug.cgi?id=98984