[webkit-reviews] review denied: [Bug 78872] [chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side scrollbar painting : [Attachment 129622] 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 129622: Patch
https://bugs.webkit.org/attachment.cgi?id=129622&action=review

------- Additional Comments from James Robinson <jamesr at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=129622&action=review


This is not bad but it could be a whole lot simpler.  Consider the state we
already have stored on the scrolling layer itself (scrollable area, current
scroll offset, visible bounds) - we don't need to and don't want to duplicate
any of that state on other objects.  The impl-side implementation of
ScrollbarThemeClient should use state that's already being synced to either the
scrolling layer or the scrollbar layer whenever possible.  In fact, I don't
think we even need a separate object to implement the theme client - I'm pretty
sure CCScrollbarLayerImpl can implement that interface directly if it has a
back (raw) pointer to the CCLayerImpl that scrolls in order to query the
current scroll position, etc.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:67
> +static GraphicsLayer* scrollLayerForFrameView(FrameView* frameView)

this function isn't necessary - we have the scroll layer stored in m_private
already. see the other ScrollingCoordinatorChromium functions

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:89
> +    else {

this nesting is a little awkward.  i think this would look cleaner if it took
care of the early-outs and returned instead of nesting

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:92
> +	   if (scrollbar->theme() != ScrollbarTheme::theme() ||
scrollbar->isCustomScrollbar())

why both checks? should be able to just check isCustomScrollbar(), no?

> Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:100
> +    virtual bool hasContentsLayer() const { return m_contentsLayer; }

what's this change for? i can't find any callers

> Source/WebCore/platform/graphics/chromium/LayerChromium.h:322
> +    ScrollbarLayerChromium *m_horizontalScrollbarLayer;
> +    ScrollbarLayerChromium *m_verticalScrollbarLayer;

the * belongs with the type, not the variable name

i don't think we actually need pointers here though - can't we just store layer
IDs for the horizontal / vertical layers instead of pointers?

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:58
> +    scrollbar->pullPropertiesFrom(m_scrollbar.get());

why is a function called pushPropertiesTo() calling another function called
pullPropertiesFrom()? feels like a bad tug of war

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:59
> +    scrollbar->setFrameRect(IntRect(IntPoint(),
scrollbarLayer->contentBounds()));

this isn't necessary. the scrollbar layer knows its bounds, why are we pushing
anything redundantly to the scrollbar?

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.h:48
> +    RefPtr<Scrollbar> m_scrollbar;

this seems very wrong. the layer shouldn't have any ownership concern with the
scrollbar.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:42
> +    HashMap<int, CCLayerImpl*> newLayers;

make a typedef for this type as well. i'd suggest RawPtrCCLayerImplMap and
renaming CCLayerImplMap to OwningCCLayerImplMap

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:53
> -void TreeSynchronizer::collectExistingCCLayerImplRecursive(CCLayerImplMap&
map, PassOwnPtr<CCLayerImpl> popCCLayerImpl)
> +void TreeSynchronizer::collectExistingCCLayerImplRecursive(CCLayerImplMap&
oldLayers, PassOwnPtr<CCLayerImpl> popCCLayerImpl)

there's no reason to change this function

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:71
> +PassOwnPtr<CCLayerImpl>
TreeSynchronizer::reuseOrCreateCCLayerImpl(HashMap<int, CCLayerImpl*>
&newLayers, CCLayerImplMap& oldLayers, LayerChromium* layer)

again the & belongs with the type, not the variable name. the type of the
parameter is "reference to a hashmap of ints to CCLayerImpl pointers" and the
name of the parameter is "newLayers"

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:101
> +void TreeSynchronizer::resolveInterLayerPointersRecursive(const HashMap<int,
CCLayerImpl*> &newLayers, LayerChromium* layer)

this function name is pretty vague and non-descriptive. the purpose of this
function is to update scrollbar layer pointers, let's call it something that
makes it clear. how about updateScrollbarLayerPointersRecursive() ?

if we want to use this functionality to do other weak-pointerish things in the
future we can rename the function as appropriate.

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:106
> +    if (layer->m_horizontalScrollbarLayer) {

just provider a getter for the horizontal scrollbar layer (or ID if that's all
we need) - reaching into other classes m_foo's looks weird and is harder to
refactor

> Source/WebCore/platform/graphics/chromium/TreeSynchronizer.cpp:109
> +	  
ccLayerImpl->m_horizontalScrollbar->setCurrentPos(ccLayerImpl->scrollPosition()
.x() + ccLayerImpl->scrollDelta().width());

this is the wrong place to update properties. i don't think you should be
updating the scrollbar position this way at all (see comments elsewhere)

> Source/WebCore/platform/graphics/chromium/cc/CCLayerImpl.cpp:201
> +    if (m_horizontalScrollbar)
> +	   m_horizontalScrollbar->setCurrentPos(m_scrollPosition.x() +
m_scrollDelta.width());
> +    if (m_verticalScrollbar)
> +	   m_verticalScrollbar->setCurrentPos(m_scrollPosition.y() +
m_scrollDelta.height());

this is backwards. the scroll position should live on the layer and scrollbars
should pull it, not the other way 'round. the only thing we should be doing
here is marking the scrollbar layers (not the scrollbars) as needing a repaint

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbar.cpp:110
> +    m_frameRect = scrollbar->frameRect();

this is pushing a whole lot of unnecessary state. consider:

*) the scrollbar layer has its bounds set up already, which covers frameRect
*) the scrollable layer itself has its current scroll position, visible bounds,
and scrollable bounds
*) we know the orientation of the scrollbar when we set the layer up

additionally many bits of state are only needed for layout and input handling
on the scrollbar itself which we aren't doing on the thread. we don't need any
of these bits of state at all:
*) isScrollableAreaActive()
*) isScrollViewScrollbar()
*) lineStep() / pageStep()
*) scrollbarOverlayStyle()

the only bits that we actually want need to get from the Scrollbar itself are:
*) enabled (so we know whether to draw the scrollbar greyed out or not)
*) hovered / pressed parts (so we know whether to draw that bit of the theme)
*) tickmarks so we can draw those

to be more consistent with other state we should push these bits of state in
ScrollbarLayerChromium::pushPropertiesTo

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:57
> +    if (bounds().isEmpty() || contentBounds().isEmpty())

ideally we wouldn't need to regenerate a texture every frame but only if the
scroll position or some scrollbar state updated. we could do that as a
follow-up, however

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:89
> +    grContext->resetContext();

no no, as i said before we can't use ganesh on the compositor context. just
software paint and upload to a texture.  this is really easy if you use
PlatformCanvas (in Source/WebCore/platform/graphics/chromium). just be careful
about what pixel order you paint and upload in

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:112
> +    ScrollbarTheme* theme = ScrollbarTheme::theme(); // FIXME

FIXME with no elaboration is useless. what does this FIXME mean?

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:115
> +    if (theme->usesOverlayScrollbars())
> +	   context->clearRect(IntRect(IntPoint(), contentBounds()));

i don't understand this branch

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.h:47
> +    void paint(GraphicsContext*);

why is this public?

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.h:52
> +    CCScrollbarLayerImpl(int id);

explicit