[webkit-reviews] review denied: [Bug 89908] [chromium] ScrollbarLayerChromium should support painting forward-track and back-track in different styles. : [Attachment 149773] Patch

[bugzilla-daemon at webkit.org]

Adrienne Walker <enne at google.com> has denied W. James MacLean
<wjmaclean at chromium.org>'s request for review:
Bug 89908: [chromium] ScrollbarLayerChromium should support painting
forward-track and back-track in different styles.
https://bugs.webkit.org/show_bug.cgi?id=89908

Attachment 149773: Patch
https://bugs.webkit.org/attachment.cgi?id=149773&action=review

------- Additional Comments from Adrienne Walker <enne at google.com>
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-differen
t-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.