Bug 40784 – Remove older versions of custom Carbon events in npapi.h

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Stuart Morgan]

WebKit's npapi.h defines three events for Carbon plugin event structures:
#define getFocusEvent        (osEvt + 16)
#define loseFocusEvent        (osEvt + 17)
#define adjustCursorEvent   (osEvt + 18)

But the upstream copy uses an enum for those values:
enum NPEventType {
  NPEventType_GetFocusEvent = (osEvt + 16),
  NPEventType_LoseFocusEvent,
  NPEventType_AdjustCursorEvent,
  ...

Use of the former should be removed from WebKit, and the #defines removed from the header.

[Stuart Morgan]

Created attachment 59040 [details]
fix v1

Note that the test plugin is still using the old names; the value of having them match seemed minimal, and this means test expectations don't have to all change.

[Darin Adler]

Comment on attachment 59040 [details]
fix v1

Seems great to fix everything in our own code. But why are we removing these macros from a public header? Are you sure this is OK? Did Netscape already do this in their copy of npapi.h?

Setting review+ assuming the answer to all those questions is "a good reason", yes, and yes.

[Stuart Morgan]

The answers are:
- To bring WebKit's copy of the header more in line with http://code.google.com/p/npapi-headers/ in order to minimize confusion both for people writing plugins ("which header should I build my plugin against?") and for browser developers (when updating the headers for NPAPI changes).
- People building plugins against the WebKit version of the header will have to make minor code changes when building against the new header, but that will be an easy change; I think the benefits outweigh the cost. (And hopefully plugin vendors will start using the central repository instead of an individual browser's copy anyway.)
- Mozilla already did this.

[Stuart Morgan]

Created attachment 65928 [details]
fix, unbitrotted

Same patch, but with bitrot from the move of TestNetscapePlugIn's main.cpp addressed.

[Eric Seidel (no email)]

Comment on attachment 59040 [details]
fix v1

Cleared Darin Adler's review+ from obsolete attachment 59040 [details] so that this bug does not appear in http://webkit.org/pending-commit.

[Stuart Morgan]

Could you also set commit-queue+ then, since it is in fact a pending commit? It would be nice not to have to un-bit-rot this a second time.

[Stuart Morgan]

Sigh; these defines have now spread to WebKitTools/DumpRenderTree/chromium/TestNetscapePlugIn/ForwardingHeaders/WebKit/npfunctions.h

Can someone explain why this has been commit-queue? for almost two months? If there's something wrong with the patch I'd be happy to fix it.

[Eric Seidel (no email)]

Comment on attachment 65928 [details]
fix, unbitrotted

OK.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 65928 [details]:

fast/workers/storage/use-same-database-in-page-and-workers.html

Please file bugs against the tests.  The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 65928 [details]
fix, unbitrotted

Clearing flags on attachment: 65928

Committed r69979: <http://trac.webkit.org/changeset/69979>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/69979 might have broken Leopard Intel Debug (Tests)