Bug 99211 - When ScrollingStateNodes are destroyed, they should be removed ScrollingCoordinator's HashMap
Summary: When ScrollingStateNodes are destroyed, they should be removed ScrollingCoord...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Beth Dakin
Depends on:
Reported: 2012-10-12 14:44 PDT by Beth Dakin
Modified: 2012-10-12 16:52 PDT (History)
5 users (show)

See Also:

Patch (5.89 KB, patch)
2012-10-12 14:51 PDT, Beth Dakin
no flags Details | Formatted Diff | Diff
Patch (6.07 KB, patch)
2012-10-12 16:49 PDT, Beth Dakin
sam: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2012-10-12 14:44:53 PDT
There is a FIXME in ScrollingCoordinatorMac::detachFromStateTree() that says:

    // FIXME: removeNode() will destroy children, and those children might still be in the HashMap.
    // This will be a big problem once there are actually children in the tree.

This bugs is to track resolving that FIXME.
Comment 1 Beth Dakin 2012-10-12 14:51:52 PDT
Created attachment 168489 [details]
Comment 2 Brady Eidson 2012-10-12 16:25:00 PDT
Comment on attachment 168489 [details]

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

> Source/WebCore/page/scrolling/ScrollingStateTree.cpp:58
> +    // Copy and clear the array of node IDs representing nodes that have been removed since the last
> +    // time we committed the tree.
> +    size_t size = m_nodesRemovedSinceLastCommit->size();
> +    for (size_t i = 0; i < size; ++i)
> +        treeState->didRemoveNode(m_nodesRemovedSinceLastCommit->at(i));
> +    m_nodesRemovedSinceLastCommit->clear();

I don't see where the copy is that the comment alludes to.

I might be missing something here:  doesn't didRemoveNode() just call back in to the same m_nodesRemovedSinceLastCommit?
Comment 3 Beth Dakin 2012-10-12 16:49:28 PDT
Created attachment 168513 [details]
Comment 4 Beth Dakin 2012-10-12 16:52:01 PDT
Thanks Sam and Brady! http://trac.webkit.org/changeset/131238