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

[bugzilla-daemon at webkit.org]

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


James Robinson <jamesr at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #129800|review?                     |review-
               Flag|                            |




--- Comment #28 from James Robinson <jamesr at chromium.org>  2012-03-02 11:59:35 PST ---
(From update of attachment 129800)
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.

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

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

> 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

> 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

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

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

> 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

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

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

explicit, please

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