[vlc-devel] Support of events in Mozilla plugin - Beta

[jpd at videolan.org]

On Wed, Nov 11, 2009 at 11:44:43AM +0100, y.brehon at qiplay.com wrote:
> +
> +const NPUTF8 * const LibvlcEventNPObject::methodNames[] =
> +{
> +    "addListener",
> +    "removeListeners"
> +};
> +COUNTNAMES(LibvlcEventNPObject,methodCount,methodNames);

There probably should be a way to enumerate the possible events, too.
A list of strings or an iterator or something. (suggestions welcome.)


> +                    // Gets the binary field corresponding to the events the
> +                    // listener must listen to if specified.
> +                    // Else, listen to all events.
> +                    eventtypes_bitmap_t *eventToGet = new eventtypes_bitmap_t();

This should not be a pointer; the object isn't large enough and if you
can copy from a pointed-to object you can copy from an on-stack object.


> +                    eventToGet->set_all_events();

I think this is not a good idea for a default. Better to start with an
empty list and support "all" as a keyword (check for that right before
asking the bitmap to parse a string). Add "none" that causes a clear();


> +
> +                    if (!parseArgs(args, argCount, *eventToGet))
> +                    {
> +                        NPN_SetException(this, strdup(ERROR_EVENT_NOT_FOUND));
> +                        return INVOKERESULT_GENERIC_ERROR;
> +                    }

Leaking eventToGet here. But see note below.


> +                    EventListener *eventListener = new EventListener();
> +                    eventListener->listener = listener;
> +                    eventListener->id = saveNPVariant(args[1]);
> +                    eventListener->eventMap = eventToGet;

Make the eventMap an object instead of a pointer-to-object again.


> +                    p_plugin->eventToCatch.add_event(*eventToGet);

If you're keeping this then you probably should lazily ask for event
delivery instead of the "send all of this set" approach currently in
use. Otherwise I don't see a need for eventToCatch.


> +                    p_plugin->eventListeners.push_back(eventListener);

Personally I would've stored the object itself again. Erase at the end
of a vector is cheap (not so at the start). Allocate a new one at the
end of the vector at the start of the function, and erase when it turns
out you don't need it after all. Use a reference for referencing the
entry in the function.


> +          case ID_root_removeListeners:
> +              if (argCount == 0)
> +              {
> +                  VlcPlugin* p_plugin = getPrivate<VlcPlugin>();
> +                  p_plugin->eventListeners.clear();
> +                  p_plugin->eventToCatch.clear_event();
> +                  return INVOKERESULT_NO_ERROR;
> +              }

No way to remove a single listener? Defaulting to dropping all listeners
might bring some nasty surprises if there are multiple parties listening.


> +void eventAsync(void *param)
> +{
> +    VlcPlugin *plugin = (VlcPlugin*)param;
> +    NPVariant result;
> +    NPVariant params[2];
> +
> +    // mutex lock
> +    pthread_mutex_lock(&plugin->mutex);
> +
> +    for (int i = 0; i < plugin->eventList.size(); i++)
> +    {
> +        for (int j = 0; j < plugin->eventListeners.size(); j++)
> +        {
> +            const NPUTF8* eventName = plugin->eventList[i];
> +
> +            if (plugin->eventListeners[j]->eventMap->have_event(eventName))

have_event() should be fed an event number, not a string, as noted below.


> +            {
> +                STRINGZ_TO_NPVARIANT(strdup(eventName), params[0]);
> +                params[1] = plugin->eventListeners[j]->id;
> +                NPN_InvokeDefault(plugin->getBrowser(), plugin->eventListeners[j]->listener, params, 2, &result);

Why do you pass both the eventname and the cookie? Wouldn't one be enough?


> +                NPN_ReleaseVariantValue(&result);
> +                NPN_ReleaseVariantValue(&params[0]);

You don't duplicate params[1], so why do you strdup params[0]?


> +void eventFct(const libvlc_event_t* event, void *param)
> +{
> +    VlcPlugin *plugin = (VlcPlugin*)param;
> +
> +    // mutex lock
> +    pthread_mutex_lock(&plugin->mutex);

I can see that pthread_mutex_lock() does a mutex lock, so the comment
is wasted. Either remove or replace with something that is useful and
isn't already clear from the code.


> +
> +    if (plugin->eventToCatch.have_event(event->type))
> +        plugin->eventList.push_back(libvlc_event_type_name(event->type));

This makes no sense. Why aren't you storing the event as event_type_t
instead of going out of your way to store a string that causes an
otherwise completely superfluous linear search? If you need the
string later, ask for it later.


> +    // mutex unlock
> +    pthread_mutex_unlock(&plugin->mutex);

Same as above.


>  bool VlcPlugin::playlist_select( int idx, libvlc_exception_t *ex )
>  {
>      libvlc_media_t *p_m = NULL;
> +    libvlc_event_manager_t *eventManager = NULL;
[...]
>      if( libvlc_media_player )
> +      {
>          set_player_window(ex);
>  
> +        // Registers the events we're interested in.
> +        eventManager = libvlc_media_player_event_manager(libvlc_media_player, ex);

Why is this and the large list of libvlc_event_attach() calls here and
not in the constructor? Altough probably best elided entirely in favour
of a "lazy" approach, as detailed earlier.


> +bool VlcPlugin::canUseEventListener()
> +{
> +    int plugin_major, plugin_minor;
> +    int browser_major, browser_minor;
> +
> +    NPN_Version(&plugin_major, &plugin_minor,
> +                &browser_major, &browser_minor);
> +
> +    if (browser_minor >= 0.19 || browser_major > 0)
> +        return true;

Why are you comparing an int to a float here?


> +class eventtypes_bitmap_t: private bitmap<libvlc_num_event_types> {
> +private:

       typedef bitmap<libvlc_num_event_types> parent;

Add the above line here for the clear_event comment below.


> +    bool add_event(const char *s)
> +    {
> +        event_t event = find_event(s);
> +        bool b = event<maxbit();
> +        if(event)
> +            set(event);

This should read if(b) set(event); of course.


> +    bool have_event(const char *s) const
> +    {
> +        event_t event = find_event(s);
> +        return event<maxbit()?get(event):false;
> +    }

Better written as (excercise: check that this is correct):

       bool have_event(const char *s) const
       {
            return have_event(find_event(s));
       }

but I think it shouldn't be needed at all.


> +    void clear_event()
> +    {
> +        clear();
> +    }

This is a misleading name because it doesn't clear one event, it clears
them all. Better to export the original clear() thus:

      void clear() { parent::clear(); }


> +typedef struct s_EventListener
> +{
> +    NPObject *listener;
> +    NPVariant id;
> +    eventtypes_bitmap_t *eventMap;
> +
> +} EventListener;

As noted, eventMap shouldn't be a pointer.


> +// Comparator for the global map of event. With it, we can use
> +// char* as key.
> +struct StringCmp
> +{
> +    bool operator() (const char* s1, const char* s2) const
> +    {
> +        return strcmp(s1, s2) < 0;
> +    }
> +};

Looks like dead code, just as the #include <map> and #include <string> are.