Editing plugin header files is a recipe for disaster

RESOLVED FIXED in Future

Status

()

Core
Plug-ins
P4
normal
RESOLVED FIXED
16 years ago
6 years ago

People

(Reporter: Brian Nesse (gone), Assigned: Josh Aas)

Tracking

Trunk
Future
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [PL2:P4])

(Reporter)

Description

16 years ago
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

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

Comment 2

16 years ago
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.
(Reporter)

Comment 3

16 years ago
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

16 years ago
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.
(Reporter)

Comment 5

16 years ago
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

15 years ago
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
Depends on: 88998
Priority: -- → P3
Whiteboard: [PL2:P4]
Target Milestone: --- → mozilla1.2alpha

Updated

15 years ago
Priority: P3 → P4

Updated

15 years ago
Target Milestone: mozilla1.2alpha → mozilla1.2beta

Updated

15 years ago
Target Milestone: mozilla1.2beta → mozilla1.3alpha

Comment 7

15 years ago
moving to future
Target Milestone: mozilla1.3alpha → Future
QA Contact: shrir → plugins
(Assignee)

Comment 8

6 years ago
I fixed this a while ago. We don't have duplicate npapi.h files or nsplugindefs.h any more.
Assignee: serhunt → joshmoz
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.