[webkit-reviews] review denied: [Bug 17679] Shared PluginView implementation : [Attachment 19570] Updated patch with a few small changes

Thu Mar 6 23:37:00 PST 2008

Jon Honeycutt <jhoneycutt at apple.com> has denied Rodney Dawes
<dobey at wayofthemonkey.com>'s request for review:
Bug 17679: Shared PluginView implementation
http://bugs.webkit.org/show_bug.cgi?id=17679

Attachment 19570: Updated patch with a few small changes
http://bugs.webkit.org/attachment.cgi?id=19570&action=edit

------- Additional Comments from Jon Honeycutt <jhoneycutt at apple.com>
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.