[Webkit-unassigned] [Bug 74769] [chromium] Add CCTimer class for the compositor

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Tien-Ren Chen <trchen at chromium.org>  2011-12-21 15:45:10 PST ---
(In reply to comment #7)
> (From update of attachment 119963 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119963&action=review
> 
> > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:35
> > +#include <cfloat>
> 
> what is cfloat for? does it need to be included in this header?

Removed. It is leftover from the shrink.

> > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:48
> > +    CCTimer(CCThread* thread, CCTimerClient* client) : m_client(client), m_thread(thread), m_task(0) { }
> 
> please expand the initializer list to one statement per line
> to keep this header smaller and more readable could you move the implementations of the c'tor/d'tor into the .cpp? I don't expect this to be performance-critical code

Done

> > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:51
> > +    void startOneShot(double interval);
> 
> is this seconds, milliseconds, microseconds, something else? we try to tag parameter names that represent periods of time with a suffix to indicate the unit
> CCThread and CCDelayBasedTimeSource both have time values of milliseconds with a "Ms" suffix, can we stick with that?

Done

However I totally agree with Nat that the use of millisecond is a source of confusion. Why not just use seconds?

> can you document in the header what this function does if there's already a timer pending (looks like it cancels the previous task, which i think is great behavior but it should be documented)

Done

> > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:52
> > +    void stop() { deschedule(); }
> 
> why are stop() and deschedule() different functions if they are just aliases of each other? Can you fold them into one function?

Done

But I'm a little bit reluctant to this. I designed them into 2 function because they have different semantics. deschedule() means to break the link between the task and the timer, where stop() is a public function that request the timer to cancel. deschedule() is a mean to cancel, but cancel does not necessarily need to be implemented this way.

Anyway, I agree with you that keep things simple is good, so I'm fine with either way.

> > Source/WebCore/platform/graphics/chromium/cc/CCTimer.h:59
> > +        Task(CCTimer* timer) : CCThread::Task(0), m_timer(timer) { }
> 
> explicit on this constructor, please
> 
> can you expand the member initializations out to separate lines? we very rarely combine multiple statements to a single line in webkit

Done

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