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>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, andersca, commit-queue, eric, joshmoz, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
fix v1
none
fix, unbitrotted none

Stuart Morgan
Reported 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), NPEventType_LoseFocusEvent, NPEventType_AdjustCursorEvent, ... Use of the former should be removed from WebKit, and the #defines removed from the header.
Attachments
fix v1 (6.89 KB, patch)
2010-06-17 15:18 PDT, Stuart Morgan 		no flags
DetailsFormatted DiffDiff
fix, unbitrotted (7.02 KB, patch)
2010-08-30 10:01 PDT, Stuart Morgan 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Stuart Morgan
Comment 1 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.
Darin Adler
Comment 2 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.
Stuart Morgan
Comment 3 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.
Stuart Morgan
Comment 4 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.
Eric Seidel (no email)
Comment 5 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.
Stuart Morgan
Comment 6 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.
Stuart Morgan
Comment 7 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.
Eric Seidel (no email)
Comment 8 2010-10-18 11:09:14 PDT
Comment on attachment 65928 [details] fix, unbitrotted OK.
WebKit Commit Bot
Comment 9 2010-10-18 11:51:46 PDT
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 10 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>
WebKit Commit Bot
Comment 11 2010-10-18 11:54:29 PDT
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 12 2010-10-18 12:36:52 PDT
http://trac.webkit.org/changeset/69979 might have broken Leopard Intel Debug (Tests)
Note You need to log in before you can comment on or make changes to this bug.