Bug 89908 – [chromium] ScrollbarLayerChromium should support painting forward-track and back-track in different styles.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[W. James MacLean]

[chromium] ScrollbarLayerChromium should support painting forward-track and back-track in different styles.

[W. James MacLean]

Created attachment 149357 [details]
Patch

[W. James MacLean]

Comment on attachment 149357 [details]
Patch

Not for review yet, still working on tests. I have some questions though:

1) Is it worth modifying it to just use a single background part if the forward- and back- styles are the same?

2) Is it worth trying to collect the two background parts into an "array of parts"? I'm assuming 'no', but I am flexible on this.

Let me know if the overall approach looks OK.

[Adrienne Walker]

Comment on attachment 149357 [details]
Patch

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

> Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:161
> +    ScrollbarPart m_part;

bikeshed: m_trackPart

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:86
> +    IntRect backTrackRect(IntPoint(), m_scrollbar.orientation() == HorizontalScrollbar ? IntSize(thumbRect.x(), thumbRect.height()) : IntSize(thumbRect.width(), thumbRect.y()));

Use theme->splitTrackRect for this.

[Adrienne Walker]

(In reply to comment #2)
> (From update of attachment 149357 [details])
> Not for review yet, still working on tests. I have some questions though:
> 
> 1) Is it worth modifying it to just use a single background part if the forward- and back- styles are the same?

I think it might be, just to save on painting and uploading the scrollbar on any frame where you scroll.  As far as I can tell, only render theme scrollbars are capable of this (so far), so you could just set a bool on the layer based on isCustomScrollbar.

> 2) Is it worth trying to collect the two background parts into an "array of parts"? I'm assuming 'no', but I am flexible on this.

I don't think there needs to be an array of parts until we support being able to draw enabled/disabled/hovered parts on the compositor thread.  I think that's the point where it goes from 2-3 parts to an array of parts in different states.

[W. James MacLean]

Created attachment 149573 [details]
Patch

[W. James MacLean]

(In reply to comment #3)
> (From update of attachment 149357 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149357&action=review
> 
> > Source/WebCore/platform/graphics/chromium/ScrollbarLayerChromium.cpp:161
> > +    ScrollbarPart m_part;
> 
> bikeshed: m_trackPart

Sure!
 
> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:86
> > +    IntRect backTrackRect(IntPoint(), m_scrollbar.orientation() == HorizontalScrollbar ? IntSize(thumbRect.x(), thumbRect.height()) : IntSize(thumbRect.width(), thumbRect.y()));
> 
> Use theme->splitTrackRect for this.

Done. I got rid of the call to theme->thumbRect since it calls splitRect anyways, although this required tweaking the thumbRect offset calculation.

(In reply to comment #4)
> (In reply to comment #2)
> > (From update of attachment 149357 [details] [details])
> > Not for review yet, still working on tests. I have some questions though:
> > 
> > 1) Is it worth modifying it to just use a single background part if the forward- and back- styles are the same?
> 
> I think it might be, just to save on painting and uploading the scrollbar on any frame where you scroll.  As far as I can tell, only render theme scrollbars are capable of this (so far), so you could just set a bool on the layer based on isCustomScrollbar.

Done. I didn't create any extra state, rather we only create/paint the second texture if isCustomScrollBar() returns true, and then the presence or lack of foreTrackTextureId determines how we go about appending quads.

> > 2) Is it worth trying to collect the two background parts into an "array of parts"? I'm assuming 'no', but I am flexible on this.
> 
> I don't think there needs to be an array of parts until we support being able to draw enabled/disabled/hovered parts on the compositor thread.  I think that's the point where it goes from 2-3 parts to an array of parts in different states.

SGTM.

Question: are we doing layout tests for gpu-scrollbars, or should I come up with a unit test? The cl passes existing unit tests and (I think) doesn't trigger any layout test failures.

[Adrienne Walker]

Comment on attachment 149573 [details]
Patch

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

No, there aren't existing good tests for this.  This path is hit in compositing tests, but (I think) only by mock scrollbars.

If you could add a pixel layout test with themed scrollbars and a different back/forward track rect for this, that'd be great.  You'll need to explicitly turn off mock scrollbars in the test.  You could probably just put it in platform/chromium/compositing/scrollbars or something.

Other than that, looks great.  Thanks!

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:38
> +#include <stdio.h>
> +

Debugging code?

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:-64
> +    IntRect thumbRect, backTrackRect, foreTrackRect;
> +    theme->splitTrack(&m_scrollbar, boundsRect, backTrackRect, thumbRect, foreTrackRect);
> +    if (m_scrollbar.orientation() == VerticalScrollbar)
> +        thumbRect.move(0, theme->backButtonRect(&m_scrollbar, BackButtonStartPart, true).size().height());
> +    else
> +        thumbRect.move(theme->backButtonRect(&m_scrollbar, BackButtonStartPart, true).size().width(), 0);
>  
> -    IntRect thumbRect = theme->thumbRect(&m_scrollbar);
> -    thumbRect.move(-m_scrollbar.x(), -m_scrollbar.y());

Why do you need to move the thumbRect like this here? Shouldn't it be positioned properly with respect to the track?

[W. James MacLean]

(In reply to comment #7)
> (From update of attachment 149573 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149573&action=review
> 
> No, there aren't existing good tests for this.  This path is hit in compositing tests, but (I think) only by mock scrollbars.
> 
> If you could add a pixel layout test with themed scrollbars and a different back/forward track rect for this, that'd be great.  You'll need to explicitly turn off mock scrollbars in the test.  You could probably just put it in platform/chromium/compositing/scrollbars or something.
> 
> Other than that, looks great.  Thanks!

Will do this and upload, probably early tomorrow.

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:38
> > +#include <stdio.h>
> > +
> 
> Debugging code?

Ooops! (blushes)

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:-64
> > +    IntRect thumbRect, backTrackRect, foreTrackRect;
> > +    theme->splitTrack(&m_scrollbar, boundsRect, backTrackRect, thumbRect, foreTrackRect);
> > +    if (m_scrollbar.orientation() == VerticalScrollbar)
> > +        thumbRect.move(0, theme->backButtonRect(&m_scrollbar, BackButtonStartPart, true).size().height());
> > +    else
> > +        thumbRect.move(theme->backButtonRect(&m_scrollbar, BackButtonStartPart, true).size().width(), 0);
> >  
> > -    IntRect thumbRect = theme->thumbRect(&m_scrollbar);
> > -    thumbRect.move(-m_scrollbar.x(), -m_scrollbar.y());
> 
> Why do you need to move the thumbRect like this here? Shouldn't it be positioned properly with respect to the track?

I was a bit surprised by this too.

It seems to calculate the thumbRect position as an offset from the track, but *exclusive* from the forward/backward buttons. Calling splitTrack this way is a bit different from calling it through thumbRect(), since the latter only passes the tracRect to splitTrack, whereas we are passing in the entire bounds rect (since we are bundling the buttons and the track into the texture). I found that if I called splitTrack with theme->trackRect() instead of bounds, I had to then manually append rects for the buttons.

[Adrienne Walker]

Comment on attachment 149573 [details]
Patch

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

>>> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:-64
>>> -    thumbRect.move(-m_scrollbar.x(), -m_scrollbar.y());
>> 
>> Why do you need to move the thumbRect like this here? Shouldn't it be positioned properly with respect to the track?
> 
> I was a bit surprised by this too.
> 
> It seems to calculate the thumbRect position as an offset from the track, but *exclusive* from the forward/backward buttons. Calling splitTrack this way is a bit different from calling it through thumbRect(), since the latter only passes the tracRect to splitTrack, whereas we are passing in the entire bounds rect (since we are bundling the buttons and the track into the texture). I found that if I called splitTrack with theme->trackRect() instead of bounds, I had to then manually append rects for the buttons.

Oh! That's the problem exactly.  splitTrack should take a trackRect, not a boundsRect.

[W. James MacLean]

(In reply to comment #9)
> (From update of attachment 149573 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149573&action=review
> 
> >>> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:-64
> >>> -    thumbRect.move(-m_scrollbar.x(), -m_scrollbar.y());
> >> 
> >> Why do you need to move the thumbRect like this here? Shouldn't it be positioned properly with respect to the track?
> > 
> > I was a bit surprised by this too.
> > 
> > It seems to calculate the thumbRect position as an offset from the track, but *exclusive* from the forward/backward buttons. Calling splitTrack this way is a bit different from calling it through thumbRect(), since the latter only passes the tracRect to splitTrack, whereas we are passing in the entire bounds rect (since we are bundling the buttons and the track into the texture). I found that if I called splitTrack with theme->trackRect() instead of bounds, I had to then manually append rects for the buttons.
> 
> Oh! That's the problem exactly.  splitTrack should take a trackRect, not a boundsRect.

OK, so I'll go with manually appending the button rects so as to pass a trackRect (although splitTrack() does seem to modify the trackRect internally to get it right). We'll still need to offset for the thumbRect though to align it with our bounds.

[Adrienne Walker]

(In reply to comment #10)
> (In reply to comment #9)
> > (From update of attachment 149573 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=149573&action=review
> > 
> > >>> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:-64
> > >>> -    thumbRect.move(-m_scrollbar.x(), -m_scrollbar.y());
> > >> 
> > >> Why do you need to move the thumbRect like this here? Shouldn't it be positioned properly with respect to the track?
> > > 
> > > I was a bit surprised by this too.
> > > 
> > > It seems to calculate the thumbRect position as an offset from the track, but *exclusive* from the forward/backward buttons. Calling splitTrack this way is a bit different from calling it through thumbRect(), since the latter only passes the tracRect to splitTrack, whereas we are passing in the entire bounds rect (since we are bundling the buttons and the track into the texture). I found that if I called splitTrack with theme->trackRect() instead of bounds, I had to then manually append rects for the buttons.
> > 
> > Oh! That's the problem exactly.  splitTrack should take a trackRect, not a boundsRect.
> 
> OK, so I'll go with manually appending the button rects so as to pass a trackRect (although splitTrack() does seem to modify the trackRect internally to get it right). We'll still need to offset for the thumbRect though to align it with our bounds.

By trackRect, I was trying to say that it'd be better to use ScrollbarThemeComposite::trackRect(ScrollbarThemeClient*) like thumbRect() does rather than trying to manually construct this rect.

Button placement can be overridden by scrollbars, so it's not really safe to use their geometry to reposition other parts.

[W. James MacLean]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > (From update of attachment 149573 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=149573&action=review
> > > 
> > > >>> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:-64
> > > >>> -    thumbRect.move(-m_scrollbar.x(), -m_scrollbar.y());
> > > >> 
> > > >> Why do you need to move the thumbRect like this here? Shouldn't it be positioned properly with respect to the track?
> > > > 
> > > > I was a bit surprised by this too.
> > > > 
> > > > It seems to calculate the thumbRect position as an offset from the track, but *exclusive* from the forward/backward buttons. Calling splitTrack this way is a bit different from calling it through thumbRect(), since the latter only passes the tracRect to splitTrack, whereas we are passing in the entire bounds rect (since we are bundling the buttons and the track into the texture). I found that if I called splitTrack with theme->trackRect() instead of bounds, I had to then manually append rects for the buttons.
> > > 
> > > Oh! That's the problem exactly.  splitTrack should take a trackRect, not a boundsRect.
> > 
> > OK, so I'll go with manually appending the button rects so as to pass a trackRect (although splitTrack() does seem to modify the trackRect internally to get it right). We'll still need to offset for the thumbRect though to align it with our bounds.
> 
> By trackRect, I was trying to say that it'd be better to use ScrollbarThemeComposite::trackRect(ScrollbarThemeClient*) like thumbRect() does rather than trying to manually construct this rect.

Yes, this is what I was describing above. It still seems to ignore the buttons when positioning the thumbRect.

> Button placement can be overridden by scrollbars, so it's not really safe to use their geometry to reposition other parts.

Hmm, OK. Let me take a look and see what I can learn about this. But everything lives within the content bounds I assume ...

[W. James MacLean]

(In reply to comment #11)
> 
> Button placement can be overridden by scrollbars, so it's not really safe to use their geometry to reposition other parts.

Is there a concise reference to tell me which parts can go where? I know how to do this now, once I can map the right parts to the right names.

[W. James MacLean]

(In reply to comment #13)
> (In reply to comment #11)
> > 
> > Button placement can be overridden by scrollbars, so it's not really safe to use their geometry to reposition other parts.
> 
> Is there a concise reference to tell me which parts can go where? I know how to do this now, once I can map the right parts to the right names.

Got it I think ... new patch in the AM ...

[W. James MacLean]

Created attachment 149773 [details]
Patch

[W. James MacLean]

(In reply to comment #15)
> Created an attachment (id=149773) [details]
> Patch

OK ... we now have a layout test, and (I think) a more rational way of handling the buttons. My mild bout of insanity regarding thumb positioning seems also to have passed.

[Adrienne Walker]

Comment on attachment 149773 [details]
Patch

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

So close.  :)

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarLayerImpl.cpp:99
> +        // Assumes backTrackRect, ForwardButtonStartPart and BackButtonStartPart form a connected, rectangular region.
> +        backTrackRect.unite(theme->forwardButtonRect(&m_scrollbar, ForwardButtonStartPart));
> +        backTrackRect.unite(theme->backButtonRect(&m_scrollbar, BackButtonStartPart));
> +        if (!backTrackRect.isEmpty())
> +            quadList.append(CCTextureDrawQuad::create(sharedQuadState, backTrackRect, m_backTrackTextureId, premultipledAlpha, toUVRect(backTrackRect, boundsRect), flipped));
> +
> +        foreTrackRect.unite(theme->forwardButtonRect(&m_scrollbar, ForwardButtonEndPart));
> +        foreTrackRect.unite(theme->backButtonRect(&m_scrollbar, BackButtonEndPart));
> +        if (!foreTrackRect.isEmpty())
> +            quadList.append(CCTextureDrawQuad::create(sharedQuadState, foreTrackRect, m_foreTrackTextureId, premultipledAlpha, toUVRect(foreTrackRect, boundsRect), flipped));

I still don't like the button assumptions.  Is there some way to get rid of them?

backTrack and foreTrack contain the entire content rect.  What about appending foreTrack with the foreTrackRect (as here), but then appending the backTrack quad using boundsRect (as you do in the else case)? In other words, make the forward track quad just represent the forward track part and make the backward track quad represent the background, the buttons and the back track rect?

> LayoutTests/platform/chromium/compositing/scrollbars/custom-composited-different-track-parts.html:68
> +<h2>Composited Custom Scrollbars with Different Coloured Track Parts</h2>
> +If this test succeeds, the track part before the thumb will be purple, and the track part after
> +the thumb will be green. All four button rects will have different colours.

No text in pixel tests please, since it makes them have cross-platform results.  You can make this a comment in the html if you want to explain what it's supposed to look like.

[W. James MacLean]

(In reply to comment #17)
> (From update of attachment 149773 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=149773&action=review
> 
> So close.  :)
> 


> I still don't like the button assumptions.  Is there some way to get rid of them?
> 
> backTrack and foreTrack contain the entire content rect.  What about appending foreTrack with the foreTrackRect (as here), but then appending the backTrack quad using boundsRect (as you do in the else case)? In other words, make the forward track quad just represent the forward track part and make the backward track quad represent the background, the buttons and the back track rect?

Sure, this can be done (there will be a little bit of overdraw, but otherwise this will work).
 
> > LayoutTests/platform/chromium/compositing/scrollbars/custom-composited-different-track-parts.html:68
> > +<h2>Composited Custom Scrollbars with Different Coloured Track Parts</h2>
> > +If this test succeeds, the track part before the thumb will be purple, and the track part after
> > +the thumb will be green. All four button rects will have different colours.
> 
> No text in pixel tests please, since it makes them have cross-platform results.  You can make this a comment in the html if you want to explain what it's supposed to look like.

Will remove text asap ... will look kind of bare, but I can live with that.

[W. James MacLean]

Created attachment 149785 [details]
Patch

[Adrienne Walker]

Comment on attachment 149785 [details]
Patch

R=me.  Thanks for all the changes.

[WebKit Review Bot]

Comment on attachment 149785 [details]
Patch

Clearing flags on attachment: 149785

Committed r121371: <http://trac.webkit.org/changeset/121371>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.