[Webkit-unassigned] [Bug 66280] [GTK] requestAnimationFrame support for gtk port

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=66280


Martin Robinson <mrobinson at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #106114|review?, commit-queue?      |review-
               Flag|                            |




--- Comment #4 from Martin Robinson <mrobinson at webkit.org>  2011-09-08 08:33:11 PST ---
(From update of attachment 106114)
View in context: https://bugs.webkit.org/attachment.cgi?id=106114&action=review

Nice work. Please fix all of Carlos' points as well. They all look valid.

In general, I would like to the see the animation stuff put into a WebCore class. It should be shared between WebKit1 and WebKit2, as it's almost identical.

> Source/WebKit2/WebProcess/WebPage/WebPage.h:634
> +    double m_timeStamp;

I don't think the name "m_timeStamp" is specific enough. What is the timestamp for?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:47
> +static const double AnimationThrottleTimeInterval = 0.016;

This should be called gAnimationThrottleTimeInterval. It's a magic number too, so you should have a comment explaining why it's 0.016.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:154
> +    WebPage* self = static_cast<WebPage*>(data);

You should probably call this "page" since this is a static method.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:161
> +    double elapsedTime = AnimationThrottleTimeInterval;
> +    if (timeStamp)
> +        elapsedTime = now - timeStamp;

When is timestamp zero?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:167
> +        if (self->corePage()->mainFrame() && self->corePage()->mainFrame()->view())
> +            self->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));

You access self->corePage()->mainFrame() three times, so you should cache it.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:176
> +    g_timeout_add_full(G_PRIORITY_DEFAULT, remainingTime * 1000, reinterpret_cast<GSourceFunc>(&requestAnimationFrameRunLoopCallback), (gpointer)self, 0);

Please insert newlines into lines that extend beyond 120 characters.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:184
> +    if (!m_requestAnimationFrameTimerSource) {
> +        GMainContext* gMainContext = g_main_context_default();

Please use an early return here.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:186
> +        g_main_context_ref(gMainContext);

Why ref and unref the main context? Is this happening on a thread other than the main thread?

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:190
> +        m_requestAnimationFrameTimerSource = g_timeout_source_new(static_cast<guint>(0));

Do you really need to cast 0 to guint? I find that really surprising.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:195
> +        // Or else some animations will be freezed while resizing window or scrolling.

freezed -> frozen
resizing window -> resizing the window

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:199
> +        g_source_set_callback(m_requestAnimationFrameTimerSource.get(), reinterpret_cast<GSourceFunc>(&requestAnimationFrameRunLoopCallback), (gpointer)this, 0);

You shouldn't need to cast to gpointer here.

> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:202
> +        g_main_context_unref(gMainContext);

If you must ref and unref the main context, it's better to use a GRefPtr here. You'll need to add a specialization in JavaScriptCore/wtf/gobject/GRefPtr.h and JavaScriptCore/wtf/gobject/GRefPtr.cpp.

> Source/WebKit/gtk/ChangeLog:12
> +        Implement requestAnimationFrame for WebKit gtk port.
> +        GSourceTimer is added to WebKitWebViewPriv and used to call callback function.
> +        The timer limits to call callback function more than one time in every 16ms.
> +        This timer has lower priority than G_PRIORITY_HIGH_IDLE + 20.
> +        Because, or else some dirty rects will not be updated while resizing and scrolling.

Super nit! Please even out the line breaks here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1307
> +static const double AnimationThrottleTimeInterval = 0.016; // For 60fps

This should be called gAnimationThrottleTimeInterval.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1311
> +    if (!webView)
> +        return;

I'm pretty sure you want to use an ASSERT here.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1315
> +    WebKitWebViewPrivate* priv = webView->priv;
> +    if (!priv || !priv->requestAnimationFrameTimerSource)
> +        return;

You shouldn't really need to check if priv is null.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:1342
> +        if (webView && core(webView)->mainFrame() && core(webView)->mainFrame()->view())
> +            core(webView)->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(now));
> +        return false;

Please cache core(webView)->mainFrame(). Might want to just use early return through this whole section.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.