Bug 7451 - Refactor the way we handle plugin information (previously KConfig)
Summary: Refactor the way we handle plugin information (previously KConfig)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 420+
Hardware: Macintosh OS X 10.4
: P4 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
Depends on:
Blocks: 7498
  Show dependency treegraph
 
Reported: 2006-02-24 17:26 PST by Eric Seidel (no email)
Modified: 2006-02-27 13:33 PST (History)
1 user (show)

See Also:


Attachments
rework the way we handle plugin info (19.71 KB, patch)
2006-02-24 17:27 PST, Eric Seidel (no email)
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-02-24 17:26:28 PST
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.
Comment 1 Eric Seidel (no email) 2006-02-24 17:27:14 PST
Created attachment 6707 [details]
rework the way we handle plugin info
Comment 2 Geoffrey Garen 2006-02-24 17:40:45 PST
This needs a layout test. The plugins info page displayed by Help->Installed Plugins is a good candidate.
Comment 3 Darin Adler 2006-02-24 19:09:07 PST
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.
Comment 4 Eric Seidel (no email) 2006-02-27 13:32:48 PST
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.