Shared PluginDatabase, PluginPackage, and PlugInInfoStore Implementations

• Component: Plug-ins
• Status: RESOLVED FIXED

[Rodney Dawes]

The attached patch moves the majority of these APIs into shared implementations, with platform-specific portions remaining in platform-specific sources.

[Rodney Dawes]

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.

[Rodney Dawes]

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.

[Mark Rowe (bdash)]

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.

[Rodney Dawes]

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.

[Alp Toker]

This looks OK to me. Any comments on the Win side?

[Rodney Dawes]

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.

[Rodney Dawes]

Created attachment 18620 [details]
Updated patch with ChangeLog

Oops. Forgot to svn resolved WebCore/ChangeLog last time. This includes the missing ChangeLog.

[Alp Toker]

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.

[Mark Rowe (bdash)]

It does break things on Windows.  I mentioned some issues to Rodney via IRC.

[Rodney Dawes]

Created attachment 18738 [details]
Updated patch to resolve some of mark's issues

[Rodney Dawes]

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.

[Rodney Dawes]

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.

[Darin Adler]

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.

[Mark Rowe (bdash)]

(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?

[Mark Rowe (bdash)]

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?

[Rodney Dawes]

(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?

[Mark Rowe (bdash)]

Weird!  A rather large hunk in PluginPackage.cpp failed to apply.

[Rodney Dawes]

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.

[Anders Carlsson]

Comment on attachment 18938 [details]
Patch to only do the renaming of API, and moving of files

Looks good, r=me

[Anders Carlsson]

Comment on attachment 18938 [details]
Patch to only do the renaming of API, and moving of files

Looks good, r=me

[Mark Rowe (bdash)]

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.

[Rodney Dawes]

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.

[Rodney Dawes]

Comment on attachment 18966 [details]
Patch for FileSystem changes

Clearing review flag as the changes for Wx need some additional fixes.

[Rodney Dawes]

Created attachment 18989 [details]
Updated patch to fix a few small issues with Wx and Qt changes

[Jon Honeycutt]

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.

[Rodney Dawes]

Created attachment 19004 [details]
Updated patch per Jon's comments

[Adam Roben (:aroben)]

Comment on attachment 19004 [details]
Updated patch per Jon's comments

Landed in r30092. Clearing review flag.

[Mark Rowe (bdash)]

Comment on attachment 18989 [details]
Updated patch to fix a few small issues with Wx and Qt changes

Clearing review flag on obsolete patch.

[Rodney Dawes]

Created attachment 19120 [details]
Patch to implement PluginDatabase as shared and use PluginInfoStore for GTK+, Qt, and Wx

[Jon Honeycutt]

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.

[Rodney Dawes]

Created attachment 19142 [details]
Updated patch per Jon's comments

[Jon Honeycutt]

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.

[Rodney Dawes]

Sounds plausible. I'll get an updated patch together asap.

[Rodney Dawes]

(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.

[Rodney Dawes]

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.

[Brent Fulgham]

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.

[Rodney Dawes]

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.

[Brent Fulgham]

(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.

[Jon Honeycutt]

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 @@
>[phone]  Rodney Dawes  <[e-mail]>
>+
>+        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.

[Rodney Dawes]

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.

[Mark Rowe (bdash)]

Landed in r30574.