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

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Mar 2 15:28:10 PST 2012


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





--- Comment #30 from James Robinson <jamesr at chromium.org>  2012-03-02 15:28:09 PST ---
(In reply to comment #29)
> (In reply to comment #28)
> > (From update of attachment 129800 [details] [details])
> > 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.
> 
> I want to leave it enabled until the last moment before committing so we can catch more bugs with EWS.

That's a sound plan, IMO.  Looks like this patch mispaints some scrollbars somewhere.

> 
> > 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
> 
> I'll add some test later on. Like making sure scroll layer pointers are properly resolved.

Cool.

> 
> > > 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.
> 
> Doesn't a switch from Scrollbar to RenderScrollbar remove and re-create the graphics layers? Anyway I agree cleaning the layer explicitly is better than let someone cleanup for us. I think setContentsToMedia(0) will do the trick.

Looking more closely switching in and out of custom scrollbar mode is just incredibly buggy in general.  Some testcases:

Switching from custom to non-custom: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1358
Switching from non-custom to custom: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/1359

I have to resize the window to get the style to even show up, and sometimes the scrollbar just vanishes completely :(.

Let's just do the defensive thing in ScrollingCoordinator, I don't think we can trust the cross-platform code too much at this point. I think this is rare enough that we can do possibly-redundant cleanup without worry about it.

> > > 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?
> 
> Argh! I forgot tickmarks is also dynamic. Let's skip tickmarks for this patch?

That's fine.  We can file a separate bug about tickmarks and just remember to fix it before turning it on for platforms that use them.

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