[OS/2] Fix plugin code to allow building with GCC 4.3.x

RESOLVED FIXED

Status

()

Core
Plug-ins
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: Peter Weilbacher, Assigned: Rich Walsh)

Tracking

({fixed1.9.1})

Trunk
x86
OS/2
fixed1.9.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.28 KB, patch
beltzner
: approval1.9.1+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
Follow-up from bug 476029 comment 2.
I guess this will also eliminate the last hurdles for bug 454956.
(Assignee)

Comment 1

9 years ago
Created attachment 359915 [details] [diff] [review]
patch to npfunctions.h

This patches modules/plugin/base/public/npfunctions.h to eliminate calling convention errors in modules/plugin/default/os2/npos2.cpp.

The NPP_* functions in nsapi.h are all declared to be NP_LOADDS, which on OS/2 resolves to '_System'.  However, the typedefs for these functions lack this declaration, producing pointer conversion errors when compiling.
(Reporter)

Updated

9 years ago
Attachment #359915 - Attachment is patch: true
Attachment #359915 - Attachment mime type: application/octet-stream → text/plain
(Reporter)

Updated

9 years ago
Attachment #359915 - Flags: superreview?(jst)
Attachment #359915 - Flags: review+
(Reporter)

Comment 2

9 years ago
Comment on attachment 359915 [details] [diff] [review]
patch to npfunctions.h

Makes sense to me, but those plugin files should get sr, too, even though it doesn't change anything for the other platforms.

Comment 3

9 years ago
Comment on attachment 359915 [details] [diff] [review]
patch to npfunctions.h

Please don't check this in until I have looked at it.
Attachment #359915 - Flags: review?(joshmoz)

Comment 4

9 years ago
Is there really no way to change the calling convention on the other end so we don't have to do this to npfunctions.h? What is nsapi.h?
(Reporter)

Comment 5

9 years ago
Sorry, Josh. Didn't want to cut you out.

"nsapi.h" in comment 1 obviously was a typo and was supposed to read "npapi.h".

We have to use _System calling convention which is the standard way to use all external functions on OS/2 and has to be maintained especially for the plugins for backward compatibility. As that convention is used in npapi.h we have to adapt npfunction.h to follow because GCC 4.3 is too picky to let it go otherwise. With that compiler we now get weird errors like

<path>/modules/plugin/default/os2/npos2.cpp:
   In function 'NPError NP_GetEntryPoints(NPPluginFuncs*)':
<path>/modules/plugin/default/os2/npos2.cpp:83:
   error: invalid conversion from 'NPError (*)(char*, NPP_t*, uint16_t,
      int16_t, char**, char**, NPSavedData*)' to 'NPError(*)(char*,
      NPP_t*, uint16_t, int16_t, char**, char**, NPSavedData*)'
<path>/modules/plugin/default/os2/npos2.cpp:84:
   error: invalid conversion from 'NPError (*)(NPP_t*, NPSavedData**)'
      to 'NPError (*)(NPP_t*, NPSavedData**)'
<path>/modules/plugin/default/os2/npos2.cpp:85:
   error: invalid conversion from 'NPError (*)(NPP_t*, NPWindow*)'
      to 'NPError (*)(NPP_t*, NPWindow*)'
<path>/modules/plugin/default/os2/npos2.cpp:86:
   error: invalid conversion from 'NPError (*)(NPP_t*, char*, NPStream*,
      NPBool, uint16_t*)' to 'NPError (*)(NPP_t*, char*, NPStream*,
      NPBool, uint16_t*)'

(Sorry should have been in comment 0.) GCC 3.3.5, that we were using before, let this pass and apparently did the right thing.

Why do you see changing npfunctions.h as a problem? It's just an internal file, right?

Comment 6

9 years ago
Changing npfunctions.h in that way is not technically a problem, but I like to keep it as uncluttered as possible.

npfunctions.h is not simply internal - that, plus "npapi.h", "nptypes.h", and "npruntime.h" are the standard NPAPI headers. All plugins should be able to compile against them and run in all browsers that support NPAPI. The files should match between browser implementations but they don't, we're working on it.

Comment 7

9 years ago
And thanks for the explanation, I'll review again soon.

Updated

9 years ago
Attachment #359915 - Flags: review?(joshmoz) → review+
(Reporter)

Comment 8

9 years ago
Thanks, Josh. I had forgotten that you had renamed npupp.h to npfunctions.h and got confused because I didn't find the new name mentioned anywhere on MDC.

Updated

9 years ago
Attachment #359915 - Flags: superreview?(jst) → superreview+

Comment 9

9 years ago
Created attachment 360656 [details] [diff] [review]
fix v1.0

Same thing, updated to current trunk.
Attachment #359915 - Attachment is obsolete: true

Updated

9 years ago
Whiteboard: [checkin-needed]
(Reporter)

Comment 10

9 years ago
Thanks Rich and Josh! Pushed to trunk:
http://hg.mozilla.org/mozilla-central/rev/4fad8c29c9be

I guess we will need this for FF 3.1, too, to eventually be able to produce a patch-free OS/2 build. But let's wait a while before asking for approval.
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
(Reporter)

Comment 11

9 years ago
Comment on attachment 360656 [details] [diff] [review]
fix v1.0

We need this to build the 1.9.1 branch with a current compiler. This hasn't caused any problems and is essentially an OS/2 only change.
Attachment #360656 - Flags: approval1.9.1?
Attachment #360656 - Flags: approval1.9.1? → approval1.9.1+
(Reporter)

Comment 13

9 years ago
Fixed in mozilla-1.9.1:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/a897e9525f7e
Keywords: fixed1.9.1
You need to log in before you can comment on or make changes to this bug.