Bug 111300 - Web Inspector: Support undo/redo of edits to local/session storage
Summary: Web Inspector: Support undo/redo of edits to local/session storage
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-04 03:05 PST by Vivek Galatage
Modified: 2016-12-13 15:40 PST (History)
12 users (show)

See Also:


Attachments
Patch (13.93 KB, patch)
2013-03-04 03:15 PST, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (13.92 KB, patch)
2013-03-04 03:22 PST, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (17.42 KB, patch)
2013-03-05 03:19 PST, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (34.50 KB, patch)
2013-03-15 03:02 PDT, Vivek Galatage
apavlov: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Vivek Galatage 2013-03-04 03:05:18 PST
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.
Comment 1 Vivek Galatage 2013-03-04 03:15:03 PST
Created attachment 191190 [details]
Patch
Comment 2 Vivek Galatage 2013-03-04 03:22:10 PST
Created attachment 191193 [details]
Patch
Comment 3 Vivek Galatage 2013-03-05 03:19:36 PST
Created attachment 191454 [details]
Patch
Comment 4 Vivek Galatage 2013-03-05 03:22:05 PST
WIP Patch without the test case. Adding the test case.
Comment 5 Alexander Pavlov (apavlov) 2013-03-05 04:07:36 PST
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
Comment 6 Vivek Galatage 2013-03-05 04:26:37 PST
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.
Comment 7 Vivek Galatage 2013-03-15 03:02:18 PDT
Created attachment 193267 [details]
Patch
Comment 8 Vivek Galatage 2013-03-15 03:16:31 PDT
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!
Comment 9 Alexander Pavlov (apavlov) 2013-03-18 02:53:16 PDT
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.
Comment 10 Radar WebKit Bug Importer 2014-12-17 11:20:44 PST
<rdar://problem/19281457>