Bug 16779 - Make the PluginStream implementation be shared across platforms
Summary: Make the PluginStream implementation be shared across platforms
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Plug-ins (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-01-07 17:35 PST by Rodney Dawes
Modified: 2008-01-11 03:34 PST (History)
1 user (show)

See Also:


Attachments
Patch to implement shared PluginStream (108.75 KB, patch)
2008-01-07 17:36 PST, Rodney Dawes
mrowe: review-
Details | Formatted Diff | Diff
Updated patch to fix issues in previous review (109.45 KB, patch)
2008-01-08 09:27 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Update for a few things bdash mentioned on IRC (109.78 KB, patch)
2008-01-08 10:22 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Update to use deleteFile for win32 also (109.70 KB, patch)
2008-01-08 13:49 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to build on Windows (118.94 KB, patch)
2008-01-09 10:18 PST, Mark Rowe (bdash)
no flags Details | Formatted Diff | Diff
Updated patch that runs correctly on Windows (124.56 KB, patch)
2008-01-10 03:06 PST, Mark Rowe (bdash)
no flags Details | Formatted Diff | Diff
Updated patch to fix 2 small issues (124.57 KB, patch)
2008-01-10 08:47 PST, Rodney Dawes
no flags Details | Formatted Diff | Diff
Updated patch to fix WebCore.pro and preserve history for the PluginStream code (138.40 KB, patch)
2008-01-10 12:25 PST, Rodney Dawes
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Rodney Dawes 2008-01-07 17:35:52 PST
As per the review of the patch in bug #14750 the attached patch implements a shared version of the PluginStream implementation, and abstracts platform-specific file i/o methods for temporary file handling into the FileSystem.h utilities.
Comment 1 Rodney Dawes 2008-01-07 17:36:26 PST
Created attachment 18323 [details]
Patch to implement shared PluginStream
Comment 2 Mark Rowe (bdash) 2008-01-07 21:04:14 PST
Comment on attachment 18323 [details]
Patch to implement shared PluginStream

I love the direction this is heading in!  Two issues that I have noticed:  
+#if 0
+    if (!m_loadManually)
+        m_pluginView->disconnectStream(this);
+#endif

I don't think this code can be omitted.  We don't land code with #if 0's in it either.


And:
+#if PLATFORM(WIN)
+    if (!m_path.isNull())
+        ::DeleteFileA(m_path.data());
+#endif

Looking at the code, this suggests that the file is only removed on Windows.  The removal of the temporary file should also be abstracted into FileSystem.h and called on all platforms.
Comment 3 Rodney Dawes 2008-01-08 09:27:07 PST
Created attachment 18328 [details]
Updated patch to fix issues in previous review

This patch resolves the #if 0'd code issue by implementing the PluginStreamClient class, and updating PluginViewWin to inherit from it. It also adds a deleteFile() call for platforms other than win32, to remove the temp file. Win32 uses ::DeleteFileA for the temp file, while deleteFile uses ::DeleteFileW, so we use an #if PLATFORM(WIN) to call ::DeleteFileA, and call deleteFile for other platforms.

Can someone please test that this patch builds and works on Win32? It needs to be tested there before being landed. Thanks.
Comment 4 Rodney Dawes 2008-01-08 10:22:44 PST
Created attachment 18329 [details]
Update for a few things bdash mentioned on IRC

Actually make PluginViewWin derive from PluginStreamClient this time. Also added a virtual destructor and made the streamDidFinishLoading virtual as well, as mentioned to me on IRC by bdash.
Comment 5 Adam Roben (:aroben) 2008-01-08 13:10:06 PST
Comment on attachment 18328 [details]
Updated patch to fix issues in previous review

+#if PLATFORM(WIN)
+    if (!m_path.isNull())
+        ::DeleteFileA(m_path.data());
+#else
+    String tempFilePath = String::fromUTF8(m_path.data());
+    if (!m_path.isNull())
+        deleteFile(tempFilePath);
+#endif

Calling deleteFile on Windows should be fine.
Comment 6 Rodney Dawes 2008-01-08 13:49:19 PST
Created attachment 18333 [details]
Update to use deleteFile for win32 also

Updated to use deleteFile on win32 as well, and only create the ::fromUTF8 String when needed, as per Adam's comments.

Can someone please test that this doesn't break the win32 build or plug-in support? Thanks.
Comment 7 Mark Rowe (bdash) 2008-01-09 10:18:12 PST
Created attachment 18352 [details]
Updated patch to build on Windows

I'm not flagging this for review as this currently crashes when navigating away from a page using an NP_ASFILEONLY stream (eg, embedded QuickTime).  I'll look into that soon.
Comment 8 Mark Rowe (bdash) 2008-01-10 03:06:38 PST
Created attachment 18362 [details]
Updated patch that runs correctly on Windows

Fixed the crash.  The file handle was not being initialized to a sane value so it was being treated as valid when in fact it had not been opened.  This lead to a crash when the handle was closed.

While debugging this issue I noticed some coding style issues and took the liberty of fixing them along with the crash.
Comment 9 Mark Rowe (bdash) 2008-01-10 03:10:37 PST
Rodney, I noticed that the patch appears to be missing the relevant WebCore.pro changes.
Comment 10 Rodney Dawes 2008-01-10 08:47:38 PST
Created attachment 18366 [details]
Updated patch to fix 2 small issues

This is an updated version of the patch to fix two small issues. There was a tab instead of 4 spaces in the initialization of m_tempFileHandle(), and g_build_filename() takes NULL as a terminator argument for the varargs.
Comment 11 Rodney Dawes 2008-01-10 12:25:03 PST
Created attachment 18370 [details]
Updated patch to fix WebCore.pro and preserve history for the PluginStream code

Here's an updated patch which adds PluginStream.cpp to WebCore.pro SOURCES, and uses svn mv to preserve the history for PluginStreamWin as it is now named PluginStream. This also fixes a build issue in the latest version where npfunctions.h was including npruntime.h and npapi.h directly instead of npruntime_internal.h, which is needed for building on X11.
Comment 12 Anders Carlsson 2008-01-10 22:34:39 PST
Comment on attachment 18370 [details]
Updated patch to fix WebCore.pro and preserve history for the PluginStream code

+CString openTempFileHandle(const char* prefix, PlatformFileHandle& handle);
+void closeTempFileHandle(PlatformFileHandle& handle);
+int writeToTempFileHandle(PlatformFileHandle handle, const char* data, int length);

I would prefer if "Handle" could be omitted from these functions (and maybe even "Temp"). Looks great otherwise. Thanks for working on this!
Comment 13 Mark Rowe (bdash) 2008-01-10 22:39:37 PST
I think openTempFileHandle definitely needs to mention "Temp" or "Temporary" in its name, while the others probably do not need it.  Agreed that the Handle suffix is redundant on the function names.
Comment 14 Mark Rowe (bdash) 2008-01-11 03:34:00 PST
Landed in r29399.  Thanks for the patch!