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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Feb 20 15:34:45 PST 2013


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





--- Comment #27 from Tien-Ren Chen <trchen at chromium.org>  2013-02-20 15:37:06 PST ---
(In reply to comment #26)
> (From update of attachment 189163 [details])
> 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.

Thank you for the valuable feedback!

> > 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)?

Done

> 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.

I'm not aware of this problem. Could you elaborate? Thanks!
Anyway, I'm happy to set platformSupported to true only for Android.

> > 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

This container class is to isolate GraphicsLayer contents layer registration. Unfortunately it is difficult to do RAII on individual layers so I have to do it in the container. Making it a nested class is to avoid other ScrollCoordinator method to accidentally free a layer without unregistering it first.

I understand that WebKit style discourages the use of nested classes. I'm happy to remove it if you feel the above reason is not strong enough to make an exception.

> > 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.

I managed to reproduced the crash. It happens about 1 out of 10 trials. It is due to null scrollbarGraphicsLayer pointer for setupScrollbarLayer when calling from scrollableAreaScrollLayerDidChange(). Theoretically a WebScrollbarLayer only exists when the GraphicsLayer gets created first. This can only happen if we have orphaned WebScrollbarLayer from previously destroyed FrameView. I can add a check to avoid the crash but I'm afraid of potential memory leak.

I doubt the use of FrameView::removeScrollableArea cures the problem. Do we always remove each ScrollableArea from its parent when nuking down a FrameView tree? (and is ScrollingCoordinator still accessible during that time?)

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


More information about the webkit-unassigned mailing list