[Webkit-unassigned] [Bug 78872] [chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side scrollbar painting

Fri Mar 2 15:02:24 PST 2012

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

--- Comment #29 from Tien-Ren Chen <trchen at chromium.org>  2012-03-02 15:02:24 PST ---
(In reply to comment #28)
> (From update of attachment 129800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=129800&action=review
> 
> So close!  I think if we restrict this to only running in threaded mode (so not on by default yet) we can land this (with the inline comments addressed) and then iterate on getting it perfect before we either turn the thread on by default or enable this everywhere.

I want to leave it enabled until the last moment before committing so we can catch more bugs with EWS.

> At a high level I think we have a bit of work to do in order to sync Scrollbar state to the layer properly (example: the tickmarks list). Let's get this in and then file a new bug to figure out an approach for all that.
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> there's an SVN presubmit check that will reject this line. please put a description of what this patch does and say which tests (if any) cover the behavior

I'll add some test later on. Like making sure scroll layer pointers are properly resolved.

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:69
> > +    if (!horizontalScrollbarLayer)
> 
> also check coordinatesScrollingForFrameView(frameView) here - it's not beneficial for us to set up special scrollbar layers for things that we aren't scrolling on the thread.
> 
> I also think we should guard this with CCProxy::hasImplThread() to only enable the scrollbar layers when running in threaded mode, so we don't turn this on for everyone as soon as it lands. There are some benefits to using this even in the single-thread case but I'd prefer to land the functionality, fix a few of the performance issues, etc, and then turn it on for the rest of the world.
> 
> same comments go for the vertical case. We could probably factor a lot of these checks into a helper function
> 
> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:77
> > +    if (scrollbar->isCustomScrollbar())
> 
> i don't think this is sufficient checking for custom scrollbars.  Consider the case of a scrollbar that goes from being non-custom to custom (say by some external stylesheet loading).  In that case, we'll first run through this code with isCustomScrollbar() == false and set up a scrollbar layer.  Then, at some point in the future we'll come through this code again with isCustomScrollbar() == true. We need to tear down the scrollbar layer and revert back to rendering with the normal layer.

Doesn't a switch from Scrollbar to RenderScrollbar remove and re-create the graphics layers? Anyway I agree cleaning the layer explicitly is better than let someone cleanup for us. I think setContentsToMedia(0) will do the trick.

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:81
> > +    horizontalScrollbarLayer->setContentsToMedia(scrollbarLayer.get());
> 
> this will set up the ScrollbarLayerChromium to render, but the scroll layer is still rendering as well. This is wasteful and in the case of a non-opaque scrollbar will render incorrectly. I think you need to call setDrawsContent(false) on the GraphicsLayer to make the contents layer the only thing that renders.
> 
> be sure to undo that when undoing the layer if the scrollbar becomes custom

No need to undo because by default m_drawsContent is false. It is enabled in ScrollView.cpp:positionScrollbarLayer()

I think we have to call setDrawsContent(false) though, in case of this execution path: page load -> custom scrollbar enabled -> setDrawsContent(true) -> custom scrollbar disabled -> scrollbar layer attached but m_drawsContent is still true

However I doubt ScrollCoordinator is the right place. IMO positionScrollbarLayer is a better place since all scrollbar layer visibility code are there.

> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:100
> > +    virtual bool hasContentsLayer() const { return m_contentsLayer; }
> 
> don't make this change in this patch unless it's necessary for this patch. if you just want to add it because you feel we should have it, file a separate bug and attach a patch with it there

It is, see ScrollView.cpp:positionScrollbarLayer()

> > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:57
> > +    scrollbar->getTickmarks(m_tickmarks);
> 
> how do the tickmarks (or other scrollbar state) get updated after the layer is created?

Argh! I forgot tickmarks is also dynamic. Let's skip tickmarks for this patch?

> > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.h:52
> > +    LayerChromium* m_scrollLayer;
> 
> do we need a layer pointer here, or can we use just an ID?  I think using an ID is a lot safer since we don't have to worry about this pointer going stale.

Sounds like a good idea. done

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:109
> > +int CCScrollbarLayerImpl::CCScrollbar::x() const { return frameRect().x(); }
> 
> please expand this (and the rest of the functions in here) out. it's more LOC, yes, but the current code is really dense and hard to read IMO.  we do often write short functions as one-liners in headers but this is a .cpp

done

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:165
> > +    // TODO(aelias): Hardcoding the first child here is weird. Think of
> 
> nit: WebKit style is that this should be a FIXME with no (owner)

done

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.h:77
> > +        CCScrollbar(CCScrollbarLayerImpl* owner) : m_owner(owner) { }
> 
> explicit, please

done

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