[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 #26 from James Robinson <jamesr at chromium.org>  2013-02-20 11:48:03 PST ---
(From update of attachment 189163)
View in context: https://bugs.webkit.org/attachment.cgi?id=189163&action=review

This is a big patch and I'm not quite done digesting it yet, but here are some initial comments.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:173
> +        // Root layer non-overlay scrollbars should be marked opaque to disable
> +        // blending.
> +        bool isOpaqueRootScrollbar = scrollableArea == static_cast<ScrollableArea*>(m_page->mainFrame()->view()) && !scrollbar->isOverlayScrollbar();

could you break this into two checks (one for root-ness and one for overlay)?

I'm a little worried that enabling this for other platforms (mainly windows) is going to break blending unless we take special care somewhere to fix up the alpha channel.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.h:80
> +    class ScrollbarLayerManager {

It's useful to wrap this functionality in separate functions, but I don't think this inner class adds much additional value.  It's just making ScrollingCoordinatorChromium.cpp more obscure.  I think you should just make these member functions and variables on ScrollingCoordinatorChromium

> Source/WebCore/rendering/RenderLayer.cpp:244
> +            scrollingCoordinator->willDestroyScrollableArea(this);

hmm - is there any way to combine this logic with FrameView::removeScrollableArea?  It's proven tricky to get that mechanism correct in all corner cases (and it still crashes sometimes, so we have a bit of work to do) so I'd like to share it as much as possible.

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