[webkit-reviews] review granted: [Bug 16924] Shared PluginDatabase, PluginPackage, and PlugInInfoStore Implementations : [Attachment 19348] Updated patch to resolve more Windows build issues

Mon Feb 25 11:34:13 PST 2008

Jon Honeycutt <jhoneycutt at apple.com> has granted Rodney Dawes
<dobey at wayofthemonkey.com>'s request for review:
Bug 16924: Shared PluginDatabase, PluginPackage, and PlugInInfoStore
Implementations
http://bugs.webkit.org/show_bug.cgi?id=16924

Attachment 19348: Updated patch to resolve more Windows build issues
http://bugs.webkit.org/attachment.cgi?id=19348&action=edit

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