Closed Bug 453883 Opened 13 years ago Closed 13 years ago

remove npapi.h dependency on nspr

Categories

(Core :: Plug-ins, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: jaas, Assigned: jaas)

References

Details

Attachments

(3 files, 8 obsolete files)

We need to remove the npapi.h dependency on nspr. Plugin developers shouldn't have to have nspr in order to compile against our npapi header. This is in line with other npapi implementations.
Attached patch fix v1.0 (obsolete) — Splinter Review
npapi.h:
- remove basic type definitions
- detab
- use C99 types
Attached patch fix v1.1 (obsolete) — Splinter Review
compile fixes for Linux and Windows
Attachment #337523 - Attachment is obsolete: true
Attachment #337529 - Flags: superreview?(jst)
Attached patch fix v1.2 (obsolete) — Splinter Review
fix Windows compile fix
Attachment #337529 - Attachment is obsolete: true
Attachment #337669 - Flags: superreview?(jst)
Attachment #337529 - Flags: superreview?(jst)
Comment on attachment 337669 [details] [diff] [review]
fix v1.2

The only things in npapi.h that might have been provided
by "prtypes.h" are the platform macros such as XP_UNIX,
XP_MACOSX, WIN32, OS2, AIX, etc.  Please make sure these macro
definitions are also provoded by "nptypes.h", or do you
require the plugin vendors to define these platform macros
in their build systems?

In npapi.h, the #pragma pack(1) for __OS2__ was the first
thing in the file.  Now you put it after #include "jri.h".
Please doublecheck this is OK.  If you're not sure, it'd
be prudent to not move the #pragma pack(1) for __OS2__.

In npshell.c, you changed FALSE to false.  Since this is
a C file, I just wanted to make sure 'false' is defined
or supported by the compiler on all platforms.
Attachment #337669 - Flags: superreview?(jst)
Attached patch fix v1.3 (obsolete) — Splinter Review
More Windows bustage fixes, still waiting on try server verification. Includes some changes suggested by Wan-Teh.
Attachment #337669 - Attachment is obsolete: true
Attached patch fix v1.4Splinter Review
more Windows bustage fixes
Attachment #337726 - Attachment is obsolete: true
Attachment #337815 - Flags: superreview?(jst)
Comment on attachment 337815 [details] [diff] [review]
fix v1.4

Looks good, r+sr=jst
Attachment #337815 - Flags: superreview?(jst)
Attachment #337815 - Flags: superreview+
Attachment #337815 - Flags: review+
landed on trunk
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attached patch solaris bustage (obsolete) — Splinter Review
Attachment #338078 - Flags: superreview?(jst)
Attachment #338078 - Flags: review?(joshmoz)
Attachment #338078 - Flags: review?(joshmoz) → review+
Comment on attachment 338078 [details] [diff] [review]
solaris bustage

>+#include <stdbool.h>

This is one of the problems I noted in comment 4 :-)
Duplicate of this bug: 454962
Looking at nptypes.h, there's a define for bool for some cases.
Also not all systems have C99.

So I think we'd better just #define false 0, here.
Attached patch patch for solaris bustage v2 (obsolete) — Splinter Review
Attachment #338078 - Attachment is obsolete: true
Attachment #339036 - Flags: review?(joshmoz)
Attachment #338078 - Flags: superreview?(jst)
Good point, but I think a better solution would be to define
the 'true' and 'false' macros right next to the 'bool'
typedef in nptypes.h.
Comment on attachment 339036 [details] [diff] [review]
patch for solaris bustage v2

lets do what Wan-Teh suggested
Attached patch define true/false in nptypes.h (obsolete) — Splinter Review
Attachment #339036 - Attachment is obsolete: true
Attachment #339183 - Flags: review?(joshmoz)
Attachment #339036 - Flags: review?(joshmoz)
Comment on attachment 339183 [details] [diff] [review]
define true/false in nptypes.h

Shouldn't we only do this for Solaris (defined(__sun))? Why do this for everyone?
I'm not sure about that.

Is Solaris the only platform which doesn't have "false" ?
C99 added the "bool" data type in a header called "stdbool.h". If I include <stdbool.h> on Mac OS X and Linux I can use "bool", "true" and "false" in a little test program. Does solaris have that header? Can we just include the header for most platforms and if Solaris doesn't have it then we can make our own bool definitions for Solaris only?

Mac OS X already picks up stdbool.h in nptypes.h but Solaris doesn't because it doesn't hit the else block where we include it. Maybe all we need to do is change that.
For Solaris, yes. It works.

But I think it's still a potential issue for unixprinting/npshell.c.
Because it uses "false", but nptypes.h doesn't promise "false" is defined.
It seems to me that nptypes.h *should* be promising that false is defined - if that's not the case for Solaris right now then we should fix that by including what is necessary on Solaris. We already ensure it in other ways for other platforms that need it in nptypes.h. I don't think we should be defining true/false ourselves when we can use platform definitions.
Attached patch make it platform specific (obsolete) — Splinter Review
Attachment #339183 - Attachment is obsolete: true
Attachment #339431 - Flags: review?(joshmoz)
Attachment #339183 - Flags: review?(joshmoz)
Comment on attachment 339431 [details] [diff] [review]
make it platform specific

This patch is what I proposed in comment 14.

We should not need to undefine 'true' and 'false' before defining them.
GCC's <stdbool.h> header doesn't do that.
Comment on attachment 339431 [details] [diff] [review]
make it platform specific

Looks good to me if you don't do the undefs. Fix that and r+. Thanks!
Attachment #339431 - Attachment is obsolete: true
Attachment #339761 - Flags: superreview?(jst)
Attachment #339761 - Flags: review?(joshmoz)
Attachment #339431 - Flags: review?(joshmoz)
Attachment #339761 - Flags: review?(joshmoz) → review+
Attachment #339761 - Flags: superreview?(jst) → superreview+
Duplicate of this bug: 366858
Reopening... mozilla-plugin.pc still expresses the dependency on NSPR.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
It would be better to use int_fastXX_t for local variables, wouldn't it?  intXX_t may not as fast as int_fastXX_t.
Attachment #367788 - Flags: superreview?(benjamin)
Attachment #367788 - Flags: superreview?(benjamin) → superreview+
Comment on attachment 367788 [details] [diff] [review]
pkgconfig fix v1.0

woot, glad it was simple!
pushed pkgconfig fix to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/4bb6d29863d0
Status: REOPENED → RESOLVED
Closed: 13 years ago13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.