[Webkit-unassigned] [Bug 23196] Add scheduletimer to PluginView

[bugzilla-daemon at webkit.org]

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


darin at apple.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #26537|review?                     |review-
               Flag|                            |




------- Comment #15 from darin at apple.com  2009-05-23 12:33 PDT -------
(From update of attachment 26537)
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.


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