[webkit-reviews] review denied: [Bug 23196] Add scheduletimer to PluginView : [Attachment 26537] patch to add ScheduleTimer to PluginView and npapi.cpp

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sat May 23 12:33:26 PDT 2009

Darin Adler <darin at apple.com> has denied Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 23196: Add scheduletimer to PluginView

Attachment 26537: patch to add ScheduleTimer to PluginView and npapi.cpp

------- Additional Comments from Darin Adler <darin at apple.com>
WebKit style has no indentation of code inside namespaces in .cpp files.

> +    static uint32 gTimerID;

Since this global is only used inside the constructor it can be defined inside
the constructor.

> +
> +    PluginTimer::PluginTimer(PluginTimer** list, NPP instance, bool repeat,
> +				void (*timerFunc)(NPP npp, uint32 timerID))
> +		   : m_list(list),
> +		     m_instance(instance),
> +		     m_timerFunc(timerFunc),
> +		     m_repeat(repeat)

WebKit style has a certain formatting for initializer lists that this does not

> +	   m_next = *list;
> +	   if (m_next) {
> +	       m_next->m_prev = this;
> +	   }
> +	   m_prev = 0;

Can m_next and m_prev be set in the initializer list instead of the body?

WebKit style has no braces around single-line if statements and while

> +	   PluginTimer* next() const { return m_next; }

I'm not sure it's useful to have a function like this if it's private, since
member functions already have access to the m_next field.

> +	   PluginTimer**   m_list;
> +	   PluginTimer*    m_prev;
> +	   PluginTimer*    m_next;
> +	   NPP		   m_instance;
> +	   void 	   (*m_timerFunc)(NPP, uint32);
> +	   uint32	   m_timerID;
> +	   bool 	   m_repeat;

WebKit formatting does not line up things like this.

> +    class PluginTimerList {

Should inherit from Noncopyable. As should PluginTimer itself.

> +	   uint32 schedule(NPP instance, uint32 interval, bool repeat,
> +			   void (*proc)(NPP npp, uint32 timerID));

The argument names "instance" and "proc" aren't helpful here and should be

I think the PluginTimer class can be private and left out this header. I
believe it's an implementation detail, and it's only the PluginTimerList class
that needs to be public. Ideally the source file would be named after that

> +	   uint32 scheduleTimer(NPP, uint32 interval, bool repeat,
> +				void (*timerFunc)(NPP, uint32 timerID));
> +	   void unscheduleTimer(NPP, uint32 timerID);

I think this code would be easier to read if we had a typedef for the timer
function. I don't think the name "timerFunc" is needed here and I suggest it be

It's great to have this new feature. We also should add test cases for it.

There are enough comments here in total that I'll say review-. Omission of a
test case is probably the single biggest issue. Also would be nice to do this
for the Mac platform too. Should be easy to hook it up there.

More information about the webkit-reviews mailing list