[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 #25 from Tien-Ren Chen <trchen at chromium.org>  2013-02-19 14:36:32 PST ---
Change from last patch:
* Rebased
* Fixed the nits

(In reply to comment #21)
> (From update of attachment 188658 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=188658&action=review
> 
> It looks to me that removing the ScrollCoordinatorChromiumPrivate makes the patch harder to follow.

Sorry about it. I understand this patch is doing too many thing at the same time.

> > Source/WebCore/page/FrameView.cpp:256
> > +        if (m_frame->page())
> > +            if (ScrollingCoordinator* scrollingCoordinator = m_frame->page()->scrollingCoordinator())
> > +                scrollingCoordinator->willDestroyScrollableArea(this);
> 
> needs wrapping {}

Done

> > Source/WebCore/page/FrameView.cpp:320
> > +    // FIXME: Technically the FrameView is not destroyed yet,
> > +    // but this is our last chance to access ScrollingCoordinator.
> > +    if (m_frame && m_frame->page())
> > +        if (ScrollingCoordinator* scrollingCoordinator = m_frame->page()->scrollingCoordinator())
> > +            scrollingCoordinator->willDestroyScrollableArea(this);
> > +
> 
> It looks strange to have this block here and at the dtor.

Done. I changed the destructor one to just call clearFrame.

(In reply to comment #22)
> (From update of attachment 188658 [details])
> Attachment 188658 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/16585802
> 
> New failing tests:
> inspector/styles/stylesheet-tracking.html
> media/video-controls-captions-trackmenu.html

Feels like a flake. Let's see if it still happens.

(In reply to comment #23)
> (From update of attachment 188658 [details])
> Thanks for adding the test. I think this looks good modulo Antonio's comments.
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=188658&action=review
> 
> > Source/WebCore/platform/ScrollableArea.h:205
> > +    friend class ScrollingCoordinator;
> 
> Could you move this next to the accessor methods so the association is clearer -- like we do for ScrollAnimator below. I can't help but wonder whether we should instead make the accessors public instead, but maybe there's a good reason for keeping them protected.

Done moving the friend clause near the accessors.

I'm thinking the same thing. It feels weird that you can access the layers through frameView->compositor()->layerForXxx() or renderLayer->backing()->layerForXxx() but impossible to do so on ScrollableArea unless downcasting it first.

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