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

Thu Mar 1 13:17:33 PST 2012

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

--- Comment #21 from James Robinson <jamesr at chromium.org>  2012-03-01 13:17:33 PST ---
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 129622 [details] [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.

Ouch, that's unfortunate!  It looks like nobody is using ScrollbarThemeClient::parent() so maybe we can remove that at some point.

For now a nested class sounds good.  The main benefit is that we shouldn't have to worry about the lifetime of the CCScrollbarLayerImpl and the thing implementing ScrollbarThemeClient separately - they are always 1:1

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

I'm pretty sure the only way to use a theme other than ScrollbarTheme::theme() is by setting isCustomScrollbar()

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

Let's not worry about the Mac theme too much yet.  We want this code to work on Android and ChromeOS (which uses the Linux theme).

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

The theme itself shouldn't have any state, it's meant to be a pretty much purely functional thing.  That appears to be the case today for the Linux and Android themes at least.  It's not true for the Mac theme today but let's worry about that when we get to it (some time out, I think).

It's OK to have a FIXME here to keep this in mind when expanding to future themes, but the comment needs to say what the concern is.

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

That isn't actually true - non-overlay scrollbars are not always opaque. Non-overlay *root* scrollbars are always opaque. This will be a bit of a landmine when we extend this code in the future, I'm not sure it's worth the cost of one memset().  If you do want to leave it here leave a FIXME at least so we know to check this again in the future.

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