Implement CSSOM Smooth Scroll API

Implement CSSOM Smooth Scroll API

This implements the CSSOM Smooth Scroll API, except for smooth scrolling
for Element.scrollIntoView and for fragment scrolling, which are left for
a follow-up.

Spec: http://www.w3.org/TR/cssom-view/
Intent-to-implement: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/YLWs9N46XjQ

BUG=243871

[ajuma, 2014-01-10 20:38:46]

I still need to add tests, but I'm going ahead and posting this for initial
feedback.

[ajuma, 2014-02-04 20:31:39]

PTAL. The bindings and CSS changes have landed separately, and I've added tests.

[Ian Vollick, 2014-02-06 16:02:30]

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
File LayoutTests/fast/scroll-behavior/listbox-interrupted-scroll.html (right):

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/listbox-interrupted-scroll.html:33: var
innerPoint = {x:0, y:0 };
Nit: extra space.

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
File LayoutTests/fast/scroll-behavior/listbox-scrollTop.html (right):

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/listbox-scrollTop.html:9: var instantScrolls =
[
If you make an object for the behavior test case, perhaps you could subclass for
the listbox that adds the 'index' field and a description of what it means. It
means 'the index of the list box item to scroll to', right?

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
File LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js (right):

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js:1:
(function() {
Please add a comment describing what a scroll behavior test
is and how they work (briefly what a test case needs to provide).

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js:19: if
(smoothScrolls[currentSmoothTest].waitForEnd) {
This is a bit nitty, but I find the bucket of globals and free functions a
little ugly. It feels like this really wants to be an object. For example, if
smoothScrollListener was part of the test object's prototype, you could bind()
it and attach it and access the test data on |this| in a natural way.

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js:40:
testElement.style.scrollBehavior = instantScrolls[i].css;
A comment here would be helpful (that explains this function needs to be
implemented by the test case and what it's intended to do).

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js:61:
testElement.style.scrollBehavior = smoothScrolls[currentSmoothTest].css;
Ditto.

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js:74:
runScrollBehaviorTests = function runScrollBehaviorTests(element,
Does this function need to be named?

Please explain what these arguments are (endX and endY in particular -- it's not
obvious at first glance that these are functions that will be used to calculate
the end x and y later. A 'callback' suffix (or something) would really help,
too).

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-behavior-test.js:88:
runInstantTestCases();
I'm hoping you go the OO route as I described earlier. If you do, you can attach
the per-test-case listener in startNextSmoothTestCase.

If you don't, please add a comment saying that this is function is responsible
for reporting when smooth scroll tests (which are asynchronous naturally) are
finished so that we can progress to the next test.

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
File LayoutTests/fast/scroll-behavior/resources/scroll-interruption-test.js
(right):

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-interruption-test.js:16:
assert_equals(testElement.scrollTop, testCase.y3);
I don't like the assumptions about the contents of testCase. The implicit
dependency a bit brittle and undocumented. Please define a test case class here,
expose it to the global scope, and have the layout tests instantiate instances
explicitly. Comments describing the fields in the case would be very helpful. y1
y2 and y3 are a bit terse.

Same for the scroll behavior test case.

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-interruption-test.js:188:
runScrollInterruptionTests = function runScrollInterruptionTests(element,
Does this function need to be named?

https://codereview.chromium.org/134443003/diff/210001/LayoutTests/fast/scroll...
LayoutTests/fast/scroll-behavior/resources/scroll-interruption-test.js:193:
testInnerPoint = innerPoint;
It's a bit funky that startInstantTest does more than just starting an instant
test, it sets up a cascade of tests starting with the instant test.

Perhaps you could go the OO route again here. The object would be responsible
for managing the invokation of the sub tests. So rather than having
startInstantTest directly start the next type of test, it could call the objects
iAmDoneMySubTest member function which would get the next one started.

It would have been much easier for me to understand if you'd done something like
this.
var runner = new InterruptionTestsRunner(element, scrolls, innerPoint);
runner.start();

https://codereview.chromium.org/134443003/diff/210001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/134443003/diff/210001/Source/core/dom/Element...
Source/core/dom/Element.cpp:760: exceptionState.throwTypeError("The
ScrollBehavior provided is invalid.");
Do you have tests that try setting a bad behavior and check that they catch this
exception?

https://codereview.chromium.org/134443003/diff/210001/Source/core/rendering/R...
File Source/core/rendering/RenderLayerScrollableArea.cpp (right):

https://codereview.chromium.org/134443003/diff/210001/Source/core/rendering/R...
Source/core/rendering/RenderLayerScrollableArea.cpp:114:
frameView->removeScrollableArea(this);
Can't we just removeAnimationScrollableArea from removeScrollableArea?

https://codereview.chromium.org/134443003/diff/210001/Source/platform/graphic...
File Source/platform/graphics/GraphicsLayer.cpp (right):

https://codereview.chromium.org/134443003/diff/210001/Source/platform/graphic...
Source/platform/graphics/GraphicsLayer.cpp:1231:
m_scrollableArea->notifyAnimationStarted(monotonicTime);
Hmm. This is confusing at a glance. Could you add a comment explaining why you
notifyAnimationStarted from notifyAnimationFinished? Was this supposed to be
notifyAnimationFinished?

https://codereview.chromium.org/134443003/diff/210001/Source/platform/scroll/...
File Source/platform/scroll/ProgrammaticScrollAnimator.cpp (right):

https://codereview.chromium.org/134443003/diff/210001/Source/platform/scroll/...
Source/platform/scroll/ProgrammaticScrollAnimator.cpp:83:
m_animationCurve->setInitialValue(FloatPoint(m_scrollableArea->scrollPosition()));
Will ::tickAnimation continue to be called throughout a composited scroll
animation? It looks like it will, since we don't return any info here to let
anyone know whether we've started an accelerated animation?

I realize that m_animationCurve would be NULL in that case, so the function wout
do nothing, but it does feel like it would be wasted ticking.

https://codereview.chromium.org/134443003/diff/210001/Source/platform/scroll/...
File Source/platform/scroll/ProgrammaticScrollAnimator.h (right):

https://codereview.chromium.org/134443003/diff/210001/Source/platform/scroll/...
Source/platform/scroll/ProgrammaticScrollAnimator.h:31: #ifndef
ProgrammaticScrollAnimator_h
I think you can muck with --similarity to convince it to treat this as a new
file.