Bug 30786 – Fix QtWebKit build for WIN_OS if Netscape plug-in support is turned off and refactor some related code

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Laszlo Gombos]

QtWebKit for WIN_OS fails to link if Netscape plug-in support is turned off as there is no implementation for PluginPackage::compareFileVersion function. There is one implementation for this function in PluginPackage.cpp and that is guarded with the ENABLE_PLUGIN_PACKAGE_SIMPLE_HASH which is not enabled for WIN_OS (see Platform.h). There is also an implementation for this interface in PluginPackageWin.cpp which is not built when Netscape plug-in support is turned off.

This problem domain is related to the PlatformModuleVersion type definition (see FileSystem.h). Notice that the there are a few areas where type definition is repeated (copied) instead of reused (e.g. PlatformModuleVersion type definition is repeated for 2x) - One should attempt to refactor FileSystem.h as part of the build fix patch as I think these changes belong together.

[Laszlo Gombos]

Created attachment 41889 [details]
proposed patch

* platform/FileSystem.h: Refactor to make sure that each different type definition is only repeated once.

* plugins/PluginPackage.cpp: (WebCore::PluginPackage::compareFileVersion): Move it out from the ENABLE_PLUGIN_PACKAGE_SIMPLE_HASH guard and combine it with the version from PluginPackageWin.

* plugins/win/PluginPackageWin.cpp: Remove compareFileVersion as it is now in PluginPackage.cpp.

[Antti Koivisto]

Comment on attachment 41889 [details]
proposed patch

Why not keep the windows specific function in the PluginPackageWin? Having both PluginPackageWin and Windows ifdefs in base PluginPackage.cpp seems weird.

> +#if PLATFORM(WIN_OS)
> +    if (m_moduleVersion.mostSig != compareVersion.mostSig)
> +        return m_moduleVersion.mostSig > compareVersion.mostSig ? 1 : -1;
> +    if (m_moduleVersion.leastSig != compareVersion.leastSig)
> +        return m_moduleVersion.leastSig > compareVersion.leastSig ? 1 : -1;

I realize this is copy code but is this really correct? If mostSig differs the leastSig is ignored? (i don't really know anything about windows plugins).

[Laszlo Gombos]

(In reply to comment #2)
> (From update of attachment 41889 [details])
> Why not keep the windows specific function in the PluginPackageWin? Having both
> PluginPackageWin and Windows ifdefs in base PluginPackage.cpp seems weird.

If netscape plugin support is turned off PluginPackageWin is not compiled in for QtWebKit (none of the files from under plugins/win). Moving the function to PluginPackage.cpp seemd like the best option.

> 
> > +#if PLATFORM(WIN_OS)
> > +    if (m_moduleVersion.mostSig != compareVersion.mostSig)
> > +        return m_moduleVersion.mostSig > compareVersion.mostSig ? 1 : -1;
> > +    if (m_moduleVersion.leastSig != compareVersion.leastSig)
> > +        return m_moduleVersion.leastSig > compareVersion.leastSig ? 1 : -1;
> 
> I realize this is copy code but is this really correct? If mostSig differs the
> leastSig is ignored? (i don't really know anything about windows plugins).

This is so that v3.1 > v2.2 - it seems reasonable to me.

[Antti Koivisto]

Comment on attachment 41889 [details]
proposed patch

Ok. As a cleaner alternative, you could eliminate the need for WIN_OS ifdef by giving PlatformModuleVersion struct the inequality and comparison operator.

But it is not that important r=me in any case.

[WebKit Commit Bot]

Comment on attachment 41889 [details]
proposed patch

Clearing flags on attachment: 41889

Committed r50138: <http://trac.webkit.org/changeset/50138>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.