[webkit-reviews] review denied: [Bug 78872] [chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side scrollbar painting : [Attachment 129800] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Tien-Ren Chen
<trchen at chromium.org>'s request for review:
Bug 78872: [chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side
scrollbar painting
https://bugs.webkit.org/show_bug.cgi?id=78872

Attachment 129800: Patch
https://bugs.webkit.org/attachment.cgi?id=129800&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
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