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

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


> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:78
> +    m_timer.startOneShot(0);

So this is not for the event coalescing. Why not to dispatch it synchronously
then?

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:83
> +    m_frontend->updateDOMStorage(m_id);

Now that there are no m_frontend checks here, you should also kill the timer
from within clearFrontend.

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

We usually do
if (!m_frontend)
    return;

in top level guards.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:230
> +	       m_notifier->notify();

Call it from the constructor ?

> Source/WebCore/storage/StorageEventDispatcher.cpp:83
> +    Storage* storage = storageType == SessionStorage ?
sourceFrame->domWindow()->sessionStorage(ec) :
sourceFrame->domWindow()->localStorage(ec);

We try to avoid extra work in case inspector is not interested. In this case, I
would pass exactly the information you receives into the instrumentation. I.e.:

const String& key, const String& oldValue, const String& newValue, StorageType
storageType, SecurityOrigin* securityOrigin, Frame* sourceFrame.

Note that we identify storage by the origin and type anyways, so ideally, you
should not even need the Storage object. The code in the agent is a bit out of
date wrt origin managing, but it should handle it.