Bug 17220 – Illogical dependency between PluginView and PluginDatabase

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Rodney Dawes]

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.

[Rodney Dawes]

Created attachment 19007 [details]
Patch to move createPluginView method to PluginView class

[Adam Roben (:aroben)]

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".

[Rodney Dawes]

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.

[Anders Carlsson]

Comment on attachment 19010 [details]
Update to rename to PluginView::create

r=me

[Jon Honeycutt]

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.

[Mark Rowe (bdash)]

Created attachment 19012 [details]
Updated patch that compiles on Windows

[Mark Rowe (bdash)]

> 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.

[Rodney Dawes]

(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.

[Rodney Dawes]

Created attachment 19067 [details]
Updated patch with Jon's suggestions

[Darin Adler]

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.

[Rodney Dawes]

(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.

[Adam Roben (:aroben)]

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

[Rodney Dawes]

Created attachment 19114 [details]
Updated to revert the KURL.h change in PluginDatabase.h

[Adam Roben (:aroben)]

Landed in r30203