[Webkit-unassigned] [Bug 109560] Implement coordinated scrollbar for subframes and sublayers

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=109560





--- Comment #15 from Sami Kyöstilä <skyostil at chromium.org>  2013-02-14 03:46:55 PST ---
(From update of attachment 188261)
I think overall this is a good simplification, but I'd appreciate some test coverage if possible.

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

> Source/WebCore/ChangeLog:22
> +        No new tests. Don't have a good way to test ScrollingCoordinator at this time.

We've got ScrollingCoordinatorChromiumTest unit tests in Source/WebKit/chromium/tests/. Maybe some of the Chromium parts could be covered there?

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:151
> +    virtual void scrollableAreaDestroyed(ScrollableArea*) { }

I think the role of ScrollingCoordinator is getting a bit weird here since on one hand it interrogates FrameView for the known ScrollableAreas, but here we're explicitly pushing information about the lifetimes of ScrollableArea to it. Maybe we could instead reuse scrollableAreaScrollbarLayerDidChange for this so that it would forget about the ScrollableArea if it didn't have any scrollbars? That would be similar to how scrollableAreaScrollLayerDidChange() works.

> Source/WebCore/page/scrolling/ScrollingCoordinator.h:156
>      virtual void touchEventTargetRectsDidChange(const Document*) { }

Can we get rid of the document argument here since it doesn't look like we're using it any longer?

> Source/WebCore/rendering/RenderLayerBacking.cpp:1037
> +        if (verticalScrollbarLayerChanged && scrollingCoordinator)

No need to check scrollingCoordinator again.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.