[webkit-reviews] review denied: [Bug 83012] Web Inspector: localStorage items are not updated when the storage changes : [Attachment 141666] Patch

[bugzilla-daemon at webkit.org]

Pavel Feldman <pfeldman at chromium.org> has denied Vivek Galatage
<vivekgalatage at gmail.com>'s request for review:
Bug 83012: Web Inspector: localStorage items are not updated when the storage
changes
https://bugs.webkit.org/show_bug.cgi?id=83012

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

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


Looks good. Couple of comments inline.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:194
> +    if (!m_frontend && !m_enabled)

&& -> || ?

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:206
> +    m_frontend->domstorage()->updateDOMStorage(id);

Following naming pattern should be used for the protocol methods:
"domStorageUpdated"

> Source/WebCore/inspector/InspectorInstrumentation.cpp:921
> +    if (!inspectorAgent || !inspectorAgent->developerExtrasEnabled())

no need to check for developer extras here.

> Source/WebCore/inspector/InspectorInstrumentation.h:1161
> +    if (InstrumentingAgents* instrumentingAgents =
instrumentingAgentsForPage(sourceFrame->page()))

Should have
FAST_RETURN_IF_NO_FRONTENDS(void());

> LayoutTests/inspector/storage-panel-dom-storage-update.html:30
> +    function dumpDataGrid() {

Here and below: You should keep { on the next line.

> LayoutTests/inspector/storage-panel-dom-storage-update.html:96
> +	       setTimeout(viewUpdatedAfterRemoval, 0 );       

setTimeout will not necessarily do what you expect. Running after pending
dispatches should be more than enough. Also, you should only use it when
absolutely necessary. It is often more reliable to do InspectorTest.addSniffer
and sniff for the UI update you are interested in.

> LayoutTests/inspector/storage-panel-dom-storage-update.html:119
> +    InspectorTest.runTestSuite([

We always declare these functions inline.