Bug 83012 - Web Inspector: localStorage items are not updated when the storage changes
Summary: Web Inspector: localStorage items are not updated when the storage changes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Vivek Galatage
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-03 02:45 PDT by Yury Semikhatsky
Modified: 2022-03-01 03:06 PST (History)
18 users (show)

See Also:


Attachments
Patch (68.59 KB, patch)
2012-04-25 13:19 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (68.57 KB, patch)
2012-04-25 13:35 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (68.79 KB, patch)
2012-04-25 23:51 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (10.72 KB, patch)
2012-04-26 10:55 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (16.79 KB, patch)
2012-05-10 12:37 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (16.29 KB, patch)
2012-05-10 13:19 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (710.65 KB, application/zip)
2012-05-11 03:58 PDT, WebKit Review Bot
no flags Details
Patch (19.94 KB, text/plain)
2012-05-14 01:28 PDT, Vivek Galatage
buildbot: commit-queue-
Details
Patch (19.92 KB, patch)
2012-05-14 01:59 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (755.37 KB, application/zip)
2012-05-14 02:35 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-04 (571.88 KB, application/zip)
2012-05-14 03:04 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (553.27 KB, application/zip)
2012-05-14 04:07 PDT, WebKit Review Bot
no flags Details
Patch (22.24 KB, patch)
2012-05-14 10:36 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (803.25 KB, application/zip)
2012-05-14 11:45 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (747.52 KB, application/zip)
2012-05-14 12:48 PDT, WebKit Review Bot
no flags Details
Patch (22.11 KB, patch)
2012-05-15 05:03 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (788.89 KB, application/zip)
2012-05-15 06:49 PDT, WebKit Review Bot
no flags Details
Patch (22.98 KB, patch)
2012-05-15 10:18 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (27.29 KB, patch)
2012-05-16 11:21 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (27.50 KB, patch)
2012-05-16 11:49 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (538.14 KB, application/zip)
2012-05-16 13:48 PDT, WebKit Review Bot
no flags Details
Patch (27.94 KB, patch)
2012-05-17 11:07 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (774.44 KB, application/zip)
2012-05-17 12:16 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (549.01 KB, application/zip)
2012-05-17 13:19 PDT, WebKit Review Bot
no flags Details
Patch (27.83 KB, patch)
2012-05-18 06:06 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (553.42 KB, application/zip)
2012-05-18 07:30 PDT, WebKit Review Bot
no flags Details
Archive of layout-test-results from ec2-cr-linux-02 (606.01 KB, application/zip)
2012-05-18 08:24 PDT, WebKit Review Bot
no flags Details
Patch (27.90 KB, patch)
2012-05-21 06:33 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (611.63 KB, application/zip)
2012-05-21 07:41 PDT, WebKit Review Bot
no flags Details
Patch (27.95 KB, patch)
2012-05-24 01:40 PDT, Vivek Galatage
no flags Details | Formatted Diff | Diff
Patch (28.12 KB, patch)
2012-05-24 01:57 PDT, Vivek Galatage
pfeldman: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yury Semikhatsky 2012-04-03 02:45:46 PDT
1. Go to http://people.w3.org/mike/localstorage.html
2. Open inspector and select local storage in Resources panel
3. Add a new item using the editor in the page

Result:
Local storage element in the inspector is not updated. It will update if you switch to another panel and back.
Comment 1 Vivek Galatage 2012-04-05 04:35:35 PDT
After analyzing the scenario, I have arrived at two points where this needs to be fixed

1. The front-end inspector code should listen on window.addEventListener("storage") so that any updates in the storage will be notified to the inspector.

2. The backend part: Currently the StorageEventDispatcher takes the responsibility of dispatching the storage related events to the related pages grouped in PageGroup object.

So in the case when I open two different tabs for the same URL as demoed in this http://html5demos.com/storage-events, these two different pages are grouped under the same group. So the statement in StorageEventDispatcher::dispatch()

for (unsigned i = 0; i < frames.size(); ++i) {
    ExceptionCode ec = 0;
    Storage* storage = frames[i]->domWindow()->localStorage(ec);
    if (!ec)
        frames[i]->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, sourceFrame->document()->url(), storage));
}

takes care of dispatching the storage event to the other page.

But the same is not applicable with the inspector page. Inspector page is not in the same group as of the page being inspected.

So now we have to either change the inspector protocol scheme to introduce a new method in the inspector.json to support custom type of event for storage events and propagate them same as being currently done. 

The other option could be bringing the inspector page also under the same group as that of the page being inspected.

But for the second option I am not aware of any security concerns of having main page and the inspector page in the same group. Please comment if know the rational behind separating the Inspector page from the main page group.

Also lets say the main page is listening on the events such as storage (as done in http://html5demos.com/storage-events), then if I go ahead and add some new property:value pair in the inspector->resources->local storage view, the main page is not getting notified again because of PageGroup differences.

I am working on these issues for further resolution and will update soon with the patch.

Thank you,
Vivek
Comment 2 Yury Semikhatsky 2012-04-05 05:09:55 PDT
(In reply to comment #1)
> After analyzing the scenario, I have arrived at two points where this needs to be fixed
> 
> 1. The front-end inspector code should listen on window.addEventListener("storage") so that any updates in the storage will be notified to the inspector.
> 

InspectorDOMStorageResource::startReportingChangesToFrontend should start listening localStorage on the inspected page side, see http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/inspector/InspectorDOMStorageResource.cpp&exact_package=chromium&q=StorageResource&type=cs&l=98 But for some reason that event handler is never invoked.
Comment 3 Timothy Hatcher 2012-04-05 07:11:05 PDT
(In reply to comment #1)
> The other option could be bringing the inspector page also under the same group as that of the page being inspected.
> 
> But for the second option I am not aware of any security concerns of having main page and the inspector page in the same group. Please comment if know the rational behind separating the Inspector page from the main page group.

Option 2 is a non-option when it comes to remote device debugging. The protocol needs to carry this info, plain and simple. Ignore the fact that the Inspector is written in HTML.
Comment 4 Vivek Galatage 2012-04-05 22:06:40 PDT
Yury and Timothy, thank you for your comments.

Regarding the scenario which I mentioned in the earlier comment about the main page listening on "storage" event. The same is done with http://html5demos.com/storage-events page, so addition of a new key-value pair or modifications done using inspector also must also be propagated to the main page isn't it?

So in the page mentioned above, its not receiving the storage events when changes are done by inspector resource view.
Comment 5 Vivek Galatage 2012-04-09 02:09:04 PDT
Adding to further analysis, the method InspectorDOMStorageResource::startReportingChangesToFrontend() adds itself as Storage event listener on the page being inspected (m_frame). 

So now when the page being inspected(m_frame) adds the new key value pair in the localStorage, the StorageEventDispatcher::dispatch() method is invoked. But now here this method filters out the page who added this change(in this scenario the m_frame itself) from sending any notifications (ref: http://dev.w3.org/html5/webstorage/#dom-localstorage)  by following lines - 

        // Send events to every page.
        const HashSet<Page*>& pages = page->group().pages();
        HashSet<Page*>::const_iterator end = pages.end();
        for (HashSet<Page*>::const_iterator it = pages.begin(); it != end; ++it) {
            for (Frame* frame = (*it)->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
===============>if (sourceFrame != frame && frame->document()->securityOrigin()->equal(securityOrigin))
                    frames.append(frame);
            }
        }

So here in this case, sourceFrame will always be the frame so this frame would never get any such notifications.

Hence InspectorDOMStorageResource::handleEvent() is not triggered.

Moving further, for testing purposes I explicitly removed this check and made sure the InspectorDOMStorageResource::handleEvent() is invoked.

So in the InspectorDOMStorageResource::handleEvent() method, there is a check if the event hasInterface("StorageEvent") which evaluates to true so the handleEvent returns from there itself.

So again for testing purposes, when I bypassed this check, the DOMStorage.js (from inspector/front-end) file's method updateDOMStorage() is invoked and it basically updates the view correctly.

So what would be your comments on how do we proceed on this. I think we need to change the inspector's behavior on how it listens to the storageEvent as the inspector page is not in the same PageGroup as the page being inspected.
Comment 6 Vivek Galatage 2012-04-15 23:28:49 PDT
Yury, Timothy, Pavel: Any comments?
Comment 7 Vivek Galatage 2012-04-24 04:47:33 PDT
Based on the discussion with Pavel, I am planning to implement the changes as depicted in the diagrams below.

Use Case 1: Inspected page adding/removing/clearing storageArea items
===========
Here in this case the originatingPage would be Page 3, hence the StorageEventDispatcher would send the notifications to other pages with the same domain( new tab, new window etc. )

Also it will instruct InspectorInstrumention via didDispatchDOMStorageEvent method to propagate storage event to inspectorFrontend resulting in update of the localStorage entries in resource view.

        +-------------------------[PageGroup 1]--+
        |                                        |
        | +--------+    +--------+    +--------+ |
        | | Page 1 |    | Page 2 |    | Page 3 |----+-+-+
        | +--------+    +--------+    +--------+ |  | | |
        +------\-------------\-------------------+  | | |
                \             \                     | | |
                 \             \                    | | |
                  \             \                   | | |                     +--------[StorageArea]--+
                   \             \                  | | +-> setItem()    O----|           Key-value   |
                    \             \                 | +---> removeItem() O----|         +----+------+ |
                     \             \                +-----> clear()      O----|         +----+------+ |
                   [storage Event Notification]                               |         +----+------+ |
                       \             \                                        |         +----+------+ |
                        \             \                                       +-----------+-----------+
                         \             \                                                |
                          \             \                                               | dispatch()
                           \             \                                              V
                            \             \                                 +------------------------+
                             +------<<-----+-------<<-----------------------| StorageEventDispatcher |
                                                                            +-----------+------------+
                                                                                        |
                                                                                        | didDispatchDOMStorageEvent( originatingPage )
                                                                                        V
                                                                            +--------------------------+
                                                                            | InspectorInstrumentation |
                                                                            +-----------+--------------+
                                                                                        |
                                                                                        | didDispatchDOMStorageEvent( originatingPage )
                                       if( originatingPage != inspectorPage )           V
                                                                            +--------------------------+
                                              +----<<-----------------------| InspectorDOMStorageAgent | 
                                             /                              +--------------------------+
                                            /                               
                                           /  updateDOMStorage()                            
        +----[PageGroup 2]--+             /                                             
        |                   |            /                              
        | +---------------+ |           /
        | | InspectorPage |------------+
        | +---------------+ |
        +-------------------+
        
        
Use Case 2: Inspector page adding/removing/clearing storageArea items
===========        
This is the case where the Inspector itself is trying to add/delete/clear localStorage entries.

So now StorageEventDispatcher will do nothing as there are no other pages in the PageGroup2. So it will invoke didDispatchDOMStorageEvent() on InspectorInstrumentation which in turn will invoke InspectorDOMStorageAgent. Here the check will be made if the originatingPage is the same inspectorPage. So as in this case it evaluates to true, the agent will grab the page being inspected and its page group. From this page group, it will notify all the pages.

                             +----[PageGroup 2]--+
                             |                   |
                             | +---------------+ |
                             | | InspectorPage |----+-+-+
                             | +---------------+ |  | | |
                             +-------------------+  | | |
                                                    | | |
                                                    | | |
                                                    | | |                     +--------[StorageArea]--+
                                                    | | +-> setItem()    O----|           Key-value   |
                                                    | +---> removeItem() O----|         +----+------+ |
                                                    +-----> clear()      O----|         +----+------+ |
                                                                              |         +----+------+ |
                                                                              |         +----+------+ |
                                                                              +-----------+-----------+
                                                                                        |
                                                                                        | dispatch()
                                                                                        V
                                                                            +------------------------+
                                                                            | StorageEventDispatcher |
                                                                            +-----------+------------+
                                                                                        |
                                                                                        | didDispatchDOMStorageEvent( originatingPage )
                                                                                        V
                                                                            +--------------------------+
                                                                            | InspectorInstrumentation |
                                                                            +-----------+--------------+
                                                                                        |
                                                                                        | didDispatchDOMStorageEvent( originatingPage )
                                         if( originatingPage == inspectorPage )         V 
                                                                            +--------------------------+
                                                            +---------------| InspectorDOMStorageAgent |
                                                           /                +--------------------------+
                                                          /                 
                                                         /
                       [storage Event Notification]     /
            +-----<<-----+------<<-----+----<<---------+
           /            /             /                 
          /            /             /
 +-------/------------/-[PageGroup1]/-----+ 
 |      /            /             /      |
 | +--------+    +--------+    +--------+ |
 | | Page 1 |    | Page 2 |    | Page 3 | |
 | +--------+    +--------+    +--------+ | 
 +----------------------------------------+ 


So please let me know if my approach is correct.
Comment 8 Vivek Galatage 2012-04-25 13:19:53 PDT
Created attachment 138865 [details]
Patch
Comment 9 Early Warning System Bot 2012-04-25 13:26:01 PDT
Comment on attachment 138865 [details]
Patch

Attachment 138865 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12526682
Comment 10 Early Warning System Bot 2012-04-25 13:26:34 PDT
Comment on attachment 138865 [details]
Patch

Attachment 138865 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/12529640
Comment 11 Build Bot 2012-04-25 13:31:46 PDT
Comment on attachment 138865 [details]
Patch

Attachment 138865 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12522579
Comment 12 Vivek Galatage 2012-04-25 13:35:09 PDT
Created attachment 138870 [details]
Patch
Comment 13 WebKit Review Bot 2012-04-25 19:11:18 PDT
Comment on attachment 138870 [details]
Patch

Attachment 138870 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12522637
Comment 14 Vivek Galatage 2012-04-25 23:51:41 PDT
Created attachment 138942 [details]
Patch
Comment 15 Vivek Galatage 2012-04-26 10:55:36 PDT
Created attachment 139027 [details]
Patch
Comment 16 Konrad Piascik 2012-04-27 05:43:04 PDT
> Source/WebCore/storage/StorageEventDispatcher.cpp:66
> +#endif // ENABLE(INSPECTOR)

This #if ENABLE guard is not needed, it already exists in InspectorInstrumentation.

> Source/WebCore/storage/StorageEventDispatcher.cpp:89
> +#endif // ENABLE(INSPECTOR)

ditto

> Source/WebCore/storage/StorageEventDispatcher.cpp:100
> +#endif // ENABLE(INSPECTOR)

ditto
Comment 17 Pavel Feldman 2012-05-02 06:05:27 PDT
Comment on attachment 139027 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        No new tests needed.

I think you need a test with this change.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:57
> +class StorageEventNotifier {

You probably want to declare it in anonymous namespace.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:65
> +    InspectorFrontend* m_frontend;

You should store InspectorFrontend::DOMStorage pointer here.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:79
> +    if (!m_timer.isActive())

You only invoke this right after the construction, no need to check for active timer.

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

No need for this check since you never clean it.

> Source/WebCore/storage/StorageEventDispatcher.cpp:62
> +#if ENABLE(INSPECTOR)

No need for ENABLE(INSPECTOR) when working with InspectorInstrumentation::

> Source/WebCore/storage/StorageEventDispatcher.cpp:63
> +                // This is to ensure all the other open inspector pages in the

Nuke the comment.

> Source/WebCore/storage/StorageEventDispatcher.cpp:65
> +                InspectorInstrumentation::didDispatchDOMStorageEvent(frames[i]->page(), storage, frames[i].get(), key, oldValue, newValue);

Inspector agents are stored in inspector controller, on the per-page basis. You don't need this to be in the for loop, just invoke it once for the sourceFrame's page:
InspectorInstrumentation::didDispatchDOMStorageEvent(page, ...).

> Source/WebCore/storage/StorageEventDispatcher.cpp:85
> +#if ENABLE(INSPECTOR)

ditto

> Source/WebCore/storage/StorageEventDispatcher.cpp:99
> +        InspectorInstrumentation::didDispatchDOMStorageEvent(page, storage, sourceFrame, key, oldValue, newValue);

So this is essentially the only instrumentation you need.
Comment 18 Vivek Galatage 2012-05-02 07:01:29 PDT
(In reply to comment #17)

Thank you Pavel for the review. I agree to all of your review comments but have a doubt regarding Instrumenting the storage event per frame inside the for loop.

If I open the same URL(http://people.w3.org/mike/localstorage.html) in multiple tabs (lets say 3 tabs), each of them will be having a different Inspector page (and inspector controller) and its own resource view for localStorage/sessionStorage.

Lets assume that the tab3 makes some changes in the storage(add/delete/update). So in order for all these inspector pages to update their respective storage views, the notification must be sent per frame. Hence I have put this instrumenting code in the for loop both for local and session storage. 
 
Please correct me if my understanding is wrong.

> (From update of attachment 139027 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=139027&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        No new tests needed.
> 
> I think you need a test with this change.
> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:57
> > +class StorageEventNotifier {
> 
> You probably want to declare it in anonymous namespace.
> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:65
> > +    InspectorFrontend* m_frontend;
> 
> You should store InspectorFrontend::DOMStorage pointer here.
> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:79
> > +    if (!m_timer.isActive())
> 
> You only invoke this right after the construction, no need to check for active timer.
> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:85
> > +    if (m_frontend)
> 
> No need for this check since you never clean it.
> 
> > Source/WebCore/storage/StorageEventDispatcher.cpp:62
> > +#if ENABLE(INSPECTOR)
> 
> No need for ENABLE(INSPECTOR) when working with InspectorInstrumentation::
> 
> > Source/WebCore/storage/StorageEventDispatcher.cpp:63
> > +                // This is to ensure all the other open inspector pages in the
> 
> Nuke the comment.
> 
> > Source/WebCore/storage/StorageEventDispatcher.cpp:65
> > +                InspectorInstrumentation::didDispatchDOMStorageEvent(frames[i]->page(), storage, frames[i].get(), key, oldValue, newValue);
> 
> Inspector agents are stored in inspector controller, on the per-page basis. You don't need this to be in the for loop, just invoke it once for the sourceFrame's page:
> InspectorInstrumentation::didDispatchDOMStorageEvent(page, ...).
> 
> > Source/WebCore/storage/StorageEventDispatcher.cpp:85
> > +#if ENABLE(INSPECTOR)
> 
> ditto
> 
> > Source/WebCore/storage/StorageEventDispatcher.cpp:99
> > +        InspectorInstrumentation::didDispatchDOMStorageEvent(page, storage, sourceFrame, key, oldValue, newValue);
> 
> So this is essentially the only instrumentation you need.
Comment 19 Pavel Feldman 2012-05-02 07:32:11 PDT
> Please correct me if my understanding is wrong.

Here is the way you get the list of frames in StorageEventDispatcher:

Page* page = sourceFrame->page();
for (Frame* frame = page->mainFrame(); frame; frame = frame->tree()->traverseNext()) {
    frames.append(frame);
}

This means that all the frames are from the same page (i.e. from the same "tab3").
Comment 20 Vivek Galatage 2012-05-10 12:37:59 PDT
Created attachment 141226 [details]
Patch
Comment 21 Vivek Galatage 2012-05-10 13:19:39 PDT
Created attachment 141236 [details]
Patch
Comment 22 WebKit Review Bot 2012-05-11 03:58:37 PDT
Comment on attachment 141236 [details]
Patch

Attachment 141236 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12671310

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 23 WebKit Review Bot 2012-05-11 03:58:42 PDT
Created attachment 141375 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 24 Vivek Galatage 2012-05-11 07:03:08 PDT
(In reply to comment #22)
> (From update of attachment 141236 [details])
> Attachment 141236 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/12671310
> 
> New failing tests:
> inspector/storage-panel-dom-storage-update.html

The compilation of StorageEventDispatcher.cpp not seen in the build log. As this is the place where instrumentation happens, and its not included, the test is failing on cr linux. So is there any issue with cr linux bot while taking the patch?
Comment 25 Pavel Feldman 2012-05-11 09:40:23 PDT
Comment on attachment 141236 [details]
Patch

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.
Comment 26 Vivek Galatage 2012-05-11 22:48:10 PDT
(In reply to comment #25)

Thank you Pavel for your feedback.

> (From update of attachment 141236 [details])
> 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?

I followed the way InspectorDOMAgent notifies the frontend. In that it uses the timer to notify asynchronously. Hence applied same logic here.

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

Agreed. Missed this one. Thank you.

> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:226
> > +    if (m_frontend) {
> 
> We usually do
> if (!m_frontend)
>     return;
> 
> in top level guards.

Agreed. 

> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:230
> > +            m_notifier->notify();
> 
> Call it from the constructor ?
>

I thought having an explicit call to notify() would look more intuitive. Your thoughts?
 
> > 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.

Agreed.

I will apply these changes and will upload a new patch. Thank you.
Comment 27 Pavel Feldman 2012-05-11 23:33:43 PDT
> I followed the way InspectorDOMAgent notifies the frontend. In that it uses the timer to notify asynchronously. Hence applied same logic here.
> 

DOM agent does this for performance reasons. It coalesces events so that it does not have to issue a signal per modification. It rather collects them into sets and sends them in chunks once in a while. I don't think you need it. Simple plain m_frontend call would be Ok. I was under the impression you were coalescing them earlier.

One more important note: you should only issue modification events to the front-end when the agent is enabled (InspectorDOMStorageAgent::m_enabled).
Comment 28 Vivek Galatage 2012-05-14 01:28:55 PDT
Created attachment 141664 [details]
Patch
Comment 29 Build Bot 2012-05-14 01:48:59 PDT
Comment on attachment 141664 [details]
Patch

Attachment 141664 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12679439
Comment 30 Vivek Galatage 2012-05-14 01:59:50 PDT
Created attachment 141666 [details]
Patch
Comment 31 WebKit Review Bot 2012-05-14 02:35:09 PDT
Comment on attachment 141664 [details]
Patch

Attachment 141664 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12685280

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 32 WebKit Review Bot 2012-05-14 02:35:14 PDT
Created attachment 141672 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 33 WebKit Review Bot 2012-05-14 03:04:28 PDT
Comment on attachment 141666 [details]
Patch

Attachment 141666 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12683320

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 34 WebKit Review Bot 2012-05-14 03:04:35 PDT
Created attachment 141675 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 35 WebKit Review Bot 2012-05-14 04:07:25 PDT
Comment on attachment 141666 [details]
Patch

Attachment 141666 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12694157

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 36 WebKit Review Bot 2012-05-14 04:07:31 PDT
Created attachment 141687 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 37 Pavel Feldman 2012-05-14 08:29:19 PDT
Comment on attachment 141666 [details]
Patch

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

Looks good. Couple of comments inline.

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:194
> +    if (!m_frontend && !m_enabled)

&& -> || ?

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:206
> +    m_frontend->domstorage()->updateDOMStorage(id);

Following naming pattern should be used for the protocol methods: "domStorageUpdated"

> Source/WebCore/inspector/InspectorInstrumentation.cpp:921
> +    if (!inspectorAgent || !inspectorAgent->developerExtrasEnabled())

no need to check for developer extras here.

> Source/WebCore/inspector/InspectorInstrumentation.h:1161
> +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForPage(sourceFrame->page()))

Should have
FAST_RETURN_IF_NO_FRONTENDS(void());

> LayoutTests/inspector/storage-panel-dom-storage-update.html:30
> +    function dumpDataGrid() {

Here and below: You should keep { on the next line.

> LayoutTests/inspector/storage-panel-dom-storage-update.html:96
> +            setTimeout(viewUpdatedAfterRemoval, 0 );       

setTimeout will not necessarily do what you expect. Running after pending dispatches should be more than enough. Also, you should only use it when absolutely necessary. It is often more reliable to do InspectorTest.addSniffer and sniff for the UI update you are interested in.

> LayoutTests/inspector/storage-panel-dom-storage-update.html:119
> +    InspectorTest.runTestSuite([

We always declare these functions inline.
Comment 38 Vivek Galatage 2012-05-14 08:52:11 PDT
(In reply to comment #37)
> (From update of attachment 141666 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141666&action=review
> 
> Looks good. Couple of comments inline.
> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:194
> > +    if (!m_frontend && !m_enabled)
> 
> && -> || ?
> 

Ah.. This is a blunder. I will correct this one. Thank you.

> > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:206
> > +    m_frontend->domstorage()->updateDOMStorage(id);
> 
> Following naming pattern should be used for the protocol methods: "domStorageUpdated"
> 

I think I am not clear on this one. Can you please provide more details on how to do this?

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:921
> > +    if (!inspectorAgent || !inspectorAgent->developerExtrasEnabled())
> 
> no need to check for developer extras here.
> 

Agreed.

> > Source/WebCore/inspector/InspectorInstrumentation.h:1161
> > +    if (InstrumentingAgents* instrumentingAgents = instrumentingAgentsForPage(sourceFrame->page()))
> 

Removed the check.

> Should have
> FAST_RETURN_IF_NO_FRONTENDS(void());
> 

Added the line.

> > LayoutTests/inspector/storage-panel-dom-storage-update.html:30
> > +    function dumpDataGrid() {
> 
> Here and below: You should keep { on the next line.
> 

Agreed.

> > LayoutTests/inspector/storage-panel-dom-storage-update.html:96
> > +            setTimeout(viewUpdatedAfterRemoval, 0 );       
> 
> setTimeout will not necessarily do what you expect. Running after pending dispatches should be more than enough. Also, you should only use it when absolutely necessary. It is often more reliable to do InspectorTest.addSniffer and sniff for the UI update you are interested in.
> 

This is no longer needed as the update is sent synchronously. Will remove this.

> > LayoutTests/inspector/storage-panel-dom-storage-update.html:119
> > +    InspectorTest.runTestSuite([
> 
> We always declare these functions inline.

The reason why I have not put these as inline is because I repeatedly call the same functions i.e. addTest, removeTest etc. Hence I thought separating them and copying its references in the array as many times as I would like to call them by the test case will be better. So shall I retain it this way or shall I make them inline?
Comment 39 Pavel Feldman 2012-05-14 09:01:41 PDT
> > > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:206
> > > +    m_frontend->domstorage()->updateDOMStorage(id);
> > 
> > Following naming pattern should be used for the protocol methods: "domStorageUpdated"
> > 
> 
> I think I am not clear on this one. Can you please provide more details on how to do this?

In the protocol method naming, we start with the subject and follow with the action type. Such as "responseReceived", "storageUpdated", "codeReviewed", etc.

> 
> > > LayoutTests/inspector/storage-panel-dom-storage-update.html:119
> > > +    InspectorTest.runTestSuite([
> > 
> > We always declare these functions inline.
> 
> The reason why I have not put these as inline is because I repeatedly call the same functions i.e. addTest, removeTest etc. Hence I thought separating them and copying its references in the array as many times as I would like to call them by the test case will be better. So shall I retain it this way or shall I make them inline?

Oh, I did not realize you were doing that. No, this is not good. Test cases are for hand-crafted named test functions only. If you test is about adding items, there should be single testAddItem that would call its inner addItem function as many times as you'd like.
Comment 40 Vivek Galatage 2012-05-14 10:36:25 PDT
Created attachment 141753 [details]
Patch
Comment 41 WebKit Review Bot 2012-05-14 11:45:09 PDT
Comment on attachment 141753 [details]
Patch

Attachment 141753 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12684447

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 42 WebKit Review Bot 2012-05-14 11:45:16 PDT
Created attachment 141762 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 43 WebKit Review Bot 2012-05-14 12:48:34 PDT
Comment on attachment 141753 [details]
Patch

Attachment 141753 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12684468

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 44 WebKit Review Bot 2012-05-14 12:48:39 PDT
Created attachment 141772 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 45 Pavel Feldman 2012-05-15 04:13:13 PDT
Comment on attachment 141753 [details]
Patch

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

Looks good. Please fix the comments before landing.

> Source/WebCore/inspector/InspectorDOMStorageResource.h:46
> +    class InspectorDOMStorageResource : public RefCounted<InspectorDOMStorageResource> {

nit: I don't think it needs to be ref counted any longer.

> Source/WebCore/inspector/InspectorInstrumentation.cpp:920
> +    InspectorAgent* inspectorAgent = instrumentingAgents->inspectorAgent();

Lines 920-922 are not necessary.

> LayoutTests/inspector/storage-panel-dom-storage-update.html:64
> +            for( var i = 0; i < storages.length; i++ ) {

Here and below: no need for spaces before var and after i++. Also there should be a space after for.
Also, we also do ++i in most cases.
Comment 46 Vivek Galatage 2012-05-15 04:50:59 PDT
(In reply to comment #45)
> (From update of attachment 141753 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=141753&action=review
> 
> Looks good. Please fix the comments before landing.
> 
> > Source/WebCore/inspector/InspectorDOMStorageResource.h:46
> > +    class InspectorDOMStorageResource : public RefCounted<InspectorDOMStorageResource> {
> 
> nit: I don't think it needs to be ref counted any longer.
> 

As discussed, we will take this up in another bug. Thank you for pointing this out.

> > Source/WebCore/inspector/InspectorInstrumentation.cpp:920
> > +    InspectorAgent* inspectorAgent = instrumentingAgents->inspectorAgent();
> 
> Lines 920-922 are not necessary.
> 

Corrected.

> > LayoutTests/inspector/storage-panel-dom-storage-update.html:64
> > +            for( var i = 0; i < storages.length; i++ ) {
> 
> Here and below: no need for spaces before var and after i++. Also there should be a space after for.
> Also, we also do ++i in most cases.

Corrected.

Patch on the way
Comment 47 Vivek Galatage 2012-05-15 05:03:00 PDT
Created attachment 141923 [details]
Patch
Comment 48 WebKit Review Bot 2012-05-15 06:48:58 PDT
Comment on attachment 141923 [details]
Patch

Attachment 141923 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12679937

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 49 WebKit Review Bot 2012-05-15 06:49:04 PDT
Created attachment 141955 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 50 Pavel Feldman 2012-05-15 08:34:34 PDT
Comment on attachment 141923 [details]
Patch

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

> LayoutTests/inspector/storage-panel-dom-storage-update.html:66
> +                if( storage.isLocalStorage ) {                

Still wrong formatting here.

> LayoutTests/inspector/storage-panel-dom-storage-update.html:81
> +                    if( count < 6 )

ditto

> LayoutTests/inspector/storage-panel-dom-storage-update.html:102
> +                InspectorTest.runAfterPendingDispatches( function() {

ditto

> LayoutTests/inspector/storage-panel-dom-storage-update.html:105
> +                    if( count > 4 )

ditto

> LayoutTests/inspector/storage-panel-dom-storage-update.html:126
> +                InspectorTest.runAfterPendingDispatches( function() {

ditto
Comment 51 Pavel Feldman 2012-05-15 08:35:13 PDT
Here is the actual result on Chromium (it looks wrong):

Test that storage panel is present and that it contains correct data whenever localStorage is updated.


Running: initialize
Initialized localStorage by clearing entries

Running: updateLocalStorageView
Resource Panel with localStorage view updated

Running: addItemTest
addItem('a1','=value1')
KeyValue pairs: []
addItem('a2','=value2')
KeyValue pairs: []
addItem('a3','=value3')
KeyValue pairs: []
addItem('a4','=value4')
KeyValue pairs: []
addItem('a5','=value5')
KeyValue pairs: []

Running: removeItemTest
removeItem('a5')
KeyValue pairs: []
removeItem('a4')
KeyValue pairs: []

Running: clearTest
clear()
KeyValue pairs: []
Comment 52 Pavel Feldman 2012-05-15 08:49:37 PDT
Here is the patch that fixes it for Chromium. Note that is kindof suggests that Instrumentation should accept Page, not Frame as a parameter.

index 00cc443..88c2069 100644
--- a/Source/WebKit/chromium/src/StorageAreaProxy.cpp
+++ b/Source/WebKit/chromium/src/StorageAreaProxy.cpp
@@ -32,6 +32,7 @@
 #include "EventNames.h"
 #include "ExceptionCode.h"
 #include "Frame.h"
+#include "InspectorInstrumentation.h"
 #include "Page.h"
 #include "PageGroup.h"
 #include "SecurityOrigin.h"
@@ -133,6 +134,7 @@ void StorageAreaProxy::dispatchLocalStorageEvent(PageGroup* pageGroup, const Str
                     frame->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));
             }
         }
+        InspectorInstrumentation::didDispatchDOMStorageEvent(key, oldValue, newValue, LocalStorage, securityOrigin, (*it)->mainFrame());
     }
 }
 
@@ -164,6 +166,7 @@ void StorageAreaProxy::dispatchSessionStorageEvent(PageGroup* pageGroup, const S
             if (!ec)
                 frame->document()->enqueueWindowEvent(StorageEvent::create(eventNames().storageEvent, key, oldValue, newValue, pageURL, storage));
         }
+        InspectorInstrumentation::didDispatchDOMStorageEvent(key, oldValue, newValue, SessionStorage, securityOrigin, page->mainFrame());
     }
 }
Comment 53 Vivek Galatage 2012-05-15 10:18:24 PDT
Created attachment 141993 [details]
Patch
Comment 54 Pavel Feldman 2012-05-15 11:22:36 PDT
Comment on attachment 141993 [details]
Patch

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

> Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:199
> +        id = storageId(sourceFrame->domWindow()->sessionStorage(ec));

See my comment below first. You would need to change InspectorDOMStorageResource::isSameHostAndType to accept SecurityOrigin to fix it.

> Source/WebCore/inspector/InspectorInstrumentation.h:1163
> +        didDispatchDOMStorageEventImpl(instrumentingAgents, key, oldValue, newValue, storageType, securityOrigin, page->mainFrame());

I probably made your life a bit harder with the additional Chromium code and migration to the Page* in instrumentation. Sorry about that. Imagine that there is an iframe from domain "foo" that is dispatching this event. Main frame is from "bar". We should update local storage for "foo", but as written in this code, we will update the "bar". Thing is that SecurityOrigin defines the local storage you'd like to update.
Comment 55 Vivek Galatage 2012-05-16 11:21:54 PDT
Created attachment 142307 [details]
Patch
Comment 56 Vivek Galatage 2012-05-16 11:49:00 PDT
Created attachment 142314 [details]
Patch
Comment 57 WebKit Review Bot 2012-05-16 13:47:58 PDT
Comment on attachment 142314 [details]
Patch

Attachment 142314 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12709535

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 58 WebKit Review Bot 2012-05-16 13:48:04 PDT
Created attachment 142338 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 59 Pavel Feldman 2012-05-17 07:43:40 PDT
Comment on attachment 142314 [details]
Patch

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

Some dead code to remove and it is good to go.

> Source/WebCore/inspector/InspectorDOMStorageAgent.h:74
> +    String storageIdForSecurityOrigin(SecurityOrigin*, bool isLocalStorage);

Rename to storageId ?

> Source/WebCore/inspector/InspectorDOMStorageResource.h:57
> +    bool isSameHostAndType(Frame*, bool isLocalStorage) const;

This is now not used.

> Source/WebCore/inspector/InspectorDOMStorageResource.h:58
> +    bool isSameHostAndTypeForSecurityOrigin(SecurityOrigin*, bool isLocalStorage) const;

Nit: this can now have a better name (isSameHostAndType).

> Source/WebCore/inspector/InspectorDOMStorageResource.h:61
> +    Frame* frame() const { return m_frame.get(); }

You don't need this now.

> Source/WebCore/inspector/InspectorDOMStorageResource.h:69
> +    RefPtr<Frame> m_frame;

You don't need this now.
Comment 60 Vivek Galatage 2012-05-17 11:07:08 PDT
Created attachment 142506 [details]
Patch
Comment 61 Vivek Galatage 2012-05-17 11:12:05 PDT
(In reply to comment #59)
> (From update of attachment 142314 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=142314&action=review
> 
> Some dead code to remove and it is good to go.
> 
> > Source/WebCore/inspector/InspectorDOMStorageAgent.h:74
> > +    String storageIdForSecurityOrigin(SecurityOrigin*, bool isLocalStorage);
> 
> Rename to storageId ?
> 

This is used by injected script code as well as all set, get methods. 

> > Source/WebCore/inspector/InspectorDOMStorageResource.h:57
> > +    bool isSameHostAndType(Frame*, bool isLocalStorage) const;
> 
> This is now not used.

Done. Changed to receive the SecurityOrigin as argument instead of frame.

> 
> > Source/WebCore/inspector/InspectorDOMStorageResource.h:58
> > +    bool isSameHostAndTypeForSecurityOrigin(SecurityOrigin*, bool isLocalStorage) const;
> 
> Nit: this can now have a better name (isSameHostAndType).
> 

Done.

> > Source/WebCore/inspector/InspectorDOMStorageResource.h:61
> > +    Frame* frame() const { return m_frame.get(); }
> 
> You don't need this now.
>

Usage found hence not removing.
 
> > Source/WebCore/inspector/InspectorDOMStorageResource.h:69
> > +    RefPtr<Frame> m_frame;
> 
> You don't need this now.

Usage found hence not removing.
Comment 62 WebKit Review Bot 2012-05-17 12:16:48 PDT
Comment on attachment 142506 [details]
Patch

Attachment 142506 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12722531

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 63 WebKit Review Bot 2012-05-17 12:16:56 PDT
Created attachment 142525 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 64 WebKit Review Bot 2012-05-17 13:19:49 PDT
Comment on attachment 142506 [details]
Patch

Attachment 142506 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12719594

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 65 WebKit Review Bot 2012-05-17 13:19:55 PDT
Created attachment 142536 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 66 Vivek Galatage 2012-05-18 06:06:19 PDT
Created attachment 142699 [details]
Patch
Comment 67 WebKit Review Bot 2012-05-18 07:30:47 PDT
Comment on attachment 142699 [details]
Patch

Attachment 142699 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12722812

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 68 WebKit Review Bot 2012-05-18 07:30:53 PDT
Created attachment 142715 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 69 WebKit Review Bot 2012-05-18 08:24:38 PDT
Comment on attachment 142699 [details]
Patch

Attachment 142699 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12729102

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 70 WebKit Review Bot 2012-05-18 08:24:44 PDT
Created attachment 142723 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 71 Vivek Galatage 2012-05-21 06:33:57 PDT
Created attachment 143019 [details]
Patch
Comment 72 WebKit Review Bot 2012-05-21 07:41:02 PDT
Comment on attachment 143019 [details]
Patch

Attachment 143019 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12734628

New failing tests:
inspector/storage-panel-dom-storage-update.html
Comment 73 WebKit Review Bot 2012-05-21 07:41:08 PDT
Created attachment 143031 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 74 jochen 2012-05-23 00:28:10 PDT
Comment on attachment 143019 [details]
Patch

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

> Source/WebKit/chromium/src/StorageAreaProxy.cpp:206
> +        InspectorInstrumentation::didDispatchDOMStorageEvent(key, oldValue, newValue, LocalStorage, securityOrigin, *it);

this code path is currently not going to be invoked, and in the future will only be invoked if the storage area was changed in another process
Comment 75 jochen 2012-05-23 00:31:11 PDT
Adding Michael.

Michael, can you have a look at the latest patch, esp. StorageAreaProxy::dispatchLocalStorageEvent and advise where the call to the inspector should be put instead?
Comment 76 Michael Nordman 2012-05-23 11:55:40 PDT
Comment on attachment 143019 [details]
Patch

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

>> Source/WebKit/chromium/src/StorageAreaProxy.cpp:206
>> +        InspectorInstrumentation::didDispatchDOMStorageEvent(key, oldValue, newValue, LocalStorage, securityOrigin, *it);
> 
> this code path is currently not going to be invoked, and in the future will only be invoked if the storage area was changed in another process

This looks like a good place to put this hook. This file has recently been changed in exactly this area of event dispatching so you should definitely update your sources. With the new sources, these two methods are the only place where StorageEvents are created (regardless of the process that caused the mutation).
Comment 77 Michael Nordman 2012-05-23 12:15:16 PDT
Comment on attachment 143019 [details]
Patch

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

> Source/WebCore/storage/StorageEventDispatcher.cpp:82
> +    InspectorInstrumentation::didDispatchDOMStorageEvent(key, oldValue, newValue, storageType, securityOrigin, page);

For localStorage, you probably need to tell the inspector for each page containing a document from 'origin' about the mutation. As coded only the page in which the mutation originated is being informed (which is fine for sessionStorage).
Comment 78 Vivek Galatage 2012-05-24 01:40:44 PDT
Created attachment 143759 [details]
Patch
Comment 79 Vivek Galatage 2012-05-24 01:57:29 PDT
Created attachment 143763 [details]
Patch
Comment 80 Pavel Feldman 2012-05-24 04:28:07 PDT
Committed r118349: <http://trac.webkit.org/changeset/118349>