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

• Component: New Bugs
• Status: RESOLVED FIXED

[Tien-Ren Chen]

[chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side scrollbar painting

[Tien-Ren Chen]

Created attachment 127514 [details]
Patch

[James Robinson]

Comment on attachment 127514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127514&action=review

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1839
> +#if PLATFORM(CHROMIUM)
> +            Scrollbar* scrollbar = m_renderView->frameView()->horizontalScrollbar();
> +            if (ScrollbarThemeCCScroll* theme = scrollbar->theme()->toScrollbarThemeCCScroll())
> +                theme->attachToGraphicsLayer(scrollbar, m_scrollLayer.get(), m_layerForHorizontalScrollbar.get());
> +#endif

if you look a few lines below here there's a call to ScrollingCoordinator::frameViewHorizontalScrollbarLayerDidChange().  I think we want to put the ScrollbarLayerChromium creation and hookup inside our implementation of that function, not in here.

I also don't think that this should have anything at all to do with the ScrollbarTheme.

[WebKit Review Bot]

Comment on attachment 127514 [details]
Patch

Attachment 127514 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11542284

New failing tests:
css1/font_properties/font_size.html
css1/box_properties/border_left_width.html
css1/basic/class_as_selector.html
css1/font_properties/font_family.html
css1/font_properties/font_weight.html
css1/box_properties/border_bottom.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_right_width.html
css1/basic/id_as_selector.html
css1/basic/inheritance.html
css1/box_properties/border.html
css1/color_and_background/background_position.html
css1/box_properties/border_right_inline.html
css1/box_properties/border_left.html
css1/cascade/cascade_order.html
css1/color_and_background/background.html
css1/conformance/forward_compatible_parsing.html
css1/box_properties/border_top.html
css1/box_properties/border_style.html
css1/basic/containment.html
accessibility/aria-describedby-on-input.html
css1/color_and_background/background_attachment.html
css1/basic/comments.html
css1/pseudo/anchor.html
css1/classification/white_space.html
css1/color_and_background/background_repeat.html
css1/font_properties/font.html
css1/formatting_model/floating_elements.html
css1/classification/display.html
css1/classification/list_style_type.html

[Tien-Ren Chen]

(In reply to comment #2)
> (From update of attachment 127514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127514&action=review
> 
> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1839
> > +#if PLATFORM(CHROMIUM)
> > +            Scrollbar* scrollbar = m_renderView->frameView()->horizontalScrollbar();
> > +            if (ScrollbarThemeCCScroll* theme = scrollbar->theme()->toScrollbarThemeCCScroll())
> > +                theme->attachToGraphicsLayer(scrollbar, m_scrollLayer.get(), m_layerForHorizontalScrollbar.get());
> > +#endif
> 
> if you look a few lines below here there's a call to ScrollingCoordinator::frameViewHorizontalScrollbarLayerDidChange().  I think we want to put the ScrollbarLayerChromium creation and hookup inside our implementation of that function, not in here.
> 
> I also don't think that this should have anything at all to do with the ScrollbarTheme.

Yes I agree with you. It is in the current shape only because we don't have a ScrollingCoordinatorChromium yet. :)

I talked with Alex this evening. He also gave me some good comments that the ScrollingCoordinator should take the ownership of the scrollbar PlatformLayer.

I'll take a look at ScrollingCoordinator and see how do we integrate that to our tree synchronization.

[Adrienne Walker]

Comment on attachment 127514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127514&action=review

> Source/WebCore/platform/chromium/ScrollbarThemeCCScrollExample.cpp:74
> +    float barBegin = scrollbar->currentPos() / scrollbar->totalSize();
> +    float barEnd = (scrollbar->currentPos() + scrollbar->visibleSize()) / scrollbar->totalSize();

I don't think this will work without pkotwicz's change in bug 78028.  Scrollbar is an object owned and manipulated on the main thread side.  We don't want to read values from it directly, as it could be being manipulated by the other thread in ways that aren't valid for the current frame on the impl side.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:1866
> +            if (ScrollbarThemeCCScroll* theme = scrollbar->theme()->toScrollbarThemeCCScroll())
> +                theme->attachToGraphicsLayer(scrollbar, m_scrollLayer.get(), m_layerForVerticalScrollbar.get());

Aren't we going to want to be able to draw any kind of scrollbar theme on the impl side? I'm not sure I follow what this class is trying to do.

[Tien-Ren Chen]

(In reply to comment #5)
> (From update of attachment 127514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127514&action=review
> 
> > Source/WebCore/platform/chromium/ScrollbarThemeCCScrollExample.cpp:74
> > +    float barBegin = scrollbar->currentPos() / scrollbar->totalSize();
> > +    float barEnd = (scrollbar->currentPos() + scrollbar->visibleSize()) / scrollbar->totalSize();
> 
> I don't think this will work without pkotwicz's change in bug 78028.  Scrollbar is an object owned and manipulated on the main thread side.  We don't want to read values from it directly, as it could be being manipulated by the other thread in ways that aren't valid for the current frame on the impl side.

No, we don't try to call anything in ScrollbarTheme from the impl side. It won't work for many reasons. Not only Scrollbar object might be updated unexpectedly, the ScrollbarTheme itself too.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:1866
> > +            if (ScrollbarThemeCCScroll* theme = scrollbar->theme()->toScrollbarThemeCCScroll())
> > +                theme->attachToGraphicsLayer(scrollbar, m_scrollLayer.get(), m_layerForVerticalScrollbar.get());
> 
> Aren't we going to want to be able to draw any kind of scrollbar theme on the impl side? I'm not sure I follow what this class is trying to do.

The whole idea about ScrollbarThemeCCScroll is to provide an interface to create specialized PlatformLayer for scrollbars. (ScrollbarLayerChromium/CCScrollbarLayerImpl) The impl-side painting logic will be handled by the CCScrollbarLayerImpl implementation.

I don't think it is easy to allow drawing any kind of scrollbar theme on the impl side. It would put too much restriction on the ScrollbarTheme implementation. That means you have to make paint a static function, no access to the ScrollbarTheme during painting.

I think it is necessary to have different painting function for the impl side. Even creating GraphicsContext in the impl side could be a problem (correct me if I'm wrong).

[Adrienne Walker]

Comment on attachment 127514 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=127514&action=review

>>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1866
>>> +                theme->attachToGraphicsLayer(scrollbar, m_scrollLayer.get(), m_layerForVerticalScrollbar.get());
>> 
>> Aren't we going to want to be able to draw any kind of scrollbar theme on the impl side? I'm not sure I follow what this class is trying to do.
> 
> The whole idea about ScrollbarThemeCCScroll is to provide an interface to create specialized PlatformLayer for scrollbars. (ScrollbarLayerChromium/CCScrollbarLayerImpl) The impl-side painting logic will be handled by the CCScrollbarLayerImpl implementation.
> 
> I don't think it is easy to allow drawing any kind of scrollbar theme on the impl side. It would put too much restriction on the ScrollbarTheme implementation. That means you have to make paint a static function, no access to the ScrollbarTheme during painting.
> 
> I think it is necessary to have different painting function for the impl side. Even creating GraphicsContext in the impl side could be a problem (correct me if I'm wrong).

If you're just creating a scrollbar layer, why do you need this factory-style interface to do it? Are you planning to have different types of scrollbar layers? I guess I just don't follow why the theme has to be replaced here just so we can create platform layers of the right type.

Also, I don't think it is necessary to have a different painting function for the impl side.  My hope is that we can create a new ScrollbarTheme object of the same type and pass it over to the impl side with all of the scrollbar data, but not an actual scrollbar pointer.  It's not a static function, but it is an object with no member variables that just uses a vtable to decide what functions to use.  Then we can reuse Ganesh to wrap a GraphicsContext3D in a GraphicsContext and reuse the same paint routines to just draw directly into the backbuffer.  I think it's do-able and it'll be the easiest path to get all the other platforms to have impl-side scrollbars without rewriting paint routines for all of them.

[Tien-Ren Chen]

(In reply to comment #7)
> (From update of attachment 127514 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=127514&action=review
> 
> >>> Source/WebCore/rendering/RenderLayerCompositor.cpp:1866
> >>> +                theme->attachToGraphicsLayer(scrollbar, m_scrollLayer.get(), m_layerForVerticalScrollbar.get());
> >> 
> >> Aren't we going to want to be able to draw any kind of scrollbar theme on the impl side? I'm not sure I follow what this class is trying to do.
> > 
> > The whole idea about ScrollbarThemeCCScroll is to provide an interface to create specialized PlatformLayer for scrollbars. (ScrollbarLayerChromium/CCScrollbarLayerImpl) The impl-side painting logic will be handled by the CCScrollbarLayerImpl implementation.
> > 
> > I don't think it is easy to allow drawing any kind of scrollbar theme on the impl side. It would put too much restriction on the ScrollbarTheme implementation. That means you have to make paint a static function, no access to the ScrollbarTheme during painting.
> > 
> > I think it is necessary to have different painting function for the impl side. Even creating GraphicsContext in the impl side could be a problem (correct me if I'm wrong).
> 
> If you're just creating a scrollbar layer, why do you need this factory-style interface to do it? Are you planning to have different types of scrollbar layers? I guess I just don't follow why the theme has to be replaced here just so we can create platform layers of the right type.

Yes, although currently I don't think any of the platform use more than one scrollbar theme, there is no reason why not to support that.

> Also, I don't think it is necessary to have a different painting function for the impl side.  My hope is that we can create a new ScrollbarTheme object of the same type and pass it over to the impl side with all of the scrollbar data, but not an actual scrollbar pointer.  It's not a static function, but it is an object with no member variables that just uses a vtable to decide what functions to use.  Then we can reuse Ganesh to wrap a GraphicsContext3D in a GraphicsContext and reuse the same paint routines to just draw directly into the backbuffer.  I think it's do-able and it'll be the easiest path to get all the other platforms to have impl-side scrollbars without rewriting paint routines for all of them.

I see. It actually sounds like a better plan. I'll try to reuse existing paint function then. This way we probably don't even have to do much modification with ScrollbarTheme.

[Tien-Ren Chen]

(In reply to comment #7)
> Also, I don't think it is necessary to have a different painting function for the impl side.  My hope is that we can create a new ScrollbarTheme object of the same type and pass it over to the impl side with all of the scrollbar data, but not an actual scrollbar pointer.  It's not a static function, but it is an object with no member variables that just uses a vtable to decide what functions to use.  Then we can reuse Ganesh to wrap a GraphicsContext3D in a GraphicsContext and reuse the same paint routines to just draw directly into the backbuffer.  I think it's do-able and it'll be the easiest path to get all the other platforms to have impl-side scrollbars without rewriting paint routines for all of them.

Allow me to ask a dumb question. Does Ganesh wrap a GraphicsContext3D or OpenGL FBO/texture? There is subtle difference here because in Android port we use command buffer to delegate drawing to another process. That is, GraphicsContext3D calls are not backed by libGL.so. I'm afraid Ganesh won't work with it.

[James Robinson]

(In reply to comment #9)
> (In reply to comment #7)
> > Also, I don't think it is necessary to have a different painting function for the impl side.  My hope is that we can create a new ScrollbarTheme object of the same type and pass it over to the impl side with all of the scrollbar data, but not an actual scrollbar pointer.  It's not a static function, but it is an object with no member variables that just uses a vtable to decide what functions to use.  Then we can reuse Ganesh to wrap a GraphicsContext3D in a GraphicsContext and reuse the same paint routines to just draw directly into the backbuffer.  I think it's do-able and it'll be the easiest path to get all the other platforms to have impl-side scrollbars without rewriting paint routines for all of them.
> 
> Allow me to ask a dumb question. Does Ganesh wrap a GraphicsContext3D or OpenGL FBO/texture? There is subtle difference here because in Android port we use command buffer to delegate drawing to another process. That is, GraphicsContext3D calls are not backed by libGL.so. I'm afraid Ganesh won't work with it.

Ganesh uses the same command buffer code that other GraphicsContext3D calls, although it hooks in at a slightly lower level.  It works fine in the Android port.

[Tien-Ren Chen]

(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > Also, I don't think it is necessary to have a different painting function for the impl side.  My hope is that we can create a new ScrollbarTheme object of the same type and pass it over to the impl side with all of the scrollbar data, but not an actual scrollbar pointer.  It's not a static function, but it is an object with no member variables that just uses a vtable to decide what functions to use.  Then we can reuse Ganesh to wrap a GraphicsContext3D in a GraphicsContext and reuse the same paint routines to just draw directly into the backbuffer.  I think it's do-able and it'll be the easiest path to get all the other platforms to have impl-side scrollbars without rewriting paint routines for all of them.
> > 
> > Allow me to ask a dumb question. Does Ganesh wrap a GraphicsContext3D or OpenGL FBO/texture? There is subtle difference here because in Android port we use command buffer to delegate drawing to another process. That is, GraphicsContext3D calls are not backed by libGL.so. I'm afraid Ganesh won't work with it.
> 
> Ganesh uses the same command buffer code that other GraphicsContext3D calls, although it hooks in at a slightly lower level.  It works fine in the Android port.

Ok, let me write some code that actually make use of it. Not having to write separate paint function sounds a big big plus to me.

[Tien-Ren Chen]

Created attachment 128622 [details]
Patch

[Tien-Ren Chen]

Design doc: https://docs.google.com/a/google.com/document/d/1zBIUZ9bqQvJiq32khyDF3Grw-h5eHSXHx64coSSWUXM/edit

[James Robinson]

Thanks a lot for producing that doc, it's been very useful.  I think that for now we should restrict this to only default-themed scrollbars and just call into the theme from the thread to raster to scrollbar from the impl thread.

I've applied this patch locally (on top of https://bugs.webkit.org/show_bug.cgi?id=78028 with a lot of fixups for RefPtr/OwnPtr, etc) and have made pretty good progress on getting things hooked up.  Once I solidify things a bit more I'll provide some more feedback either in terms of a patch or just ideas.  I think we're really close here!

[Tien-Ren Chen]

Created attachment 129619 [details]
Patch

[Tien-Ren Chen]

(In reply to comment #14)
> Thanks a lot for producing that doc, it's been very useful.  I think that for now we should restrict this to only default-themed scrollbars and just call into the theme from the thread to raster to scrollbar from the impl thread.
> 
> I've applied this patch locally (on top of https://bugs.webkit.org/show_bug.cgi?id=78028 with a lot of fixups for RefPtr/OwnPtr, etc) and have made pretty good progress on getting things hooked up.  Once I solidify things a bit more I'll provide some more feedback either in terms of a patch or just ideas.  I think we're really close here!

Thanks for the feedback. I just updated the patch based on latest tree and #78028.

The current CCScrollbar implementation just copies everything from Scrollbar. So far so good it looks.

Now I think I have to add some tests for this thing...

[James Robinson]

Please post this patch relative to https://bugs.webkit.org/show_bug.cgi?id=78028 and not including it, it's really hard to view the diff when it has an extra 120kb of stuff.

[Tien-Ren Chen]

Created attachment 129622 [details]
Patch

[James Robinson]

Comment on attachment 129622 [details]
Patch

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

[Tien-Ren Chen]

(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

[James Robinson]

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

[Tien-Ren Chen]

(In reply to comment #20)
> (In reply to comment #19)
> > > 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.

I find this won't work. m_visibleSize and m_totalSize is something very dynamic, we need to update their value on every commits.
I think we might be able to get these information from the layer data, any suggestion?

[James Robinson]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > > 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.
> 
> I find this won't work. m_visibleSize and m_totalSize is something very dynamic, we need to update their value on every commits.
> I think we might be able to get these information from the layer data, any suggestion?

These are asking about the visible / total size of the scrollable area, not the scrollbar itself.  I think you should get those from the CCLayerImpl that's actually scrolling by giving the CCScrollbar (or CCScrollbarLayerImpl) a weak pointer to the CCLayerImpl that scrolls.  For example, totalSize() is some combination of the CCLayerImpl's maxScrollPosition, bounds, etc.

Keep in mind that these values can change on the thread without us going through a commit - for example, if the page scale changes due to a pinch gesture that changes the max scroll position and thus the totalSize() value. I think the only way to stay sane with these updates is to always query the value from the scrolling layer and not try to proactively sync them to other objects.

[Tien-Ren Chen]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > > 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.
> > 
> > I find this won't work. m_visibleSize and m_totalSize is something very dynamic, we need to update their value on every commits.
> > I think we might be able to get these information from the layer data, any suggestion?
> 
> These are asking about the visible / total size of the scrollable area, not the scrollbar itself.  I think you should get those from the CCLayerImpl that's actually scrolling by giving the CCScrollbar (or CCScrollbarLayerImpl) a weak pointer to the CCLayerImpl that scrolls.  For example, totalSize() is some combination of the CCLayerImpl's maxScrollPosition, bounds, etc.
> 
> Keep in mind that these values can change on the thread without us going through a commit - for example, if the page scale changes due to a pinch gesture that changes the max scroll position and thus the totalSize() value. I think the only way to stay sane with these updates is to always query the value from the scrolling layer and not try to proactively sync them to other objects.

Ok, I managed to get all the values. (for totalSize again we use m_scrollLayer->children()[0]->contentBounds() hack)

However I observed that the scroll maximum is not 100% identical to the main-side. The impl-side scroll maximum is slight smaller than the main-side on http://www.nytimes.com/ This can be easily reproduced using arrow-key scroll (slow) and mouse-wheel scroll (fast).

[Tien-Ren Chen]

(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > (In reply to comment #20)
> > > > (In reply to comment #19)
> > > > > > 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.
> > > 
> > > I find this won't work. m_visibleSize and m_totalSize is something very dynamic, we need to update their value on every commits.
> > > I think we might be able to get these information from the layer data, any suggestion?
> > 
> > These are asking about the visible / total size of the scrollable area, not the scrollbar itself.  I think you should get those from the CCLayerImpl that's actually scrolling by giving the CCScrollbar (or CCScrollbarLayerImpl) a weak pointer to the CCLayerImpl that scrolls.  For example, totalSize() is some combination of the CCLayerImpl's maxScrollPosition, bounds, etc.
> > 
> > Keep in mind that these values can change on the thread without us going through a commit - for example, if the page scale changes due to a pinch gesture that changes the max scroll position and thus the totalSize() value. I think the only way to stay sane with these updates is to always query the value from the scrolling layer and not try to proactively sync them to other objects.
> 
> Ok, I managed to get all the values. (for totalSize again we use m_scrollLayer->children()[0]->contentBounds() hack)
> 
> However I observed that the scroll maximum is not 100% identical to the main-side. The impl-side scroll maximum is slight smaller than the main-side on http://www.nytimes.com/ This can be easily reproduced using arrow-key scroll (slow) and mouse-wheel scroll (fast).

NVM. Can no longer reproduce this after merging to latest tree.

[Tien-Ren Chen]

Created attachment 129800 [details]
Patch

[WebKit Review Bot]

Comment on attachment 129800 [details]
Patch

Attachment 129800 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11766793

New failing tests:
compositing/iframes/scrolling-iframe.html
compositing/geometry/fixed-in-composited.html
compositing/geometry/vertical-scroll-composited.html
compositing/iframes/connect-compositing-iframe-delayed.html
compositing/direct-image-compositing.html
compositing/iframes/connect-compositing-iframe2.html
compositing/geometry/fixed-position.html
http/tests/inspector/inspect-element.html
accessibility/aria-disabled.html
compositing/masks/multiple-masks.html
compositing/iframes/overlapped-iframe.html
compositing/iframes/become-overlapped-iframe.html
compositing/geometry/tall-page-composited.html
compositing/iframes/iframe-resize.html
compositing/masks/simple-composited-mask.html
compositing/culling/filter-occlusion-alpha-large.html
compositing/iframes/connect-compositing-iframe3.html
compositing/scaling/tiled-layer-recursion.html
compositing/iframes/enter-compositing-iframe.html
compositing/geometry/horizontal-scroll-composited.html
compositing/reflections/nested-reflection-opacity.html
compositing/culling/filter-occlusion-blur.html
compositing/iframes/connect-compositing-iframe.html
compositing/iframes/iframe-size-from-zero.html
compositing/iframes/composited-parent-iframe.html
compositing/iframes/invisible-nested-iframe-show.html
compositing/overflow/fixed-position-ancestor-clip.html
compositing/culling/filter-occlusion-blur-large.html
compositing/iframes/resizer.html
compositing/reflections/reflection-ordering.html

[James Robinson]

Comment on attachment 129800 [details]
Patch

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.

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

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

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:81
> +    horizontalScrollbarLayer->setContentsToMedia(scrollbarLayer.get());

this will set up the ScrollbarLayerChromium to render, but the scroll layer is still rendering as well. This is wasteful and in the case of a non-opaque scrollbar will render incorrectly. I think you need to call setDrawsContent(false) on the GraphicsLayer to make the contents layer the only thing that renders.

be sure to undo that when undoing the layer if the scrollbar becomes custom

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

don't make this change in this patch unless it's necessary for this patch. if you just want to add it because you feel we should have it, file a separate bug and attach a patch with it there

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

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.h:52
> +    LayerChromium* m_scrollLayer;

do we need a layer pointer here, or can we use just an ID?  I think using an ID is a lot safer since we don't have to worry about this pointer going stale.

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:109
> +int CCScrollbarLayerImpl::CCScrollbar::x() const { return frameRect().x(); }

please expand this (and the rest of the functions in here) out. it's more LOC, yes, but the current code is really dense and hard to read IMO.  we do often write short functions as one-liners in headers but this is a .cpp

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:165
> +    // TODO(aelias): Hardcoding the first child here is weird. Think of

nit: WebKit style is that this should be a FIXME with no (owner)

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.h:77
> +        CCScrollbar(CCScrollbarLayerImpl* owner) : m_owner(owner) { }

explicit, please

[Tien-Ren Chen]

(In reply to comment #28)
> (From update of attachment 129800 [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.

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

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

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:81
> > +    horizontalScrollbarLayer->setContentsToMedia(scrollbarLayer.get());
> 
> this will set up the ScrollbarLayerChromium to render, but the scroll layer is still rendering as well. This is wasteful and in the case of a non-opaque scrollbar will render incorrectly. I think you need to call setDrawsContent(false) on the GraphicsLayer to make the contents layer the only thing that renders.
> 
> be sure to undo that when undoing the layer if the scrollbar becomes custom

No need to undo because by default m_drawsContent is false. It is enabled in ScrollView.cpp:positionScrollbarLayer()

I think we have to call setDrawsContent(false) though, in case of this execution path: page load -> custom scrollbar enabled -> setDrawsContent(true) -> custom scrollbar disabled -> scrollbar layer attached but m_drawsContent is still true

However I doubt ScrollCoordinator is the right place. IMO positionScrollbarLayer is a better place since all scrollbar layer visibility code are there.

> > Source/WebCore/platform/graphics/chromium/GraphicsLayerChromium.h:100
> > +    virtual bool hasContentsLayer() const { return m_contentsLayer; }
> 
> don't make this change in this patch unless it's necessary for this patch. if you just want to add it because you feel we should have it, file a separate bug and attach a patch with it there

It is, see ScrollView.cpp:positionScrollbarLayer()

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

> > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.h:52
> > +    LayerChromium* m_scrollLayer;
> 
> do we need a layer pointer here, or can we use just an ID?  I think using an ID is a lot safer since we don't have to worry about this pointer going stale.

Sounds like a good idea. done

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:109
> > +int CCScrollbarLayerImpl::CCScrollbar::x() const { return frameRect().x(); }
> 
> please expand this (and the rest of the functions in here) out. it's more LOC, yes, but the current code is really dense and hard to read IMO.  we do often write short functions as one-liners in headers but this is a .cpp

done

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:165
> > +    // TODO(aelias): Hardcoding the first child here is weird. Think of
> 
> nit: WebKit style is that this should be a FIXME with no (owner)

done

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.h:77
> > +        CCScrollbar(CCScrollbarLayerImpl* owner) : m_owner(owner) { }
> 
> explicit, please

done

[James Robinson]

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

[Adrienne Walker]

*** Bug 79137 has been marked as a duplicate of this bug. ***

[Tien-Ren Chen]

Created attachment 130213 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130213 [details]
Patch

Attachment 130213 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11836073

New failing tests:
compositing/geometry/fixed-position.html
platform/chromium/compositing/layout-width-change.html
compositing/geometry/fixed-in-composited.html
compositing/masks/multiple-masks.html
compositing/iframes/overlapped-nested-iframes.html
compositing/geometry/horizontal-scroll-composited.html
compositing/overflow/fixed-position-ancestor-clip.html
compositing/culling/filter-occlusion-blur-large.html
compositing/direct-image-compositing.html
compositing/geometry/tall-page-composited.html
compositing/geometry/vertical-scroll-composited.html
compositing/masks/simple-composited-mask.html
compositing/culling/filter-occlusion-alpha-large.html
platform/chromium/virtual/gpu/fast/canvas/canvas-text-alignment.html
compositing/iframes/become-composited-nested-iframes.html
platform/chromium/compositing/img-layer-grow.html
compositing/scaling/tiled-layer-recursion.html

[James Robinson]

Comment on attachment 130213 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130213&action=review

Some of the rects seem off in various cases.  I tried dumping all properties exposed by ScrollbarThemeClient on the main and impl side for the test compositing/culling/filter-occlusion-alpha-large.html:

erty height is 600
property size().width is 15
property size().height is 600
property location().x is 785
property location().y is 0
property frameRect().x is 785
property frameRect().y is 0
property frameRect().width is 15
property frameRect().height is 600
property value is 0
property visibleSize is 600
property totalSize is 0
property maximum is -600
main thread horizontal scrollbar:
property x is 0
property y is 0
property width is 15
property height is 15
property size().width is 15
property size().height is 15
property location().x is 0
property location().y is 0
property frameRect().x is 0
property frameRect().y is 0
property frameRect().width is 15
property frameRect().height is 15
property value is 0
property visibleSize is 0
property totalSize is 0
property maximum is 0

impl thread vertical scrollbar:
property x is 0
property y is 0
property width is 15
property height is 585
property size().width is 15
property size().height is 585
property location().x is 0
property location().y is 0
property frameRect().x is 0
property frameRect().y is 0
property frameRect().width is 15
property frameRect().height is 585
property value is 0
property visibleSize is 600
property totalSize is 800
property maximum is 200
impl thread horizontal scrollbar:
property x is 0
property y is 0
property width is 785
property height is 15
property size().width is 785
property size().height is 15
property location().x is 0
property location().y is 0
property frameRect().x is 0
property frameRect().y is 0
property frameRect().width is 785
property frameRect().height is 15
property value is 0
property visibleSize is 800
property totalSize is 800
property maximum is 0

printing code here: http://paste.lisp.org/+2QW4, hope this helps

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:90
> +    if (scrollbar->isCustomScrollbar() /* && CCProxy::hasImplThread() */) { // FIXME: also filter out single thread case when we

looks like this comment trailed off. The idea is to add this check back before landing, right?

this logic is a bit off - i think you want isCustomScrollbar() || !CCProxy::hasImplThread(). if we are a custom scrollbar _or_ if we're in single threaded mode, disable compositor scrollbar painting

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:107
> +    if (!scrollLayer) // workaround calling order problem

Comment seems incomplete.  I think this should be a FIXME, right?

[Tien-Ren Chen]

(In reply to comment #34)
> (From update of attachment 130213 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130213&action=review
> 
> Some of the rects seem off in various cases.  I tried dumping all properties exposed by ScrollbarThemeClient on the main and impl side for the test compositing/culling/filter-occlusion-alpha-large.html:
> 
> erty height is 600
> property size().width is 15
> property size().height is 600
> property location().x is 785
> property location().y is 0
> property frameRect().x is 785
> property frameRect().y is 0
> property frameRect().width is 15
> property frameRect().height is 600
> property value is 0
> property visibleSize is 600
> property totalSize is 0
> property maximum is -600
> main thread horizontal scrollbar:
> property x is 0
> property y is 0
> property width is 15
> property height is 15
> property size().width is 15
> property size().height is 15
> property location().x is 0
> property location().y is 0
> property frameRect().x is 0
> property frameRect().y is 0
> property frameRect().width is 15
> property frameRect().height is 15
> property value is 0
> property visibleSize is 0
> property totalSize is 0
> property maximum is 0
> 
> impl thread vertical scrollbar:
> property x is 0
> property y is 0
> property width is 15
> property height is 585
> property size().width is 15
> property size().height is 585
> property location().x is 0
> property location().y is 0
> property frameRect().x is 0
> property frameRect().y is 0
> property frameRect().width is 15
> property frameRect().height is 585
> property value is 0
> property visibleSize is 600
> property totalSize is 800
> property maximum is 200
> impl thread horizontal scrollbar:
> property x is 0
> property y is 0
> property width is 785
> property height is 15
> property size().width is 785
> property size().height is 15
> property location().x is 0
> property location().y is 0
> property frameRect().x is 0
> property frameRect().y is 0
> property frameRect().width is 785
> property frameRect().height is 15
> property value is 0
> property visibleSize is 800
> property totalSize is 800
> property maximum is 0
> 
> printing code here: http://paste.lisp.org/+2QW4, hope this helps

The difference in frameRect() is intentional. The frameRect is relative to the GraphicsContext which the scrollbar will be drawn. In impl thread the scrollbar has its exclusive GraphicsContext, so the frameRect should be exactly the bound of the texture.

For the scroll range I feel the values reported from the main thread is weird. Seems like there is a viewport size change, and the new size is propagated to the layers before the Scrollbar. (means the values calculated from impl thread is more up-to-date)

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:90
> > +    if (scrollbar->isCustomScrollbar() /* && CCProxy::hasImplThread() */) { // FIXME: also filter out single thread case when we
> 
> looks like this comment trailed off. The idea is to add this check back before landing, right?
> 
> this logic is a bit off - i think you want isCustomScrollbar() || !CCProxy::hasImplThread(). if we are a custom scrollbar _or_ if we're in single threaded mode, disable compositor scrollbar painting

done. what an embarrassing mistake. lol

> > Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:107
> > +    if (!scrollLayer) // workaround calling order problem
> 
> Comment seems incomplete.  I think this should be a FIXME, right?

done

[Tien-Ren Chen]

(In reply to comment #35)
> (In reply to comment #34)
> The difference in frameRect() is intentional. The frameRect is relative to the GraphicsContext which the scrollbar will be drawn. In impl thread the scrollbar has its exclusive GraphicsContext, so the frameRect should be exactly the bound of the texture.
> 
> For the scroll range I feel the values reported from the main thread is weird. Seems like there is a viewport size change, and the new size is propagated to the layers before the Scrollbar. (means the values calculated from impl thread is more up-to-date)

CORRECT: viewport size change -> document size change

[James Robinson]

Right - a better comparison would be dumping these values in Scrollbar::paint() when it's hit on the software path (when this code is disabled) vs the values in the impl side.  Dumping at the frameView..ScrollbarDid() time aren't going to be necessarily useful.

[Tien-Ren Chen]

Created attachment 130518 [details]
Patch

[WebKit Review Bot]

Comment on attachment 130518 [details]
Patch

Attachment 130518 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11832862

New failing tests:
compositing/iframes/become-composited-nested-iframes.html
compositing/iframes/overlapped-nested-iframes.html

[Tien-Ren Chen]

Created attachment 130677 [details]
Patch

[James Robinson]

Comment on attachment 130677 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130677&action=review

Nice, and clean EWS to boot. R=me, just need to fix up some comments and put this back behind the thread guard to land.

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:90
> +    if (scrollbar->isCustomScrollbar() /* || !CCProxy::hasImplThread() */) { // FIXME: also filter out single thread case when we

think we need to uncomment this check before landing

> Source/WebCore/page/scrolling/chromium/ScrollingCoordinatorChromium.cpp:107
> +    if (!scrollLayer) // FIXME: sometimes we get called before setScrollLayer, workaround by finding out ourself

comment isn't quite right - typo in "ourself" and lacks a period at the end (same goes for the one on line 119)

[James Robinson]

Committed r110338: <http://trac.webkit.org/changeset/110338>