Bug 7451 – Refactor the way we handle plugin information (previously KConfig)

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Eric Seidel (no email)]

Refactor the way we handle plugin information (previously KConfig)

This is part one.  The second part will be moving the reworked class into Platform and splitting out the Obj-c specific goop.

[Eric Seidel (no email)]

Created attachment 6707 [details]
rework the way we handle plugin info

[Geoffrey Garen]

This needs a layout test. The plugins info page displayed by Help->Installed Plugins is a good candidate.

[Darin Adler]

Comment on attachment 6707 [details]
rework the way we handle plugin info

If it was me, I wouldn't bother having a PluginInfoStore class. I'd just have a single free function in the WebCore namespace that returns a Vector<PlugInInfo*>, perhaps named createPlugInInfoVector. I'd suggest Vector<OwnPtr<PlugInInfo> >, except I don't think that works. There's no requirement here to have a way to read the plug-in info for one plug-in at a time.

If PluginInfoStore is going to be noncopyable, it should be inheriting from the Noncopyable class. But I don't see any reason it needs to be non-copyable. You should remove the constructor from PluginInfoStore. And perhaps remove the class as I suggest above.

+    PluginInfo *createPluginInfoForPluginAtIndex(unsigned);

* should be by PluginInfo.

I think that Plugin is supposed to be PlugIn whenever possible, because it's "plug-in", not "plugin".

+    pluginInfo->file = String([plugin filename]);

I don't think you need the explicit String() here.

Nice improvement. r=me if you want to land it as is, or you could consider some of my suggestions.

[Eric Seidel (no email)]

Thanks for the great suggestions.  I've made some of them as part of landing this patch.  I will make the rest of them when I move this into platform (and split out the Obj-C specific goop).

Re: ggaren's comment on testing. I did a manual test of this using the Plugin Info page as Geoff suggested, but it's not possible to land a layout test for this yet, as DRT offers no way to control which plugins are loaded (to my knowledge).  I could write one which only searched for the one plugin that DRT adds, and will do that when I move this to platform.