Bug 23196 – Add scheduletimer to PluginView

• Component: Plug-ins
• Status: RESOLVED WONTFIX

[Mike Reed]

PluginView, and npapi.cpp do not implement NPN_ScheduleTimer and NPN_UnscheduleTimer, even though it is declared in npapi.h

[Mike Reed]

Created attachment 26537 [details]
patch to add ScheduleTimer to PluginView and npapi.cpp

The android platform wants to allow plugins to call NPN_ScheduleTimer. This patch (re)adds the plumbing to PluginView and npapi.cpp (I think it used to be there).

[Darin Adler]

Anders, comments?

I didn't try to review the patch for coding style, because first I want to hear from Anders why we don't already have this.

[Mike Reed]

Hi Anders,

PluginTimer.[cp,h] was at one point in webcore/plugins, but then it was removed. This patch is basically to restore it, and to hook it up to npapi.cpp.

If you have any questions about the code, please ping me. I have one more android-specific change to submit for plugins, but I want to wait for this patch first.

thanks,
mike

[Anders Carlsson]

(In reply to comment #3)
> Hi Anders,
> 
> PluginTimer.[cp,h] was at one point in webcore/plugins, but then it was
> removed. This patch is basically to restore it, and to hook it up to npapi.cpp.
> 
> If you have any questions about the code, please ping me. I have one more
> android-specific change to submit for plugins, but I want to wait for this
> patch first.
> 
> thanks,
> mike
> 

Hi Mike,

so the reason I added NPN_ScheduleTimer/NPN_UnscheduleTimer was as a replacement for null events on Mac. The idea behind these timers is that the browser is free to pause/throttle them as necessary. (That's why they're in WebKit - they were Mac specific)

Do you need the API on Android for the same reasons? If so, I think we should propose on plugin-futures that they be made generic and not mac-specific at all. 

[Mike Reed]

NPN_ScheduleTimer appears in the old mozilla docs, so I had always
treated it as part of the pseudo standard for plugins. I had not
thought they were mac-specific.

Android offers them to plugins as real timers, since our plugins have
no "windowed" mode, and thus the NPN functions are pretty much all we
give them (modulo we extend npapi for our drawing model).

As submitted (I believe), the change should not actually affect other
port, since hooking these up is left to the browserFuncs
initialization, which is platform-specific.

[Anders Carlsson]

(In reply to comment #5)
> NPN_ScheduleTimer appears in the old mozilla docs, so I had always
> treated it as part of the pseudo standard for plugins. I had not
> thought they were mac-specific.
> 

What old mozilla docs? :)

[Mike Reed]

https://wiki.mozilla.org/Mac:NPAPI_Event_Models

but duh, now I see that it is in the mac-specific section. My bad.

What do you think of the patch?

[Anders Carlsson]

(In reply to comment #7)
> https://wiki.mozilla.org/Mac:NPAPI_Event_Models
> 
> but duh, now I see that it is in the mac-specific section. My bad.
> 

Yeah, I added it there and got confused when you said it already existed :)

> What do you think of the patch?
> 

I think you can just move the code from WebNetscapePluginView as is to begin with. Does that sound OK?

[Mike Reed]

The only WebNetscapePluginView I see is WebNetscapePluginView.mm, which is in objective-c. My patch is your old C++ code. What are you proposing?

[Eric Seidel (no email)]

Discussion seems to have stalled here.  Anders?

[Eric Seidel (no email)]

Comment on attachment 26537 [details]
patch to add ScheduleTimer to PluginView and npapi.cpp

Discussion has stalled.   Marking r- to remove this from the review queue.  Please email anders directly to take up your questions with him.

[Maciej Stachowiak]

Comment on attachment 26537 [details]
patch to add ScheduleTimer to PluginView and npapi.cpp

Reflagging this for review. There isn't any unaddressed feedback here, it just needs a reviewer to look at the comments.

[Mark Rowe (bdash)]

(In reply to comment #9)
> The only WebNetscapePluginView I see is WebNetscapePluginView.mm, which is in
> objective-c. My patch is your old C++ code. What are you proposing?

WebNetscapePluginView is Objective-C++, and the portions related to ScheduleTimer are primarily in C++.  See the PluginTimer class at <http://trac.webkit.org/browser/trunk/WebKit/mac/Plugins/WebNetscapePluginView.mm?rev=44048#L113> and the other references to PluginTimer within that file.  I think Anders may have been suggesting that the WebCore implementation be based on this existing version.  It may even be possible to move the PluginTimer class from WebKit/mac in to WebCore and have WebNetscapePluginView use the WebCore implementation.

[Darin Adler]

I agree that there should be more code shared cross-platform in plug-in implementation. The Objective-C++ source files used to support plug-ins on Mac can easily call shared C++ functions that are also used on the non-Mac platform. Not sure its relevant to this patch, though.

[Darin Adler]

Comment on attachment 26537 [details]
patch to add ScheduleTimer to PluginView and npapi.cpp

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.

[Mike Reed]

I will update the patch with the coding style / typedef changes mentioned in the last comment.

[Anders Carlsson]

We're unlikely to make this plug-in change - plug-ins are pretty much in maintenance only mode.