[webkit-reviews] review denied: [Bug 111300] Web Inspector: Developers should be able to do undo/redo changes to DOMStorage : [Attachment 193267] 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 193267: Patch
https://bugs.webkit.org/attachment.cgi?id=193267&action=review

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


>> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:102
>> +	    if (!m_sourceFrame->page())
> 
> Now that we listen for frameDetached event and do clear the history on that
event, should this be replaced with a simple ASSERT(m_sourceFrame->page())?
(Applicable to all such occurences below)

This approach is wrong. Essentially, you have reintroduced the action-frame
coupling I got rid of in the huge changeset that removed
InspectorDOMStorageResource. Actions are not bound to frames. Rather, they are
bound to SecurityOrigins, along with the StorageAreas. Once m_sourceFrame is
gone, you will not be able to do anything, yet the StorageArea and its
SecurityOrigin may still be there.