118828 - Editing plugin header files is a recipe for disaster

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect, P4)

Description

[Brian Nesse (gone)]

While working on adding support for scrolling notification for plug-ins I have
come across a few things in the plugin header files that concern me greatly.

First of all, there are 2 copies of the file npapi.h in the tree.
/embedding/browser/activex/src/pluginhostctrl/pluginsdk_include/npapi.h
and
/include/npapi.h

Second, in the directory /modules/plugin/base/public there are 2 files,
nsPluginDefs.idl and nsplugindefs.h, which appear to be used somewhat
interchangeably.
http://lxr.mozilla.org/seamonkey/search?string=nsPluginDefs.idl
http://lxr.mozilla.org/seamonkey/search?string=nsplugindefs.h
The problem with this is that while the files are essentially similar, there are
a number of definitions which do not exist in both files.

The combination of these 4 files (assuming there are not more like them) all but
guarantees that mistakes will be made when working in the plug-in code.

We need to find a way to eliminate all of these multiple files and definitions,
perhaps via header restructuring or the creative use of #includes.

Comment 1

[Peter Lubczynski]

cc:ing Adam Lock for npapi.h used by embedding

Comment 2

[Adam Lock]

I originally checked in npapi.h there so that people could build my plugin with
out downloading the entire plugin sdk. It is used nowhere else and the plugin is
not part of the Mozilla build.

I will review if it is still necessary.

Comment 3

[Brian Nesse (gone)]

As av pointed out via email, the copy of npapi.h in the embedding directory is
also out of date. Even if it is still necessary we need to find a way to get
down to one copy of npapi.h.

Comment 4

[Adam Lock]

The npapi.h may be out of date for Mozilla, but the plugin host control is not
intended to run in Mozilla! It is an ActiveX control designed to allow plugins
to run in IE and other ActiveX control containers. For that purpose, the plugin
host control implements the NPN API as found in 4.x and uses the header files to
define the various structures and headers it needs to do this.

I would be happy to rename them to avoid confusion, but I would be wary of using
the new headers in Mozilla unless I could be assured that they will remain
backwards compatible with the 4.x plugin SDK headers.

Comment 5

[Brian Nesse (gone)]

As far as I know, the headers *must* retain backwards compatibility to 4.x.
Something you may not be aware of is that the version of npapi.h you are using
is not from the 4.x SDK, it is an early version of the one in the Mozilla tree.
There have been some bug fixes along the way.

For good or bad, an LXR search tends to imply the need to change both files if a
change is made. If these files don't need to be kept in lock step
synchronization we should try to keep it from appearing that they do. One way to
do this would be to actually use the 4.x SDK file instead... another would be to
do as you suggest and rename the file. Either way, I believe the current
scenario is bad.

Comment 6

[rubydoo123]

The plug-ins triage team (av, beppe, peterl, serge and shrir) have reviewed this
issue and have made the following determination:

This is dependent upon the work being done in the general clean-up work, adding
dependency

Comment 7

[rubydoo123]

moving to future

Comment 8

[Josh Aas]

I fixed this a while ago. We don't have duplicate npapi.h files or nsplugindefs.h any more.