[webkit-reviews] review denied: [Bug 78872] [chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side scrollbar painting : [Attachment 130213] Patch

[bugzilla-daemon at webkit.org]

James Robinson <jamesr at chromium.org> has denied Tien-Ren Chen
<trchen at chromium.org>'s request for review:
Bug 78872: [chromium] ScrollbarLayerChromium/CCScrollbarLayerImpl for CC-side
scrollbar painting
https://bugs.webkit.org/show_bug.cgi?id=78872

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

------- Additional Comments from James Robinson <jamesr at chromium.org>
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?