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

Darin Adler <darin at apple.com> has denied Maciej Stachowiak <mjs at apple.com>'s
request for review:
Bug 23196: Add scheduletimer to PluginView
https://bugs.webkit.org/show_bug.cgi?id=23196

Attachment 26537: patch to add ScheduleTimer to PluginView and npapi.cpp
https://bugs.webkit.org/attachment.cgi?id=26537&action=review

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

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

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

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

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

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.