Currently createPluginView is a class method in PluginDatabase, presumably because findPlugin was a private class method there. This is unnecessary and causes an unneeded dependency between the two classes. The attached patch makes findPlugin a public method, and moves createPluginView to the PluginView class, as well as updates the instances where it is called, to reflect this change.
Created attachment 19007 [details] Patch to move createPluginView method to PluginView class
Comment on attachment 19007 [details] Patch to move createPluginView method to PluginView class If this method is moving to the PluginView class then the method should be named simply "create" instead of "createPluginView".
Created attachment 19010 [details] Update to rename to PluginView::create Oops. Also forgot to change PluginDatabase:: when copying the method over. Renamed the method to PluginView::create and updated the ChangeLog entries to reflect that.
Comment on attachment 19010 [details] Update to rename to PluginView::create r=me
Comment on attachment 19010 [details] Update to rename to PluginView::create The call to PluginDatabase::installedPlugins() in PluginView::create() can't be made without making PluginView aware of PluginDatabase. Doesn't that just move the dependency around? I think the change is still OK to make, but this needs some work: + PluginView* create(Frame* parentFrame, const IntSize&, Element* element, const KURL& url, const Vector<String>& paramNames, const Vector<String>& paramValues, const String& mimeType, bool loadManually); This needs to be static. PluginView should include PluginDatabase.h. PluginDatabase.h shouldn't include PluginView.h, but will then require KURL.h. You can then remove the "class KURL;" declaration. The ChangeLog entry should be updated.
Created attachment 19012 [details] Updated patch that compiles on Windows
> The call to PluginDatabase::installedPlugins() in PluginView::create() can't be > made without making PluginView aware of PluginDatabase. Doesn't that just move > the dependency around? I'm not landing this yet as I think this question still needs to be addressed.
(In reply to comment #7) > > The call to PluginDatabase::installedPlugins() in PluginView::create() can't be > > made without making PluginView aware of PluginDatabase. Doesn't that just move > > the dependency around? Ah, unnecessary/unneeded was the wrong term to use. Illogical is more accurate. Yes, this makes the dependency logical. PluginDatabase should only manage the PluginPackage instances, and PluginView then requests the appropriate PluginPackage from PluginDatabase, to use in the PluginView creation.
Created attachment 19067 [details] Updated patch with Jon's suggestions
Comment on attachment 19067 [details] Updated patch with Jon's suggestions 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.
(In reply to comment #10) > (From update of attachment 19067 [details] [edit]) > r=me, although I'm not entirely happy with these changes. I presume you meant to set review+ instead of clearing the review flag? Seems odd to say r=me, and clear the flag. :) > We'd normally omit the name for the Element* and const KURL& parameters, > because the type names make it clear what they are. > We'd normally omit the name for the PluginPackage* parameter, because the type > name makes it clear what it is. These can easily be removed. They were already there however, and I merely just copied the API between files, and changed the method name. :) > 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! PluginDatabase.h still uses KURL in findPlugin(). As per comment #5 from Jon: PluginDatabase.h shouldn't include PluginView.h, but will then require KURL.h. You can then remove the "class KURL;" declaration. > 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. PluginDatabase does not manage a database of PluginViews. It manages a database of PluginPackages. Asking it for a PluginView is therefore illogical, as explained in comment #8. It lacks the logical hierarchy of a well-designed API. This change makes PluginView API consistent with the PluginPackage API, where we call PluginPackage::createPackage.
Comment on attachment 19067 [details] Updated patch with Jon's suggestions +#include "KURL.h" #include "PlatformString.h" #include "PluginPackage.h" #include "StringHash.h" @@ -37,7 +38,6 @@ namespace WebCore { class Element; class Frame; class IntSize; - class KURL; I know Jon said to include KURL.h, but I think he was mistaken and it's not really needed. Everything should compile fine with just a forward declaration, so you should be able to undo this change. r=me
Created attachment 19114 [details] Updated to revert the KURL.h change in PluginDatabase.h
Landed in r30203