WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
17220
Illogical dependency between PluginView and PluginDatabase
https://bugs.webkit.org/show_bug.cgi?id=17220
Summary
Illogical dependency between PluginView and PluginDatabase
Rodney Dawes
Reported
2008-02-08 13:41:01 PST
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.
Attachments
Patch to move createPluginView method to PluginView class
(7.66 KB, patch)
2008-02-08 13:42 PST
,
Rodney Dawes
no flags
Details
Formatted Diff
Diff
Update to rename to PluginView::create
(7.62 KB, patch)
2008-02-08 16:35 PST
,
Rodney Dawes
andersca
: review+
Details
Formatted Diff
Diff
Updated patch that compiles on Windows
(8.24 KB, patch)
2008-02-08 18:41 PST
,
Mark Rowe (bdash)
no flags
Details
Formatted Diff
Diff
Updated patch with Jon's suggestions
(8.53 KB, patch)
2008-02-11 08:20 PST
,
Rodney Dawes
aroben
: review+
Details
Formatted Diff
Diff
Updated to revert the KURL.h change in PluginDatabase.h
(8.28 KB, patch)
2008-02-13 09:19 PST
,
Rodney Dawes
dobey
: review+
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Rodney Dawes
Comment 1
2008-02-08 13:42:08 PST
Created
attachment 19007
[details]
Patch to move createPluginView method to PluginView class
Adam Roben (:aroben)
Comment 2
2008-02-08 14:38:39 PST
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
Comment 3
2008-02-08 16:35:16 PST
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 4
2008-02-08 16:41:21 PST
Comment on
attachment 19010
[details]
Update to rename to PluginView::create r=me
Jon Honeycutt
Comment 5
2008-02-08 17:25:50 PST
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)
Comment 6
2008-02-08 18:41:48 PST
Created
attachment 19012
[details]
Updated patch that compiles on Windows
Mark Rowe (bdash)
Comment 7
2008-02-08 18:42:40 PST
> 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
Comment 8
2008-02-11 08:16:54 PST
(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
Comment 9
2008-02-11 08:20:23 PST
Created
attachment 19067
[details]
Updated patch with Jon's suggestions
Darin Adler
Comment 10
2008-02-11 09:20:47 PST
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
Comment 11
2008-02-11 11:02:58 PST
(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 12
2008-02-13 08:14:59 PST
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
Comment 13
2008-02-13 09:19:54 PST
Created
attachment 19114
[details]
Updated to revert the KURL.h change in PluginDatabase.h
Adam Roben (:aroben)
Comment 14
2008-02-13 09:29:08 PST
Landed in
r30203
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug