[webkit-reviews] review denied: [Bug 111300] Web Inspector: Developers should be able to do undo/redo changes to DOMStorage : [Attachment 191454] Patch

[bugzilla-daemon at webkit.org]

Alexander Pavlov (apavlov) <apavlov at chromium.org> has denied Vivek Galatage
<vivekg at webkit.org>'s request for review:
Bug 111300: Web Inspector: Developers should be able to do undo/redo changes to
DOMStorage
https://bugs.webkit.org/show_bug.cgi?id=111300

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

------- Additional Comments from Alexander Pavlov (apavlov)
<apavlov at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=191454&action=review


> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:97
> +	   m_value = m_storageArea->getItem(m_key, ec, m_sourceFrame.get());

What if the frame is no longer attached to the page?
Ditto for all the m_sourceFrame usages below.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:98
> +	   if (m_value.isNull())

Shouldn't we "do nothing" if this is the case? What if you undo a removal of an
item that hadn't existed?

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:106
> +	   return !hadException(ec);

Why not just "return !ec;" ?
Ditto for all other similar "return !hadException(ec)"

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:137
> +	   if (m_oldValue.isNull())

Shouldn't "undo()" remove an item if didn't exist before setItem()?

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:374
> +	   OwnPtr<InspectorHistory> history = adoptPtr(new InspectorHistory());


You can inline this

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:375
> +	   it = m_storageHistoryMap.set(storageArea,
history.release()).iterator;

Entries are never removed from the map, except for the page reload, when the
map is cleared. In addition to leaking InspectorHistory instances, you are
leaking StorageAreas, which are far more heavyweight. If you cannot implement
SecurityOrigin tracking in the backend (akin to what has been done in the
ResourceTreeModel), please at least use stringified storageIds as the map keys.


> Source/WebCore/inspector/InspectorDOMStorageAgent.h:88
> +

Extraneous blank line