[webkit-reviews] review cancelled: [Bug 17220] Illogical dependency between PluginView and PluginDatabase : [Attachment 19067] Updated patch with Jon's suggestions

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has cancelled Rodney Dawes
<dobey at wayofthemonkey.com>'s request for review:
Bug 17220: Illogical dependency between PluginView and PluginDatabase
http://bugs.webkit.org/show_bug.cgi?id=17220

Attachment 19067: Updated patch with Jon's suggestions
http://bugs.webkit.org/attachment.cgi?id=19067&action=edit

------- Additional Comments from Darin Adler <darin at apple.com>
r=me, although I'm not entirely happy with these changes.

 79	    static PluginView* create(Frame* parentFrame, const IntSize&,
Element* element, const KURL& url, const Vector<String>& paramNames, const
Vector<String>& paramValues, const String& mimeType, bool loadManually);

We'd normally omit the name for the Element* and const KURL& parameters,
because the type names make it clear what they are.

 141	     PluginView(Frame* parentFrame, const IntSize&, PluginPackage*
plugin, Element*, const KURL&, const Vector<String>& paramNames, const
Vector<String>& paramValues, const String& mimeType, bool loadManually);

We'd normally omit the name for the PluginPackage* parameter, because the type
name makes it clear what it is.

 32 #include "KURL.h"

It does not seem good to add KURL.h to the PluginDatabase.h header. Why are you
doing that? Please don't add more includes if you can avoid it!

 55	    PluginPackage* findPlugin(const KURL& url, String& mimeType);
5556	 private:

Please leave a blank line before the private: line.

I don't think it's clear what you mean by "illogical dependency". I don't see
anything illogical here. It'll be easier to understand your changes if you use
more-precise language.