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

[Yannick Bréhon]

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: vlc-plugin-3.patch
Type: text/x-patch
Size: 17421 bytes
Desc: not available
URL: <http://mailman.videolan.org/pipermail/vlc-devel/attachments/20091007/dd3cdb88/attachment.bin>