Bug 40784

Summary: Remove older versions of custom Carbon events in npapi.h
Product: WebKit Reporter: Stuart Morgan <stuartmorgan>
Component: Plug-insAssignee: Stuart Morgan <stuartmorgan>
Severity: Normal CC: abarth, andersca, commit-queue, eric, joshmoz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Description Flags
fix v1
fix, unbitrotted none

Description Stuart Morgan 2010-06-17 09:28:19 PDT
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),

Use of the former should be removed from WebKit, and the #defines removed from the header.
Comment 1 Stuart Morgan 2010-06-17 15:18:11 PDT
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.
Comment 2 Darin Adler 2010-08-29 12:29:01 PDT
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.
Comment 3 Stuart Morgan 2010-08-30 09:03:09 PDT
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.
Comment 4 Stuart Morgan 2010-08-30 10:01:52 PDT
Created attachment 65928 [details]
fix, unbitrotted

Same patch, but with bitrot from the move of TestNetscapePlugIn's main.cpp addressed.
Comment 5 Eric Seidel (no email) 2010-09-07 03:19:29 PDT
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.
Comment 6 Stuart Morgan 2010-09-07 09:34:29 PDT
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.
Comment 7 Stuart Morgan 2010-10-18 11:05:00 PDT
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.
Comment 8 Eric Seidel (no email) 2010-10-18 11:09:14 PDT
Comment on attachment 65928 [details]
fix, unbitrotted

Comment 9 WebKit Commit Bot 2010-10-18 11:51:46 PDT
The commit-queue encountered the following flaky tests while processing attachment 65928 [details]:


Please file bugs against the tests.  The commit-queue is continuing to process your patch.
Comment 10 WebKit Commit Bot 2010-10-18 11:54:24 PDT
Comment on attachment 65928 [details]
fix, unbitrotted

Clearing flags on attachment: 65928

Committed r69979: <http://trac.webkit.org/changeset/69979>
Comment 11 WebKit Commit Bot 2010-10-18 11:54:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 WebKit Review Bot 2010-10-18 12:36:52 PDT
http://trac.webkit.org/changeset/69979 might have broken Leopard Intel Debug (Tests)