Add an initial stab at a generic supplimental interface for WebProcess
Created attachment 180774 [details] Patch
Attachment 180774 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/WebProcess/KeyValueStorage/WebKeyValueStorageManager.h:32: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 13 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 180774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180774&action=review Change looks OK, but not sure about the rationale for using a class and virtual functions here. > Source/WebKit2/WebProcess/WebProcessSupplement.h:43 > + virtual void initialize(const WebProcessCreationParameters&) > + { > + } I don’t understand why this is a virtual function, thus I don’t understand the purpose of the WebProcessSupplement class. I see no clients that are calling this function polymorphically. It seems to me that we could just have a calling convention about having initialize functions that take WebProcessCreationParameters instead of a base class with virtual functions. If this does stay a class, then given the way this is currently used, I suggest a pure virtual function here instead. Callers aren’t calling through to the base class, and all derived classes are required to implement this, it seems. > Source/WebKit2/WebProcess/KeyValueStorage/WebKeyValueStorageManager.h:40 > -class WebKeyValueStorageManager : public WebCore::StorageTrackerClient, private CoreIPC::MessageReceiver { > +class WebKeyValueStorageManager : public WebCore::StorageTrackerClient, public WebProcessSupplement { Why public derivation from the WebProcessSupplement base class? What client needs to know this is a supplement? I couldn’t find that line of code. It seems all the clients need is the initialize function, which is defined publicly below and does not depend on the base class at all. So this should be public inheritance, or perhaps not inheritance at all (see my comment above). > Source/WebKit2/WebProcess/KeyValueStorage/WebKeyValueStorageManager.h:46 > + // WebProcessSupplement > + virtual void initialize(const WebProcessCreationParameters&) OVERRIDE; To me this seems simply a part of the initialization process for this class and not really related to the base class in any interesting way. Not clear why the function is virtual nor why OVERRIDE is relevant. > Source/WebKit2/WebProcess/WebCoreSupport/WebDatabaseManager.h:40 > -class WebDatabaseManager : public WebCore::DatabaseManagerClient, private CoreIPC::MessageReceiver { > +class WebDatabaseManager : public WebCore::DatabaseManagerClient, public WebProcessSupplement { Same question about public derivation as above. > Source/WebKit2/WebProcess/WebCoreSupport/WebDatabaseManager.h:46 > + // WebProcessSupplement > + virtual void initialize(const WebProcessCreationParameters&) OVERRIDE; Same comments as above.
(In reply to comment #3) > (From update of attachment 180774 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180774&action=review > > Change looks OK, but not sure about the rationale for using a class and virtual functions here. > > > Source/WebKit2/WebProcess/WebProcessSupplement.h:43 > > + virtual void initialize(const WebProcessCreationParameters&) > > + { > > + } > > I don’t understand why this is a virtual function, thus I don’t understand the purpose of the WebProcessSupplement class. I see no clients that are calling this function polymorphically. It seems to me that we could just have a calling convention about having initialize functions that take WebProcessCreationParameters instead of a base class with virtual functions. > > If this does stay a class, then given the way this is currently used, I suggest a pure virtual function here instead. Callers aren’t calling through to the base class, and all derived classes are required to implement this, it seems. > Ah, indeed, adding the intent would have been wise on my part. The general idea is to have a system where the WebProcess object holds map of something (probably a name)->WebProcessSupplement, so that it is possible to extend the WebProcess object without polluting the object itself. (This will be useful to those that want to extend it in ways that aren't needed or wanted by every port.) Once there is a map, the virtual functions (which in the derived class will be able to become private at that time) will make more sense. The reason this looks a bit odd right now, is that I am trying to stage it as not to have too large a patch and not break too much each time. I will add the explanation to the ChangeLog where it should have been to start with.
Committed r138511: <http://trac.webkit.org/changeset/138511>