[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


Tien-Ren Chen <trchen at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |yusufo at chromium.org




--- Comment #16 from Tien-Ren Chen <trchen at chromium.org>  2013-02-14 12:17:16 PST ---
(In reply to comment #15)
> (From update of attachment 188261 [details])
> 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?

Ah ha, I'll try that. Great hints!

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

Agree, and we're calling to ScrollingCoordinator from too many different places: Document, HistoryController, FrameView, Page, RenderLayer, RenderLayerBacking, RenderLayerCompositor :(

I wish we have a cleaner way to do the lifetime management, but it is tricky because FrameView could be destructed when a scrollbar is still alive, calling scrollableAreaScrollbarLayerDidChange in RenderLayerCompositor destructor would be dangerous because the FrameView is already partially deconstructed.

Also I wanted to consolidate all scrollbar-related hook to ScrollableArea, but ScrollableArea doesn't have access to Page(thus no access to ScrollingCoordinator).

Consider the problems, that's why I made the decision to call scrollableAreaDestroyed in FrameView and RenderLayer. Any idea how to remove this clutter is welcome!

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

Technically we can, but I don't know if we have a plan to have per-frame touch event rects. CCed leviw@ and yusufo@ for that matter.

> > Source/WebCore/rendering/RenderLayerBacking.cpp:1037
> > +        if (verticalScrollbarLayerChanged && scrollingCoordinator)
> 
> No need to check scrollingCoordinator again.

Oops, that is a copy & paste mistake. I'll rearrange that.

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