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

Thu Sep 8 10:45:37 PDT 2011

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

--- Comment #5 from ChangSeok Oh <kevin.cs.oh at gmail.com>  2011-09-08 10:45:37 PST ---
(From update of attachment 106114)
View in context: https://bugs.webkit.org/attachment.cgi?id=106114&action=review

Thank you for review. :)
I'm not convinced sharing code between WebKit1 & WebKit2 is possible and better way. But let me consider more.

>> configure.ac:935
>> +
> 
> Why not enable it by default?

I think this feature is experimental yet. Formal spec is not fixed so that we may need to modify this later.
But I don't mind changing default = yes.

>> 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?

I'll change m_timeStampForPreviousScene.

>> 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.

Done

>> 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.

Done

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:161
>> +        elapsedTime = now - timeStamp;
> 
> When is timestamp zero?

First call for requestAnimationFrame.

>> Source/WebKit2/WebProcess/WebPage/gtk/WebPageGtk.cpp:167
>> +            self->corePage()->mainFrame()->view()->serviceScriptedAnimations(convertSecondsToDOMTimeStamp(currentTime()));
> 
> You access self->corePage()->mainFrame() three times, so you should cache it.

Done

>> 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.

Done

>> 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?

Maybe No. I'll remove ref, unref.

>> 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.

Oops. sorry.

>> 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

Done.

>> 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.

Done

>> 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.

Let me try again after removing ref, unref. If I find any issue. then I'll add changes for GRefPtr. :)

>> Source/WebKit/gtk/ChangeLog:12
>> +        Because, or else some dirty rects will not be updated while resizing and scrolling.
> 
> Super nit! Please even out the line breaks here.

Done

>> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:673
>> +}
> 
> Maybe we could implement the animation stuff here, so that we don't need to add a new signal to web view.

Yor're right. My first implementation was like you mentioned.
But I moved code from ChromeClient to webview after seeing cmarrin's patch for mac port (bug59146). Is it better maintaining consistency among ports?

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:210
>> +#endif
> 
> I would avoid adding a new signal for this.

ditto.

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

Done

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1311
>> +        return;
> 
> I'm pretty sure you want to use an ASSERT here.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1315
>> +        return;
> 
> You shouldn't really need to check if priv is null.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1342
>> +        return false;
> 
> Please cache core(webView)->mainFrame(). Might want to just use early return through this whole section.

Done.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1365
>> +        g_main_context_wakeup(gMainContext);
> 
> Why do you need to wakeup the main context here?

Oops. This line is useless in current implementation. I'll remove this.

>> Source/WebKit/gtk/webkit/webkitwebview.cpp:1369
>> +
> 
> I don't understand this. The first time you create a timeout surce with 0, to be dispatched inmediately (I think you could use a g_idle_source() instead), but in the callback you check whether if elapsed time is 16ms, why not schedule a 16ms timeout here instead? I guess the first time timerFiredCallback is called it will always schedule a new timeout source.

There are some reasons.  
At the first time scheduleAnimation is called, we don't need to wait 16ms to update first scene. 
Using g_timeout_source_new(16) here is valid in the best case. However let's suppose that the content is too heavy or system performance is too low, so that the system can't support 60fps. It means it takes more time than 16ms to update next scene. In that case we have to call timerFiredCallback without extra delay to show better fps.
For instance, if it takes 30ms to be called scheduleAnimation again from first call, then next scene will be updated after 46ms(30 + 16)) from first update. Here we don't need to waste extra 16ms.

> I guess the first time timerFiredCallback is called it will always schedule a new timeout source.
Actually it's not true. it depend on the content. If JS callback function call requestAnimationFrame again, scheduleAnimation is called back again, Or else it's just one-shot.

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