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

Thu Mar 1 12:48:32 PST 2012

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

--- Comment #20 from Tien-Ren Chen <trchen at chromium.org>  2012-03-01 12:48:32 PST ---
(In reply to comment #19)
> (From update of attachment 129622 [details])
> 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.

Unfortunately we can't, due to namespace problem... Both CCLayerImpl and ScrollbarThemeClient has parent() function.
I would consider write CCScrollbar as a nested class.

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

cool. done

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

done

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

should be alright. I just keep imagine there might be more than one theme at the same time...

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

it is used somewhere in the scrollbar layer positioning logic (existing code). if it finds the scrollbar layer has contents, it will hide the main layer. it is a standard part of GraphlicsLayer, I think we need to implement it anyway.

> > Source/WebCore/platform/graphics/chromium/LayerChromium.h:322
> > +    ScrollbarLayerChromium *m_horizontalScrollbarLayer;
> > +    ScrollbarLayerChromium *m_verticalScrollbarLayer;
> 
> the * belongs with the type, not the variable name

done

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

yes if we always pull scroll offset instead of pushing. I'll try if we can do that.

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

ideally I want to add a Scrollbar::pushPropertiesTo, but Scrollbar is not in our control.
The other way is to let CCScrollbar make friend with ScrollbarLayerChromium, the layer will push everything.

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

done. I just copied everything brainlessly. lol

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

the alternative would be copying everything when creating ScrollbarLayerChromium.

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

sounds good. done

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

just to make the variable name consistent with other functions.

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

done

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

ok, sounds good

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

done

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

I'm agree with lineStep / pageStep, however a theme might decide to draw differently depending on whether the scrollable area is active or not. Does it make sense?

And scrollbar overlay style is used by Apple to change the color of the overlay scrollbar based on the background color of the scrollable area. We probably don't used it currently, but I think we should sync it.

> 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

ok

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

ok

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

We might need to make a clone of the theme. Though none of the platform we support have thread-safety issue currently. I'm thinking when we add android scrollbar animation timer later on, we might need to store data in the theme.

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:115
> > +    if (theme->usesOverlayScrollbars())
> > +        context->clearRect(IntRect(IntPoint(), contentBounds()));
> 
> i don't understand this branch

if the scrollbar is opaque, we don't need to clear canvas. non-overlay scrollbars are always opaque.

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

a mistake. done

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

done

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