[webkit-reviews] review denied: [Bug 83012] Web Inspector: localStorage items are not updated when the storage changes : [Attachment 139027] 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 139027: Patch
https://bugs.webkit.org/attachment.cgi?id=139027&action=review

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


> Source/WebCore/ChangeLog:12
> +	   No new tests needed.

I think you need a test with this change.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:57
> +class StorageEventNotifier {

You probably want to declare it in anonymous namespace.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:65
> +    InspectorFrontend* m_frontend;

You should store InspectorFrontend::DOMStorage pointer here.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:79
> +    if (!m_timer.isActive())

You only invoke this right after the construction, no need to check for active
timer.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:85
> +    if (m_frontend)

No need for this check since you never clean it.

> Source/WebCore/storage/StorageEventDispatcher.cpp:62
> +#if ENABLE(INSPECTOR)

No need for ENABLE(INSPECTOR) when working with InspectorInstrumentation::

> Source/WebCore/storage/StorageEventDispatcher.cpp:63
> +		   // This is to ensure all the other open inspector pages in
the

Nuke the comment.

> Source/WebCore/storage/StorageEventDispatcher.cpp:65
> +		  
InspectorInstrumentation::didDispatchDOMStorageEvent(frames[i]->page(),
storage, frames[i].get(), key, oldValue, newValue);

Inspector agents are stored in inspector controller, on the per-page basis. You
don't need this to be in the for loop, just invoke it once for the
sourceFrame's page:
InspectorInstrumentation::didDispatchDOMStorageEvent(page, ...).

> Source/WebCore/storage/StorageEventDispatcher.cpp:85
> +#if ENABLE(INSPECTOR)

ditto

> Source/WebCore/storage/StorageEventDispatcher.cpp:99
> +	   InspectorInstrumentation::didDispatchDOMStorageEvent(page, storage,
sourceFrame, key, oldValue, newValue);

So this is essentially the only instrumentation you need.