Bug 16779 – Make the PluginStream implementation be shared across platforms

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Rodney Dawes]

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.

[Rodney Dawes]

Created attachment 18323 [details]
Patch to implement shared PluginStream

[Mark Rowe (bdash)]

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.

[Rodney Dawes]

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.

[Rodney Dawes]

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.

[Adam Roben (:aroben)]

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.

[Rodney Dawes]

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.

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

Rodney, I noticed that the patch appears to be missing the relevant WebCore.pro changes.

[Rodney Dawes]

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.

[Rodney Dawes]

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.

[Anders Carlsson]

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!

[Mark Rowe (bdash)]

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.

[Mark Rowe (bdash)]

Landed in r29399.  Thanks for the patch!