Bug 91688 – [chromium] Add CCScrollbarAnimationController class for compositor scrollbar animation

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

[Tien-Ren Chen]

[chromium] Add CCScrollbarAnimationController class for compositor scrollbar animation

[Tien-Ren Chen]

Created attachment 153127 [details]
Patch

[WebKit Review Bot]

Comment on attachment 153127 [details]
Patch

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

[Adrienne Walker]

Comment on attachment 153127 [details]
Patch

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

This patch is great.  I really like the switch from pulling information from layers to pushing it there.

> Source/WebCore/page/FrameView.cpp:652
> +#if OS(ANDROID)
> +    if (m_frame->page()->mainFrame() == m_frame) {
> +        if (hMode == ScrollbarAuto)
> +            hMode = ScrollbarAlwaysOn;
> +        if (vMode == ScrollbarAuto)
> +            vMode = ScrollbarAlwaysOn;
> +    }
> +#endif

Is there a better place for this? How do other ports set the kind of scrollbars they want?

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarAnimationControllerAndroid.h:34
> +    CCScrollbarAnimationControllerAndroid(CCLayerImpl* scrollLayer);

1-arg ctor needs the explicit keyword.  Can you also make a create function that returns a PassOwnPtr?

> Source/WebCore/platform/graphics/chromium/cc/CCScrollbarAnimationControllerAndroid.h:40
> +    virtual bool animate(double monotonicTime);
> +    virtual void didPinchGestureUpdate();
> +    virtual void didPinchGestureEnd();
> +    virtual void updateScrollOffset(CCLayerImpl* scrollLayer);

OVERRIDE

[Tien-Ren Chen]

Created attachment 154796 [details]
Patch

[Tien-Ren Chen]

Also added a new test to verify scroll offsets are propagated properly.

(In reply to comment #3)
> (From update of attachment 153127 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=153127&action=review
> 
> This patch is great.  I really like the switch from pulling information from layers to pushing it there.
> 
> > Source/WebCore/page/FrameView.cpp:652
> > +#if OS(ANDROID)
> > +    if (m_frame->page()->mainFrame() == m_frame) {
> > +        if (hMode == ScrollbarAuto)
> > +            hMode = ScrollbarAlwaysOn;
> > +        if (vMode == ScrollbarAuto)
> > +            vMode = ScrollbarAlwaysOn;
> > +    }
> > +#endif
> 
> Is there a better place for this? How do other ports set the kind of scrollbars they want?

I also feel this might be too deep into WebCore... but seems like this is the only place we can override scrollbar visibility. :(

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarAnimationControllerAndroid.h:34
> > +    CCScrollbarAnimationControllerAndroid(CCLayerImpl* scrollLayer);
> 
> 1-arg ctor needs the explicit keyword.  Can you also make a create function that returns a PassOwnPtr?

done

> > Source/WebCore/platform/graphics/chromium/cc/CCScrollbarAnimationControllerAndroid.h:40
> > +    virtual bool animate(double monotonicTime);
> > +    virtual void didPinchGestureUpdate();
> > +    virtual void didPinchGestureEnd();
> > +    virtual void updateScrollOffset(CCLayerImpl* scrollLayer);
> 
> OVERRIDE

done

[Tien-Ren Chen]

Created attachment 154800 [details]
Patch

[WebKit Review Bot]

Comment on attachment 154800 [details]
Patch

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

New failing tests:
CCLayerImplTest.verifyLayerChangesAreTrackedProperly

[WebKit Review Bot]

Created attachment 154878 [details]
Archive of layout-test-results from gce-cr-linux-05

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-05  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[James Robinson]

Comment on attachment 154796 [details]
Patch

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

I like, but please split the FrameView.cpp changes into a different bug and rebase this patch (it seems to not apply and may have bitrotted a bit).

> Source/WebCore/page/FrameView.cpp:647
> +// On Android (and potentially other platform that supports pinch zoom) 
> +// the main frame should always create scrollbars unless hidden explicitly,
> +// as the viewport size can change in the compositor

can you split this change into a separate patch where we can discuss the right way to do this?

[James Robinson]

Ah, I see you've rebased it.  Any idea what's up with the test?

[Tien-Ren Chen]

(In reply to comment #9)
> (From update of attachment 154796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154796&action=review
> 
> I like, but please split the FrameView.cpp changes into a different bug and rebase this patch (it seems to not apply and may have bitrotted a bit).
> > Source/WebCore/page/FrameView.cpp:647
> > +// On Android (and potentially other platform that supports pinch zoom) 
> > +// the main frame should always create scrollbars unless hidden explicitly,
> > +// as the viewport size can change in the compositor
> 
> can you split this change into a separate patch where we can discuss the right way to do this?

Roger that, will split the patch.

(In reply to comment #10)
> Ah, I see you've rebased it.  Any idea what's up with the test?

For the unit tests I probably forgot to update some of the tests. As for layout test it seems to be some RenderText is off by one pixel, probably irrelevant. let's see if the error persists.

[Adrienne Walker]

Comment on attachment 154800 [details]
Patch

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

I would really like a unit test for CCScrollbarAnimationControllerAndroid.  If somebody refactors something and it breaks, then we won't know.

Could you refactor CCScrollbarAnimationControllerAndroid into a CCScrollbarAnimationControllerLinearFade(float hideDelay, float fadeoutLen) and then have Android just instantiate that with the values that it needs? This would let you test that this class has reasonable opacity values and handles pinches correctly on all platforms.

> Source/WebKit/chromium/tests/ScrollbarLayerChromiumTest.cpp:163
> +    scrollbarController->updateScrollOffset(ccLayerTreeRoot.get());

Why do you need to call this explicitly in this test?

[Tien-Ren Chen]

(In reply to comment #10)
> Ah, I see you've rebased it.  Any idea what's up with the test?

Ah, the CCLayerImplTest.verifyLayerChangesAreTrackedProperly failure is an interesting one.

I added early exit for CCLayerImpl::scrollBy so it becomes a no-op if newDelta == m_scrollDelta. We never set scrollMaximum on the layer in the test, thus scrollBy exits early and none of the layers are marked as changed.

Does it break any assumption to do an early exit in scrollBy?

(In reply to comment #12)
> (From update of attachment 154800 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154800&action=review
> 
> I would really like a unit test for CCScrollbarAnimationControllerAndroid.  If somebody refactors something and it breaks, then we won't know.
> 
> Could you refactor CCScrollbarAnimationControllerAndroid into a CCScrollbarAnimationControllerLinearFade(float hideDelay, float fadeoutLen) and then have Android just instantiate that with the values that it needs? This would let you test that this class has reasonable opacity values and handles pinches correctly on all platforms.

Sounds like a great way to reuse the code!

I think we want to verify that the scrollbar will show in responding to the user gesture, and will eventually fade out as time goes. Is there a good way to simulate monotonicallyIncreasingTime()?

> > Source/WebKit/chromium/tests/ScrollbarLayerChromiumTest.cpp:163
> > +    scrollbarController->updateScrollOffset(ccLayerTreeRoot.get());
> 
> Why do you need to call this explicitly in this test?

No we don't, and I agree it's probably wrong to require so. Fixed.

[Tien-Ren Chen]

Created attachment 155376 [details]
Patch

[Tien-Ren Chen]

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

This new patch fixes CCLayerImplTest.verifyLayerChangesAreTrackedProperly test by rearranging the order of property changes (move setMaxScrollPosition before scrollBy)

Also renamed the android animator to linear fade animator.

Tests for the linear fade animator hasn't been added yet. Need some way to mock monotonicallyIncreasingTime.

[Adrienne Walker]

Comment on attachment 155376 [details]
Patch

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

> Source/WebCore/page/FrameView.cpp:655
> +// On Android (and potentially other platform that supports pinch zoom)
> +// the main frame should always create scrollbars unless hidden explicitly,
> +// as the viewport size can change in the compositor
> +#if OS(ANDROID)
> +    if (m_frame->page()->mainFrame() == m_frame) {
> +        if (hMode == ScrollbarAuto)
> +            hMode = ScrollbarAlwaysOn;
> +        if (vMode == ScrollbarAuto)
> +            vMode = ScrollbarAlwaysOn;
> +    }
> +#endif

I thought this was going into a separate patch?

> Source/WebCore/rendering/RenderLayer.cpp:2961
> -    if (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant())
> +    if (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant() && !(paintFlags & PaintLayerPaintingOverlayScrollbars))

Where did all these RenderLayer changes come from in this latest patch?

[Adrienne Walker]

(In reply to comment #13)

> I added early exit for CCLayerImpl::scrollBy so it becomes a no-op if newDelta == m_scrollDelta. We never set scrollMaximum on the layer in the test, thus scrollBy exits early and none of the layers are marked as changed.
> 
> Does it break any assumption to do an early exit in scrollBy?

That seems totally reasonable.

> I think we want to verify that the scrollbar will show in responding to the user gesture, and will eventually fade out as time goes. Is there a good way to simulate monotonicallyIncreasingTime()?

I'm not sure I understand what the problem is. You should just be able to pass arbitrary monotonically increasing time values to functions that need them.

[Tien-Ren Chen]

(In reply to comment #17)
> (In reply to comment #13)
> > I think we want to verify that the scrollbar will show in responding to the user gesture, and will eventually fade out as time goes. Is there a good way to simulate monotonicallyIncreasingTime()?
> 
> I'm not sure I understand what the problem is. You should just be able to pass arbitrary monotonically increasing time values to functions that need them.

That's a reasonable way to do it, but will make the client code very awkward. For example, CCLayerImpl doesn't have any dependency to monotonicallyIncreasingTime, however CCLayerImpl::scrollBy needs the timestamp to pass it to CCScrollbarAnimator::updateScrollOffset. If we let CCLayerImpl to generate the timestamp, now it's the CCLayerImpl becomes un-testable. Or again we can require the client of CCLayerImpl to pass in the current timestamp... There is just no end to this.

I think it is not a bad assumption to have only one global monotonic clock singleton, and who ever needs it just generate it instead of requiring the client to pass it in. Just that we need a way to mock the monotonic clock in unit tests.

(In reply to comment #16)
> (From update of attachment 155376 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155376&action=review
> 
> > Source/WebCore/page/FrameView.cpp:655
> > +// On Android (and potentially other platform that supports pinch zoom)
> > +// the main frame should always create scrollbars unless hidden explicitly,
> > +// as the viewport size can change in the compositor
> > +#if OS(ANDROID)
> > +    if (m_frame->page()->mainFrame() == m_frame) {
> > +        if (hMode == ScrollbarAuto)
> > +            hMode = ScrollbarAlwaysOn;
> > +        if (vMode == ScrollbarAuto)
> > +            vMode = ScrollbarAlwaysOn;
> > +    }
> > +#endif
> 
> I thought this was going into a separate patch?

Ah I forgot to add -g option when preparing the patch. Let me re-upload it.

> > Source/WebCore/rendering/RenderLayer.cpp:2961
> > -    if (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant())
> > +    if (!isSelfPaintingLayer() && !hasSelfPaintingLayerDescendant() && !(paintFlags & PaintLayerPaintingOverlayScrollbars))
> 
> Where did all these RenderLayer changes come from in this latest patch?

Same as above. It is another patch to solve an issue with main thread side overlay scrollbar rendering.

[Tien-Ren Chen]

Created attachment 155392 [details]
Patch

[Tien-Ren Chen]

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

Rebased and re-uploaded with relevant changes omitted.

[Tien-Ren Chen]

Created attachment 155963 [details]
Patch

[Adrienne Walker]

Comment on attachment 155963 [details]
Patch

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

R=me.

> Source/WebKit/chromium/tests/CCScrollbarAnimationControllerLinearFadeTest.cpp:38
> +class CCScrollbarAnimationControllerLinearFadeTest : public testing::Test {

Thank you.  This is exactly the sort of test I was expecting to see.  :)

[WebKit Review Bot]

Comment on attachment 155963 [details]
Patch

Clearing flags on attachment: 155963

Committed r124513: <http://trac.webkit.org/changeset/124513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.