Enable Scroll Animation for RenderLayerScrollableArea

Enable Scroll Animation for RenderLayerScrollableArea

In Blink, only FrameView scrollbars animation is enabled and serviced.
This patch enables scroll animation for non-mainframe scrollbars (
RenderLayerScrollableArea) and service them.

BUG=575, 299059, 307578, 359028
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172418

[Peter Kasting, 2014-03-27 21:00:10]

I'm not an appropriate reviewer here; I've never touched any of this code.

Please find a reviewer who has worked on this.

[weiliangc, 2014-03-27 23:48:03]

On 2014/03/27 21:00:10, Peter Kasting wrote:
> I'm not an appropriate reviewer here; I've never touched any of this code.
> 
> Please find a reviewer who has worked on this.

Thanks, changed to more relevant reviewer.
(I thought I already replied... dejavu?)

[abarth-chromium, 2014-03-28 00:03:03]

Test?

[weiliangc, 2014-04-10 13:28:12]

Add Layout test

+ajuma

[abarth-chromium, 2014-04-10 21:05:33]

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:39: (*it)->serviceScrollAnimations();
Don't we need to collect up all the RefPtr<Documents> before calling this
function?

[weiliangc, 2014-04-10 21:26:38]

Found problem with current LayoutTest, WIP.

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (left):

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:38: Vector<RefPtr<Document> > documents;
This was moved because we are traversing the frames a 2nd time to collect
RefPtr<Document> while it could be done on the first traverse.

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:39: (*it)->serviceScrollAnimations();
On 2014/04/10 21:05:34, abarth wrote:
> Don't we need to collect up all the RefPtr<Documents> before calling this
> function?

Every frame owns a list of scrollableAreas that contains scrollbars. So
traversing all scrollableAreas inside all frames should cover all scrollable
area.

[abarth-chromium, 2014-04-10 22:21:32]

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (left):

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:38: Vector<RefPtr<Document> > documents;
On 2014/04/10 21:26:39, weiliangc wrote:
> This was moved because we are traversing the frames a 2nd time to collect
> RefPtr<Document> while it could be done on the first traverse.

I suspect we need to move this to the top of the function and then always
traverse this vector instead of touching the FrameTree again.

[weiliangc, 2014-04-14 17:12:38]

Added correct Layout test. PTAL.

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (left):

https://codereview.chromium.org/214953004/diff/60001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:38: Vector<RefPtr<Document> > documents;
On 2014/04/10 22:21:32, abarth wrote:
> On 2014/04/10 21:26:39, weiliangc wrote:
> > This was moved because we are traversing the frames a 2nd time to collect
> > RefPtr<Document> while it could be done on the first traverse.
> 
> I suspect we need to move this to the top of the function and then always
> traverse this vector instead of touching the FrameTree again.

I moved it to (almost) top of function(line 32), and this function collects
RefPtr<Document> when traverse the frame tree the first time instead of traverse
the tree twice.

Is this what you mean?

[abarth-chromium, 2014-04-14 18:52:25]

https://codereview.chromium.org/214953004/diff/80001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (left):

https://codereview.chromium.org/214953004/diff/80001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:40: documents.append(frame->document());
Please just move this loop to the top of the function and then iterate the
documents Vector instead of walking the frame tree.  Yes, we'll still have
multiple loops, but that's fine.

[abarth-chromium, 2014-04-14 18:52:57]

https://codereview.chromium.org/214953004/diff/80001/Source/core/page/PageAni...
File Source/core/page/PageAnimator.cpp (left):

https://codereview.chromium.org/214953004/diff/80001/Source/core/page/PageAni...
Source/core/page/PageAnimator.cpp:40: documents.append(frame->document());
On 2014/04/14 18:52:25, abarth wrote:
> Please just move this loop to the top of the function and then iterate the
> documents Vector instead of walking the frame tree.  Yes, we'll still have
> multiple loops, but that's fine.

By the top of the function, I mean between lines 31 and 32.

[weiliangc, 2014-04-14 19:04:31]

Update to move loop to beginning of function.

https://codereview.chromium.org/214953004/diff/100001/Source/core/page/PageAn...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/214953004/diff/100001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:34: documents.append(frame->document());
Like this?

[abarth-chromium, 2014-04-14 20:15:34]

https://codereview.chromium.org/214953004/diff/100001/Source/core/page/PageAn...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/214953004/diff/100001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:34: documents.append(frame->document());
On 2014/04/14 19:04:32, weiliangc wrote:
> Like this?

Yes, thank you.

https://codereview.chromium.org/214953004/diff/100001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:36: for (RefPtr<LocalFrame> frame =
m_page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
Please iterate the documents collection instead of the Frames.  It's possible
that inside this loop, we'll run JavaScript, which can mutate the frame tree. 
That will break this iteration.

When iterating the documents, you'll need to check whether they're still present
in their frames but null checking document->frame().

[weiliangc, 2014-04-14 20:29:37]

https://codereview.chromium.org/214953004/diff/100001/Source/core/page/PageAn...
File Source/core/page/PageAnimator.cpp (right):

https://codereview.chromium.org/214953004/diff/100001/Source/core/page/PageAn...
Source/core/page/PageAnimator.cpp:36: for (RefPtr<LocalFrame> frame =
m_page->mainFrame(); frame; frame = frame->tree().traverseNext()) {
On 2014/04/14 20:15:34, abarth wrote:
> Please iterate the documents collection instead of the Frames.  It's possible
> that inside this loop, we'll run JavaScript, which can mutate the frame tree. 
> That will break this iteration.
> 
> When iterating the documents, you'll need to check whether they're still
present
> in their frames but null checking document->frame().


I need to use FrameViews. as long as document->frame() not null, I should be
able to get valid FrameView from document->view(), correct?

Also is the checking also needed for all other three looping though document
under? Or only the some of the loops?

Thanks.

[abarth-chromium, 2014-04-14 20:36:35]

On 2014/04/14 20:29:37, weiliangc wrote:
> I need to use FrameViews. as long as document->frame() not null, I should be
> able to get valid FrameView from document->view(), correct?

Correct.

> Also is the checking also needed for all other three looping though document
> under? Or only the some of the loops?

It's only needed if you need to access the Frame-level objects.  The other loops
don't seem to need to do that (or if they do, the null checks happen in some
other file when we move from a Document-level object to a Frame-level object.)

[weiliangc, 2014-04-14 21:15:58]

On 2014/04/14 20:36:35, abarth wrote:
> On 2014/04/14 20:29:37, weiliangc wrote:
> > I need to use FrameViews. as long as document->frame() not null, I should be
> > able to get valid FrameView from document->view(), correct?
> 
> Correct.
> 
> > Also is the checking also needed for all other three looping though document
> > under? Or only the some of the loops?
> 
> It's only needed if you need to access the Frame-level objects.  The other
loops
> don't seem to need to do that (or if they do, the null checks happen in some
> other file when we move from a Document-level object to a Frame-level object.)

Done. Thanks.

[abarth-chromium, 2014-04-14 21:28:26]

Thanks!  Sorry for having to go so many rounds.  LGTM

[weiliangc, 2014-04-23 18:23:03]

Add Layout test Failure expectation for Mac, PTAL.

For Mac, whether smooth scrolling is turned on depend on both command line flag
as well as system settings
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...).
For now there is no way to change system setting for single layout test at run
time (crbug.com/364614), add expected failure to Mac.

Also change in this patch should fix crash in crbug.com/359028 as well.

[abarth-chromium, 2014-04-23 20:13:45]

On 2014/04/23 18:23:03, weiliangc wrote:
> Also change in this patch should fix crash in crbug.com/359028 as well.

Do we have a test for that crash?

[abarth-chromium, 2014-04-23 20:14:09]

lgtm

[commit-bot: I haz the power, 2014-04-23 20:14:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/weiliangc@chromium.org/214953004/180001

[commit-bot: I haz the power, 2014-04-23 21:43:26]

Change committed as 172418