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

[Anthony Loiseau]

Hi,

I have not really read your diff, just looked at it.
It inserts some tabs and trailing spaces you may want to remove, just
for you to know.

Regards,
Anthony Loiseau


On Thu, 2009-10-15 at 14:43 +0200, Yannick Bréhon wrote:
> Anyone?
> 
> Yannick Bréhon a écrit :
> > See inline
> > 
> > jpd at videolan.org a écrit :
> >> On Tue, Sep 08, 2009 at 10:15:56AM +0200, Yannick Br?hon wrote:
> >>> Here is our updated version of our patch to the plugin for enabling
> >>> javascript reception of VLC events.
> >> Sadly I really don't have time to properly review it, and won't for
> >> a week or two at least, but I do have a couple of comments.
> >>
> >> First, I noticed you did use a bitmap, which is good, but I had
> >> something slightly different in mind. I have that code around so I'll
> >> drop that in soonish. Same with the list of events, or you could take
> >> a look at the lookup function in core.
> > 
> > We are not quite sure what you want here, can you please expand on this
> > topic? Thanks.
> > 
> >> Second, I'd really rather you didn't traverse string arguments where you
> >> could be parsing an array-of-strings passed from javascript instead.
> > 
> > We have mimicked the mechanism used today to setup a new VLC plugin to
> > get the options. We now support both a string of options or an array of
> > strings (exactly the same as the options for a new plugin). We have
> > deliberately not called the LibvlcPlaylistNPObject::parseOptions method
> > for the following two reasons:
> > - it is apparently bound to evolve quite a bit according to the number
> > of FIXMEs and we are not sure our code will be compatible with such
> > evolutions and/or that our code won't prevent such evolutions
> > - this method is not naturally accessed from within the plugin and all
> > the circonvolutions don't seem appropriate and logical
> > 
> >> Third, there's still a global that I'd rather see go. Give the map to
> >> some object to care for, and since you need to compare against NPUTF8*
> >> anyway you could be saving a couple string conversions here.
> > 
> > We have changed the map of strings for a map of NPUTF8*. We have also
> > hidden said map as well as another global (the eventToCatch bitmask) in
> > the VlcPLugin object.
> > 
> >>> The function returns True if all goes well, or a string with the error
> >>> message otherwise. If any of the event in eventList does not exist, an
> >>> error message is sent back to javascript, and the eventListener is not
> >>> added.
> >> I think this needs rethinking. It is perhaps more conventional to return
> >> true or false and find some other way to return the string or perhaps
> >> not return anything (`return void') but throw an exception with the
> >> message on error.
> > 
> > The function now returns void on success, and throws an exception with
> > the message on error.
> > 
> > 
> > 
> > Moreover, we noticed on *our* development platform that compilation
> > fails because the HAS_NPFUNCTIONS_H variable is not #defined for us. We
> > had to manually define it, otherwise compiler uses npupp.h instead of
> > npfunctions.h, which doesn't define the API functions we need for our patch.
> > Possible solutions are:
> > - use a better development platform than ours ;)
> > - define HAS_NPFUNCTIONS_H manually at compilation time if npfunctions.h
> > exists
> > - define HAS_NPFUNCTIONS_H in appropriate header file (npapi.h or
> > npunix.h, not sure where this should be)
> > 
> > 
> > Awaiting on your comments, thanks for all the help,
> > Yannick
> >