[webkit-reviews] review denied: [Bug 54455] Web Inspector: move Database and DOMStorage related things from InspectorAgent : [Attachment 82447] [patch] initial version

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Ilya Tikhonovsky
<loislo at chromium.org>'s request for review:
Bug 54455: Web Inspector: move Database and DOMStorage related things from
InspectorAgent
https://bugs.webkit.org/show_bug.cgi?id=54455

Attachment 82447: [patch] initial version
https://bugs.webkit.org/attachment.cgi?id=82447&action=review

------- Additional Comments from Pavel Feldman <pfeldman at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82447&action=review

>> Source/WebCore/inspector/InspectorAgent.cpp:142
>> +	,
m_databaseAgentResources(InspectorDatabaseAgent::createOfflineStorage())
> 
> Consider renaming createOfflineStorage to createStorage

This does not really make the agents modular.

> Source/WebCore/inspector/InspectorAgent.cpp:393
> +    m_databaseAgent =
InspectorDatabaseAgent::create(databaseAgentResources(), m_frontend);

Pass inspectorAgent into it instead.

>> Source/WebCore/inspector/InspectorAgent.cpp:463
>> +	m_databaseAgentResources->pushDataCollectedOffline(m_frontend);
> 
> pushDataCollectedOffline should be a method on the database agent.

This should be done upon agent instantiation, from within agent.

>> Source/WebCore/inspector/InspectorAgent.cpp:466
>> +	m_domStorageAgentResources->pushDataCollectedOffline(m_frontend);
> 
> Same here.

ditto.

> Source/WebCore/inspector/InspectorAgent.h:105
> +    virtual void pushDataCollectedOffline(InspectorFrontend*) = 0;

I think adding pushToFrontend method to the storage does not make much sense.
Storage should be simple, it should store.