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

[jpd at videolan.org]

On Tue, Sep 01, 2009 at 03:52:49PM +0200, Yannick Br?hon wrote:
> It is still in early stages:
> - it only works when compiled with a recent npfunctions.h, npapi.h,
> npruntime.h, nptypes.h, npupp.h (on our Ubuntu system, we used the
> latest API Mozilla furnishes with Firefox, which is located in a
> Seamonkey folder)
> - it only works on a browser supporting this recent plugin API

You'll want to tone that down a bit then, if at all possible.

You'll also want to clean up the patch a bit: I see a lot of
re-indenting that should either go entirely or go in a separate
patch, and new code that matches neither the rest of the file nor the
re-indented code in indentation style.


> - if eventProfile is set to "all":
> this will call the "callback" function with parameter "parameter" (which
> can be for instance a user-specified id) as soon as any event is fired
> by VLC
> - if eventProfile is set to "minimalist":
> this will call the "callback" function with parameter "parameter" (which
> can be for instance a user-specified id) as soon as an event is fired by
> VLC which is an event of the "minimalist" group of events

Why the distinction? Why not then, say, a bitmap so people can pick
and choose what events to receive? What is the performance penalty
of (unnecessairy) calls-to-javascript?

Also, even mozilla code mostly uses char * and doesn't use std::string,
so might as well stick to const char * for static global tables and save
a bit of startup converting.


> +    // associates a list of event with a name.
> +    eventProfiles[GENERIC_EVENTS_PARAM] = g_generic;
> +    eventProfiles[MINIMALIST_EVENTS_PARAM] = g_minimalist;

A map is perhaps a bit of overkill for these two members. I think
we can survive linear search here, if that.


> +  // Search in the event list of the given profile if the current event
> +  // is inside.
> +  for (int k = 0; plugin->eventProfiles[eventProfileName][k].size(); k++)
> +    if (plugin->eventProfiles[eventProfileName][k] == eventName)
> +      eventFound = true;

Now here, a map might possibly have been useful, but instead we
get a tree lookup or two in each linear search iteration.


> +	// Registers the event we're interested in.
> +	eventManager = libvlc_media_player_event_manager(libvlc_media_player, ex);
> +	libvlc_event_attach(eventManager, libvlc_MediaPlayerOpening, eventFct, this, ex);
[and so on]

This is perhaps better done with a list.