Bug 111300 – Web Inspector: Support undo/redo of edits to local/session storage

• Status: NEW
• Component: Web Inspector

[Vivek Galatage]

Using Resources panel and the DOM Storage views, the developers can add/edit/delete from the view. So in case of accidental/intentional modifications of entries, the panel should be able to restore them back using undo/redo.

Patch follows.

[Vivek Galatage]

Created attachment 191190 [details]
Patch

[Vivek Galatage]

Created attachment 191193 [details]
Patch

[Vivek Galatage]

Created attachment 191454 [details]
Patch

[Vivek Galatage]

WIP Patch without the test case. Adding the test case.

[Alexander Pavlov (apavlov)]

Comment on attachment 191454 [details]
Patch

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

[Vivek Galatage]

Comment on attachment 191454 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191454&action=review

Thank you apavlov for your feedback.

>> 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.

I am afraid, I couldn't follow this one. Can you please explain a bit more?

>> 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?

Right!

>> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:106
>> +        return !hadException(ec);
> 
> Why not just "return !ec;" ?
> Ditto for all other similar "return !hadException(ec)"

argh... This is a dumb mistake from my side. :)

>> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:137
>> +        if (m_oldValue.isNull())
> 
> Shouldn't "undo()" remove an item if didn't exist before setItem()?

Right!

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

You mean something like: 

m_storageHistoryMap.set(storageArea, adoptPtr(new InspectorHistory()).release()).iterator;

>> 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.

I see... 

I thought as the StorageArea is a RefPtr<> in typedef HashMap<RefPtr<StorageArea>, OwnPtr<InspectorHistory> > StorageIdToHistoryMap; 

this wouldn't cause the memory leak for the storage area. InspectorHistory is also per storage area. I agree I should be calling the history->reset() to periodically reclaim the memory.

But as you pointed out, may be keeping the map of string storageId to history is better in any case. Thank you for pointing.

[Vivek Galatage]

Created attachment 193267 [details]
Patch

[Vivek Galatage]

Comment on attachment 193267 [details]
Patch

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)

> Source/WebCore/inspector/InspectorPageAgent.h:88
> +        virtual void frameDetached(Frame*) = 0;

I could think of only the frameDetached event at the moment as thats only required for DOM storage history cleanup. May be I am wrong!

[Alexander Pavlov (apavlov)]

Comment on attachment 193267 [details]
Patch

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.

[Radar WebKit Bug Importer]

<rdar://problem/19281457>