Bug 17679

Summary:

Shared PluginView implementation

Product:

WebKit

Reporter:

Rodney Dawes <dobey>

Component:

Plug-ins

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

jhoneycutt

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

All

Attachments:

Description	Flags
Shared PluginView patch	none
Updated patch with a few small changes	jhoneycutt: review-
Updated patch to refactor PluginView	jhoneycutt: review+
Updated patch to fix minor typos	none

Rodney Dawes

Reported 2008-03-05 08:21:14 PST

Here's my patch to refactor PluginView to be shared. This patch doesn't change the Wx port currently as it needs changes in the Widget implementation to be able to use PluginView. And looking at Widget.h, it looks like there could be a good bit of refactoring done there as well. Also, once Widget gets refactored, we could add some cross-platform methods there to replace bits that are currently in #if PLATFORM() blocks inside PluginView.cpp in this patch, such as calling ::UpdateWindow() on Windows.

Attachments
Shared PluginView patch (126.54 KB, patch) 2008-03-05 08:24 PST, Rodney Dawes	no flags	Details Formatted Diff Diff
Updated patch with a few small changes (126.54 KB, patch) 2008-03-06 08:38 PST, Rodney Dawes	jhoneycutt: review-	Details Formatted Diff Diff
Updated patch to refactor PluginView (112.48 KB, patch) 2008-03-17 17:21 PDT, Rodney Dawes	jhoneycutt: review+	Details Formatted Diff Diff
Updated patch to fix minor typos (112.53 KB, patch) 2008-03-18 06:58 PDT, Rodney Dawes	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Rodney Dawes

Comment 1 2008-03-05 08:24:52 PST

Created attachment 19546 [details] Shared PluginView patch

Rodney Dawes

Comment 2 2008-03-06 08:38:05 PST

Created attachment 19570 [details] Updated patch with a few small changes This is an updated patch with a few small changes. The wxWidget class apparently doesn't exist in Wx, so I've changed that typedef to wxWindow* instead. I've also changed the new typedef to define PlatformWidget rather than PlatformWindow, as I believe it is more accurate, and fitting, for Widget.h.

Jon Honeycutt

Comment 3 2008-03-06 23:36:59 PST

Comment on attachment 19570 [details] Updated patch with a few small changes Please avoid these few uses of platform-specific code in cross-platform code, even if they're meant to be temporary. I think you should use platform-defined helper functions like platformForceRedraw and platformInvalidateRect. r- for build breakage and platform-dependent code in cross-platform code; more comments below. ><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">Index: WebCore/ChangeLog >=================================================================== >+ Move PluginmessageThrottler to a shared class. Typo 'm.' >Index: WebCore/plugins/PluginView.cpp >=================================================================== > >+#if PLATFORM(WIN) > // Unsubclass the window > if (m_isWindowed) { >- WNDPROC currentWndProc = (WNDPROC)GetWindowLongPtr(m_window, GWLP_WNDPROC); >- >- if (currentWndProc == PluginViewWndProc) >- SetWindowLongPtr(m_window, GWLP_WNDPROC, (LONG)m_pluginWndProc); >+ unsetWndProc(); > } Can remove the braces now that this is one line. >- KJS::JSLock::DropAllLocks; >+ KJS::JSLock::DropAllLocks dropAllLocks; Good catch! You should mention this fix in the ChangeLog. >+#if PLATFORM(WIN) > // Get file info > WIN32_FILE_ATTRIBUTE_DATA attrs; > if (GetFileAttributesExW(filename.charactersWithNullTermination(), GetFileExInfoStandard, &attrs) == 0) >@@ -1188,6 +782,7 @@ NPError PluginView::handlePost(const cha > > if (retval == 0 || bytesRead != attrs.nFileSizeLow) > return NPERR_FILE_NOT_FOUND; >+#endif Can use SharedBuffer::createWithContentsOfFile here. >Index: WebCore/plugins/PluginView.h >=================================================================== >+ class PluginMessageThrottler { I'm not sure of the utility of making this cross-platform. On Windows, this is used exclusively for the Flash plug-in, which sends Windows WM_USER + 1 messages to its own window at a high enough rate to cause the app to stutter. We subclass its window, intercept these messages, and dispatch them when WebCore timers run. Do you know if this will be needed by plug-ins on other platforms, and if their "message" flood can both be handled asynchronously and be stored as an NPEvent? If not, you should leave it Windows-only for now, and make it cross-platform as needed. If so, you should at least change the name from PluginMessage to PluginEvent. >+ bool isEventUserGesture(NPEvent&); This could be a static method taking a const ref. > >+#if PLATFORM(WIN) >+ void setWndProc(); >+ void unsetWndProc(); >+ static LRESULT CALLBACK PluginViewWndProc(HWND, UINT, WPARAM, LPARAM); >+ Please remove these and any other tab characters. Windows build is currently broken here; you need to prefix the definition of PluginViewWndProc with the class name in PluginViewWin.cpp or move this back out of PluginView. If you want it to remain in PluginView, you should rename it from PluginView::PluginViewWndProc to remove the redundancy. >+ NPEvent npEvent = { message, wParam, lParam }; On Windows, NPEvent.event is a unsigned short. Windows messages are unsigned int.

Rodney Dawes

Comment 4 2008-03-17 17:21:12 PDT

Created attachment 19856 [details] Updated patch to refactor PluginView Here is an updated smaller patch which implements a shared PluginView class. This version avoids using #if PLATFORM(WIN) in the cross-platform code, and just migrates the bare minimum for now, until other refactoring can be done with Widget as well. This will allow platforms to implement working plug-in support for their ports. It would be great if we could get this in asap, as I will be submitting refactored portions of my GTK+ plug-in support patch based on this refactoring, tomorrow. :)

Jon Honeycutt

Comment 5 2008-03-17 23:44:07 PDT

Comment on attachment 19856 [details] Updated patch to refactor PluginView This looks good to me, a couple of minor things: 16 * WebCoreSOurces.bkl: Capital 'O' 69 void PluginView::invalidateRegion(NPRegion) {notImplemented(); } Missing a space. Should combine the private members in PluginView.h that are wrapped in #if PLATFORM(WIN). Looks like you're missing PluginMessageThrottlerWin.{h,cpp}. r=me if you add the missing files, and thanks!

Rodney Dawes

Comment 6 2008-03-18 06:58:26 PDT

Created attachment 19863 [details] Updated patch to fix minor typos

Rodney Dawes

Comment 7 2008-03-18 06:59:54 PDT

(In reply to comment #5) > Looks like you're missing PluginMessageThrottlerWin.{h,cpp}. > > r=me if you add the missing files, and thanks! I think you don't have trunk. These two files were added in a previous patch, reviewed and landed by aroben. :) Updated this patch to resolve the other issues you mentioned though and specify you as the reviewer. Thanks!

Matt Lilek

Comment 8 2008-03-18 07:23:37 PDT

Committed revision 31123 per Jon's r+ in comment #5 with the files already being in SVN.

Matt Lilek

Comment 9 2008-03-18 07:24:01 PDT

Comment on attachment 19863 [details] Updated patch to fix minor typos Clearing review flag.

Note You need to log in before you can comment on or make changes to this bug.