[Webkit-unassigned] [Bug 78872] [chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side scrollbar painting
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Feb 29 21:44:45 PST 2012
https://bugs.webkit.org/show_bug.cgi?id=78872
James Robinson <jamesr at chromium.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #129622|review? |review-
Flag| |
--- Comment #19 from James Robinson <jamesr at chromium.org> 2012-02-29 21:44:44 PST ---
(From update of attachment 129622)
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
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list