The attached patch moves the majority of these APIs into shared implementations, with platform-specific portions remaining in platform-specific sources.
Created attachment 18529[details]
Patch to provide shared API classes and implementations
This patch affects the Windows, GTK+, and Qt builds. Please test that the Windows and Qt builds still build and work as expected, before landing this patch.
Created attachment 18531[details]
Updated to use String::fromUTF8 with glib strings
This patch just switches to using fromUTF8 for creating Strings from glib strings. Still needs to be built/tested on win32 and qt.
A few things jump out at me. You've taken two member variables of PluginPackage and turned them into file-level statics, which seems like a bad way for passing data between functions.
+static DWORD m_fileVersionLS = 0;
+static DWORD m_fileVersionMS = 0;
You've moved isPluginBlacklisted to a static function rather than a member function, but it appears to need access to the instance to check name() and fileName(), and the two file version variables mentioned above.
The new PluginViewWin::createPluginView is not declared correctly in the header. It should be declared as static and have the class name omitted.
There's a small change in two places in PluginDatabase that needs some justification:
for (PluginSet::const_iterator it = m_plugins.begin(); it != end; ++it) {
if ((*it)->mimeToDescriptions().contains(key)) {
plugin = (*it).get();
- // prefer plugins in our own plugins directory
- if (plugin->parentDirectory() == ourPath)
- break;
}
}
Why is it ok to remove this?
I'm not sure that it makes sense to put unloadModule in FileSystem.cpp either. It doesn't seem to be all that related to the file system.
Created attachment 18604[details]
Updated patch to fix issues
This patch resolves the issues presented by Mark in review of the previous version.
As for the FileSystem concern, it is probably the wrong name for the file, as none of the operations in it are actually filesystem-dependent. They are file operations, not filesystem operations. A module is a special type of file. I think the method to unload it in a platform-independent way belongs in the same place as closing a normal file in a platform-independent way.
Created attachment 18619[details]
Updated patch
Discovered a crash in the gtk+ build with the patch. Fixed the crash and updated TemporaryLinkStubs for Qt as well in this revision.
Created attachment 18620[details]
Updated patch with ChangeLog
Oops. Forgot to svn resolved WebCore/ChangeLog last time. This includes the missing ChangeLog.
Mark, can you confirm that this doesn't break anything on Windows?
I think the best way to break the current deadlock is to get this landed and work on it in-tree.
Can we PLEASE get this reviewed so we can get it landed? It's very important, and the longer it takes to get reviewed and landed, the bigger the patch is going to get.
Created attachment 18916[details]
Updated patch against svn trunk r29968
Here's an updated version against r29968. Can we PLEASE get this reviewed/landed? Thanks.
Comment on attachment 18916[details]
Updated patch against svn trunk r29968
- if (compareFileVersion(silverlightPluginMinRequiredVersionMS, silverlightPluginMinRequiredVersionLS) >= 0)
+ if (info->dwFileVersionMS < silverlightPluginMinRequiredVersionMSDWORD ||
+ (info->dwFileVersionMS == silverlightPluginMinRequiredVersionMSDWORD && info->dwFileVersionLS < silverlightPluginMinRequiredVersionLSDWORD))
You want the new version of this, with the compareFileVersion function. Not the old version.
This patch makes too many changes at once, making it unnecessarily large, and making it harder to get review.
I suggest we do the renaming part of this patch first, separately, so we can more clearly see the changes other than renaming. Lets use the do-webcore-rename script to do the renaming.
Then a next patch could be to just add the new FileSystem functions.
Then we can see what's left. The patch remaining after taking those steps should be way smaller.
(In reply to comment #12)
> Created an attachment (id=18916) [edit]
> Updated patch against svn trunk r29968
>
> Here's an updated version against r29968. Can we PLEASE get this
> reviewed/landed? Thanks.
>
Rodney, this patch doesn't apply cleanly against r29968. Are you sure that's the right revision?
I've been looking at this and I think the first step should be:
PluginDatabaseWin -> PluginDatabase
PluginPackageWin -> PluginPackage
PlugInInfoStore -> PluginInfoStore (I'm not sure whether we want Plugin or PlugIn, but either way I think we should be consistent about this while we're renaming things)
It should also move the PluginInfoStore header into WebCore/plugins from WebCore/platform, and the mac and qt implementations of PlugInInfoStore into plugins/{mac,qt}.
Using do-webcore-renames will result in WebCore/plugins/win/PluginDatabaseWin.cpp being renamed to WebCore/plugins/win/PluginDatabase.cpp when we'll probably want it to eventually end up staying at the original location and only containing the Windows-specific bits of code. I think it would be fine to have it renamed briefly and then move it out of plugins/win/ into plugins/ in a later revision, which can also move the Windows-specific bits back to PluginDatabaseWin.cpp.
Does this sound like a reasonable first step?
(In reply to comment #14)
> Rodney, this patch doesn't apply cleanly against r29968. Are you sure that's
> the right revision?
That's the revision "svn info" tells me. What failed?
Created attachment 18938[details]
Patch to only do the renaming of API, and moving of files
Here is a patch which only does the moving of files, and renaming of API. PluginInfoStore.cpp and npapi.cpp will be able to built cross-platform, so they have also been moved. Can we please get this in so the rest of the refactoring can go more smoothly? Thanks.
Comment on attachment 18938[details]
Patch to only do the renaming of API, and moving of files
Landed this part of the change in r30015, with two build fixes shortly after. Clearing review flag to get this change out of the commit queue.
Created attachment 18966[details]
Patch for FileSystem changes
This patch adds two new methods to FileSystem.h, and implements them for the GTK+ and Windows ports. It also defines two new cross-platform types, and stubs out the new and missing methods for the Qt and Wx ports. These new methods are needed for the cross-platform plug-in api refactoring.
Comment on attachment 18989[details]
Updated patch to fix a few small issues with Wx and Qt changes
Looks good to me. Few small comments:
+/* Methods for dealing with loadable modules */
We prefer // style comments.
+ * Copyright (C) 2008 Collabora, LTd. All rights reserved.
Typo: LTd.
+String pathGetFileName(const String& path)
+{
+ notImplemented();
+ return String();
+}
+
+bool unloadModule(PlatformModule module)
+{
+ notImplemented();
+ return false;
+}
No need to name the args here.
Comment on attachment 19120[details]
Patch to implement PluginDatabase as shared and use PluginInfoStore for GTK+, Qt, and Wx
This mostly looks good, although it does break the Windows build, because PluginPackage::storeFileVersion was removed but is still in PluginPackageWin.cpp.
I think a good fix would be to add an abstraction for a PlatformModuleVersion and make storeModuleVersion a static helper function.
+PluginSet PluginDatabase::getPluginsInPaths() const { notImplemented(); PluginSet plugins; return plugins;}
+Vector<String> PluginDatabase::defaultPluginPaths() { notImplemented(); Vector<String> paths; return paths;}
No need to name the return values, need a space before closing brace.
+bool PluginPackage::fetchInfo() { notImplemented(); return false;}
+unsigned PluginPackage::hash() const { notImplemented(); return 0;}
Need a space before closing brace.
r- for the build breakage, but I think this looks great.
Comment on attachment 19142[details]
Updated patch per Jon's comments
PluginView::determineQuirks() on Windows uses PluginPackage::compareFileVersion(), which is no longer available.
I think there should be a cross-platform abstraction for a module version, even if it's only used on Windows for now. I think it's likely that other platforms will eventually have the same uses for it that Windows has: preferring the latest version of several installed plug-ins that handle the same MIME type, or determining plug-in idiosyncrasies specific to only some versions.
My suggestion is then:
#if PLATFORM(WIN)
typedef struct { unsigned leastSig; unsigned mostSig; } PlatformModuleVersion;
#else
typedef unsigned PlatformModuleVersion;
#endif
int PluginPackage::compareFileVersion(int, int) const ->
int PluginPackage::compareModuleVersion(const PlatformModuleVersion&) const
etc. Does this seem like a good idea to you?
r- for the build breakage mentioned above.
(In reply to comment #32)
> (From update of attachment 19142[details] [edit])
> PluginView::determineQuirks() on Windows uses
> PluginPackage::compareFileVersion(), which is no longer available.
>
> etc. Does this seem like a good idea to you?
Should we just move ::determineQuirks() to PluginPackage instead? It seems like the more appropriate place for it. It doesn't seem like there is any code specific to the PluginView inside determineQuirks(), while it is specific to the PluginPackage. It seems like it belongs there instead, reading the code, and moving it would allow us to clean up some of the code there as well.
Created attachment 19284[details]
Updated patch to move determineQuirks to PluginPackage also
Here's an updated patch. This version also moves the quirks to PluginPackage instead of PluginView, as they are issues with the plug-ins themselves, and not the view, and other classes may want to peruse them in the future, that don't use a view, and don't depend on PluginView.
1. PluginPackageWin.cpp
You are missing an include:
#include "MIMETypeRegistry.h"
- , m_fileVersionLS(0)
- , m_fileVersionMS(0)
+ , m_moduleVersion(0)
This fails to compile, but can be made to work by modifying PluginPackage.h as follows. You need the one-argument version to allow initialization with zero, the two-value version for the other structure declarations.
#if PLATFORM(WIN)
struct PlatformModuleVersion
{
unsigned leastSig;
unsigned mostSig;
PlatformModuleVersion(unsigned)
: leastSig(0)
, mostSig(0)
{
}
PlatformModuleVersion(unsigned lsb, unsigned msb)
: leastSig(lsb)
, mostSig(msb)
{
}
};
#else
typedef unsigned PlatformModuleVersion;
#endif
2. This method declaration looks wrong:
-int PluginPackage::compareFileVersion(unsigned compareVersionMS, unsigned compareVersionLS) const
+int PluginPackage::compareFileVersion(const PlatformModuleVersion& compareVersion)
Why are you losing the 'const' here? I get a compile failure due to this under Windows. You probably didn't notice this since it doesn't get built on your platform.
3. The use of a structure for the PlatformModuleVersion means that this:
static const PlatformModuleVersion slPluginMinRequired = {0x51BE0000, 0x00010000};
must become this:
static const PlatformModuleVersion slPluginMinRequired(0x51BE0000, 0x00010000);
likewise:
static const PlatformModuleVersion lastKnownUnloadableRealPlayerVersion = { 0x000B0B24, 0x00060000 };
4. You have the wrong classname used for 'determineQuirks' in PluginPackageWin.cpp:
void PluginView::determineQuirks(const String& mimeType)
should be:
void PluginPackage::determineQuirks(const String& mimeType)
5. Typo:
m_mdouleVersion.mostSig = info->dwFileVersionMS;
should be
m_moduleVersion.mostSig = info->dwFileVersionMS;
6. Use of the OwnArrayPtr prevents the LPVOID signature getVersion function from working. I got things to compile by using the get() method, which produces a char* which will implicitly cast to a void*. This seems to be what was done elsewhere in the file.
m_name = getVersionInfo(versionInfoData, "ProductName");
m_description = getVersionInfo(versionInfoData "FileDescription");
becomes
m_name = getVersionInfo(versionInfoData.get(), "ProductName");
m_description = getVersionInfo(versionInfoData.get(), "FileDescription");
likewise:
fastFree(versionInfoData);
becomes:
fastFree(versionInfoData.get());
and:
if (!VerQueryValue(versionInfoData, TEXT("\\"), (LPVOID*) &info, &infoSize) || infoSize < sizeof(VS_FIXEDFILEINFO))
return false;
becomes:
if (!VerQueryValue(versionInfoData.get(), TEXT("\\"), (LPVOID*) &info, &infoSize) || infoSize < sizeof(VS_FIXEDFILEINFO))
return false;
7. The method bool PluginPackage::fetchInfo(){ has some strange indenting and is missing a curly brace. The culprit appears to be this line:
if (m_name.isNull() || m_description.isNull()) {
fastFree(versionInfoData);
Note that this is also mentioned above due to the LPVOID casting problem. I suspect the curly brace is not needed here.
Created attachment 19348[details]
Updated patch to resolve more Windows build issues
Here's an updated patch which should take care of all the issues now. Thanks again for the reviewing and build testing on Windows.
(In reply to comment #37)
> Created an attachment (id=19348) [edit]
> Updated patch to resolve more Windows build issues
>
> Here's an updated patch which should take care of all the issues now. Thanks
> again for the reviewing and build testing on Windows.
>
After applying this patch, there is still the minor problem of a missing header include in PluginPackageWin.cpp:
#include "MIMETypeRegistry.h"
Once this correction is made, the code compiles properly and seems to work properly under the Windows platform.
Comment on attachment 19348[details]
Updated patch to resolve more Windows build issues
Looks good! r=me, few comments below.
><html><body><pre style="word-wrap: break-word; white-space: pre-wrap;">Index: WebCore/ChangeLog
>===================================================================
>--- WebCore/ChangeLog (revision 30564)
>+++ WebCore/ChangeLog (working copy)
>@@ -1,3 +1,43 @@
>+2008-02-25 Rodney Dawes <dobey@wayofthemonkey.com>
>+
>+ Reviewed by NOBODY (OOPS!!)
>+
>+ Add PluginInfoStore.cpp and new PluginDatabase.cpp to GTK+ and Qt ports
>+ Remove old PlugInInfoStoreQt.cpp as it is obsoleted by shared code
>+ Add PluginInfoStore, PluginDatabase, and PluginStream files to Wx build
>+ Add new PluginDatabase.cpp to Windows build
>+ Add temporary stubs for new PluginDatabase and PluginPackage
>+ shared classes to GTK+, Qt, and Wx ports
>+ Copy PluginDatabaseWin.cpp to PluginDatabase.cpp to preserve history
>+ Remove shared code from PluginDatabaseWin.cpp
>+ Remove Windows-specific code from PluginDatabase.cpp
>+ Use PlatformModule and PlatformFileTime instead of HMODULE and FILETIME
>+ Remove extraneous PluginPackage:: from hash() class method prototype
>+ Subsume storeFileVersion into PluginPackage::fetchInfo
>+ Add cross-platform PlatformModuleVersion type definition
>+ Use PlatformModuleVersio to store the module version
>+ Rename m_fileVersion[ML]S to m_moduleVersion
>+ Change compareFileVersion to use PlatformModuleVersion as the argument
>+ Move PluginView::determineQuirks and m_quirks to PluginPackage
>+ Updated determineQuirks for the PlatformModuleVersion
Typo: PlatformModuleVersio.
Please add punctuation to each sentence.
>Index: WebCore/plugins/PluginPackage.h
>
>+#if PLATFORM(WIN)
>+struct PlatformModuleVersion {
I think this would fit better in FileSystem.h with the other platform definitions.
>+ int compareFileVersion(const PlatformModuleVersion&) const;
>+
>+ PluginQuirkSet m_quirks;
Please make m_quirks private and add a getter.
>Index: WebCore/plugins/win/PluginPackageWin.cpp
>@@ -76,49 +77,37 @@ void PluginPackage::freeLibraryTimerFire
> PluginPackage::PluginPackage(const String& path, const FILETIME& lastModified)
> : RefCounted<PluginPackage>(0)
> , m_path(path)
>+ , m_moduleVersion(0)
Should remove this.
Needs MIMETypeRegistry.h as bfulgham mentioned.
>+ static const PlatformModuleVersion slPluginMinRequired(0x51BE0000, 0x00010000 );
Extra space here.
>+ // Determine the quirks for the MIME types this plug-in supports
>+ determineQuirks(type);
>+
I think there's a tab here. Please remove this and grep for any other tabs.
>Index: WebCore/plugins/win/PluginViewWin.cpp
Should use the quirks getter throughout this file.
Created attachment 19354[details]
Updated patch to fix last minor issues
Updated patch which incorporates Jon's suggestions, minus the removal of the m_moduleVersion initializer, which he was confused about. I'm setting the flag to r+ as he set the previous version of the patch, and this version solves the issues he mentioned.
2008-01-18 12:57 PST, Rodney Dawes
2008-01-18 13:24 PST, Rodney Dawes
2008-01-22 13:17 PST, Rodney Dawes
2008-01-23 08:43 PST, Rodney Dawes
2008-01-23 08:55 PST, Rodney Dawes
2008-01-28 11:27 PST, Rodney Dawes
2008-02-04 13:06 PST, Rodney Dawes
2008-02-05 12:04 PST, Rodney Dawes
2008-02-06 12:42 PST, Rodney Dawes
2008-02-07 14:22 PST, Rodney Dawes
2008-02-08 07:15 PST, Rodney Dawes
2008-02-14 11:52 PST, Rodney Dawes
2008-02-15 12:45 PST, Rodney Dawes
2008-02-22 16:14 PST, Rodney Dawes
2008-02-25 07:45 PST, Rodney Dawes
2008-02-25 12:39 PST, Rodney Dawes