Summary: | Refactor the way we handle plugin information (previously KConfig) | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Seidel (no email) <eric> | ||||
Component: | WebKit Misc. | Assignee: | Eric Seidel (no email) <eric> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | ggaren | ||||
Priority: | P4 | ||||||
Version: | 420+ | ||||||
Hardware: | Mac | ||||||
OS: | OS X 10.4 | ||||||
Bug Depends on: | |||||||
Bug Blocks: | 7498 | ||||||
Attachments: |
|
Description
Eric Seidel (no email)
2006-02-24 17:26:28 PST
Created attachment 6707 [details]
rework the way we handle plugin info
This needs a layout test. The plugins info page displayed by Help->Installed Plugins is a good candidate. 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.
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. |