Chromium support for window.webkitRequestAnimationFrame()

Chromium support for window.webkitRequestAnimationFrame()

This is a very simple scheduler that targets 100fps.

BUG=64848
TEST=layout tests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=71909

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72230

[jamesr, 2011-01-11 00:36:02]

This corresponds with the patch currently up for review at
https://bugs.webkit.org/show_bug.cgi?id=51218.  Assuming everything looks good,
I'll have to land the impls of the functions added to WebWidget/WebWidgetClient,
wait for a roll, land the WebKit side, then land the rest.

[darin (slow to review), 2011-01-11 22:02:30]

http://codereview.chromium.org/6136005/diff/1/chrome/renderer/render_widget.cc
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/6136005/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:491: if (!is_hidden() &&
animation_update_pending_ && base::Time::Now() > animation_floor_time_) {
nit: wrap lines at 80 chars

http://codereview.chromium.org/6136005/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:509: UpdateAnimationsIfNeeded();
nit: indent by 2

http://codereview.chromium.org/6136005/diff/1/chrome/renderer/render_widget.c...
chrome/renderer/render_widget.cc:702: animation_floor_time_ = base::Time::Now()
+ base::TimeDelta::FromMilliseconds(10);
nit: indentation and line length limit

http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
webkit/tools/test_shell/test_webview_delegate.cc:687:
MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
nit: indentation

Piggy backing on scheduleComposite here might cause problems in the future if
scheduleComposite gets "fixed" to trigger a call to WebWidget::composite(). 
Maybe we should plumb a scheduleAnimation instead and then just make it an
implementation detail of the WebWidgetHost that scheduleAnimation and
scheduleComposite do the same thing?

[jamesr, 2011-01-12 00:02:43]

http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
File webkit/tools/test_shell/test_webview_delegate.cc (right):

http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
webkit/tools/test_shell/test_webview_delegate.cc:687:
MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
On 2011/01/11 22:02:30, darin wrote:
> Piggy backing on scheduleComposite here might cause problems in the future if
> scheduleComposite gets "fixed" to trigger a call to WebWidget::composite(). 
> Maybe we should plumb a scheduleAnimation instead and then just make it an
> implementation detail of the WebWidgetHost that scheduleAnimation and
> scheduleComposite do the same thing?

It's a pain to do deferred work in WebWidgetHost since it isn't
ThreadSafeRefCounted (so I can't use PostTask()).  I don't think anyone is
planning to support real composition in test_shell any time soon, so I think
(hope) the concern is mostly theoretical.

[darin (slow to review), 2011-01-12 22:00:54]

On Tue, Jan 11, 2011 at 4:02 PM, <jamesr@chromium.org> wrote:

>
>
>
http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
> File webkit/tools/test_shell/test_webview_delegate.cc (right):
>
>
>
http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
> webkit/tools/test_shell/test_webview_delegate.cc:687:
> MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
> On 2011/01/11 22:02:30, darin wrote:
>
>> Piggy backing on scheduleComposite here might cause problems in the
>>
> future if
>
>> scheduleComposite gets "fixed" to trigger a call to
>>
> WebWidget::composite().
>
>> Maybe we should plumb a scheduleAnimation instead and then just make
>>
> it an
>
>> implementation detail of the WebWidgetHost that scheduleAnimation and
>> scheduleComposite do the same thing?
>>
>
> It's a pain to do deferred work in WebWidgetHost since it isn't
> ThreadSafeRefCounted (so I can't use PostTask()).  I don't think anyone
> is planning to support real composition in test_shell any time soon, so
> I think (hope) the concern is mostly theoretical.


No need to cross threads, so you can just use
ScopedRunnableMethodFactory<T>.

http://dev.chromium.org/developers/design-documents/threading#Scoped_factories

-Darin



>
>
> http://codereview.chromium.org/6136005/
>

[jamesr1, 2011-01-12 22:02:16]

Woah!  mind=blown.  I can do that (although I'll have to write it three
times for gtk, win, and mac).

On Wed, Jan 12, 2011 at 2:00 PM, Darin Fisher <darin@chromium.org> wrote:

> On Tue, Jan 11, 2011 at 4:02 PM, <jamesr@chromium.org> wrote:
>
>>
>>
>>
http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
>> File webkit/tools/test_shell/test_webview_delegate.cc (right):
>>
>>
>>
http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
>> webkit/tools/test_shell/test_webview_delegate.cc:687:
>> MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
>> On 2011/01/11 22:02:30, darin wrote:
>>
>>> Piggy backing on scheduleComposite here might cause problems in the
>>>
>> future if
>>
>>> scheduleComposite gets "fixed" to trigger a call to
>>>
>> WebWidget::composite().
>>
>>> Maybe we should plumb a scheduleAnimation instead and then just make
>>>
>> it an
>>
>>> implementation detail of the WebWidgetHost that scheduleAnimation and
>>> scheduleComposite do the same thing?
>>>
>>
>> It's a pain to do deferred work in WebWidgetHost since it isn't
>> ThreadSafeRefCounted (so I can't use PostTask()).  I don't think anyone
>> is planning to support real composition in test_shell any time soon, so
>> I think (hope) the concern is mostly theoretical.
>
>
> No need to cross threads, so you can just use
> ScopedRunnableMethodFactory<T>.
>
>
> http://dev.chromium.org/developers/design-documents/threading#Scoped_factories
>
> -Darin
>
>
>
>>
>>
>> http://codereview.chromium.org/6136005/
>>
>
>

[darin (slow to review), 2011-01-12 22:27:09]

it seems like we could have shared code for this, right?
-darin

On Wed, Jan 12, 2011 at 2:02 PM, James Robinson <jamesr@google.com> wrote:

> Woah!  mind=blown.  I can do that (although I'll have to write it three
> times for gtk, win, and mac).
>
>
> On Wed, Jan 12, 2011 at 2:00 PM, Darin Fisher <darin@chromium.org> wrote:
>
>> On Tue, Jan 11, 2011 at 4:02 PM, <jamesr@chromium.org> wrote:
>>
>>>
>>>
>>>
http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
>>> File webkit/tools/test_shell/test_webview_delegate.cc (right):
>>>
>>>
>>>
http://codereview.chromium.org/6136005/diff/1/webkit/tools/test_shell/test_we...
>>> webkit/tools/test_shell/test_webview_delegate.cc:687:
>>> MessageLoop::current()->PostTask(FROM_HERE, NewRunnableMethod(
>>> On 2011/01/11 22:02:30, darin wrote:
>>>
>>>> Piggy backing on scheduleComposite here might cause problems in the
>>>>
>>> future if
>>>
>>>> scheduleComposite gets "fixed" to trigger a call to
>>>>
>>> WebWidget::composite().
>>>
>>>> Maybe we should plumb a scheduleAnimation instead and then just make
>>>>
>>> it an
>>>
>>>> implementation detail of the WebWidgetHost that scheduleAnimation and
>>>> scheduleComposite do the same thing?
>>>>
>>>
>>> It's a pain to do deferred work in WebWidgetHost since it isn't
>>> ThreadSafeRefCounted (so I can't use PostTask()).  I don't think anyone
>>> is planning to support real composition in test_shell any time soon, so
>>> I think (hope) the concern is mostly theoretical.
>>
>>
>> No need to cross threads, so you can just use
>> ScopedRunnableMethodFactory<T>.
>>
>>
>>
http://dev.chromium.org/developers/design-documents/threading#Scoped_factories
>>
>> -Darin
>>
>>
>>
>>>
>>>
>>> http://codereview.chromium.org/6136005/
>>>
>>
>>
>

[jamesr, 2011-01-12 22:55:30]

this puts the logic in a cross-platform webwidget_host.cc file, but i still have
to touch each platform's webwidget_host_* to modify the constructor.

[darin (slow to review), 2011-01-13 07:13:24]

LGTM

It seems like we would benefit from factoring other platform independent bits of
WebWidgetHost out, but that is definitely out of scope for this CL.

http://codereview.chromium.org/6136005/diff/13001/webkit/tools/test_shell/web...
File webkit/tools/test_shell/webwidget_host.cc (right):

http://codereview.chromium.org/6136005/diff/13001/webkit/tools/test_shell/web...
webkit/tools/test_shell/webwidget_host.cc:13: 
nit: extra whitespace at the end of the file

[jamesr, 2011-01-20 03:01:40]

commit the render_widget_fullscreen_pepper.cc change so the WebKit side could
land.  Will land the rest after the WebKit roll (so I see the WebWidget
interface change).