[webkit-reviews] review granted: [Bug 105779] Add an initial stab at a generic supplemental interface for WebProcess : [Attachment 180774] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Sam Weinig <sam at webkit.org>'s request
for review:
Bug 105779: Add an initial stab at a generic supplemental interface for
WebProcess
https://bugs.webkit.org/show_bug.cgi?id=105779

Attachment 180774: Patch
https://bugs.webkit.org/attachment.cgi?id=180774&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
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.