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.
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
(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.
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.
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.
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 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.
(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.
> 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").
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
(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 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.
(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.
> 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).
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
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
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 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.
(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?
> > > 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.
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
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 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.
(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
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
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 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.
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 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.
(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.
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
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
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
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
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 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
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 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 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).
2012-04-25 13:19 PDT, Vivek Galatage
2012-04-25 13:35 PDT, Vivek Galatage
2012-04-25 23:51 PDT, Vivek Galatage
2012-04-26 10:55 PDT, Vivek Galatage
2012-05-10 12:37 PDT, Vivek Galatage
2012-05-10 13:19 PDT, Vivek Galatage
2012-05-11 03:58 PDT, WebKit Review Bot
2012-05-14 01:28 PDT, Vivek Galatage
2012-05-14 01:59 PDT, Vivek Galatage
2012-05-14 02:35 PDT, WebKit Review Bot
2012-05-14 03:04 PDT, WebKit Review Bot
2012-05-14 04:07 PDT, WebKit Review Bot
2012-05-14 10:36 PDT, Vivek Galatage
2012-05-14 11:45 PDT, WebKit Review Bot
2012-05-14 12:48 PDT, WebKit Review Bot
2012-05-15 05:03 PDT, Vivek Galatage
2012-05-15 06:49 PDT, WebKit Review Bot
2012-05-15 10:18 PDT, Vivek Galatage
2012-05-16 11:21 PDT, Vivek Galatage
2012-05-16 11:49 PDT, Vivek Galatage
2012-05-16 13:48 PDT, WebKit Review Bot
2012-05-17 11:07 PDT, Vivek Galatage
2012-05-17 12:16 PDT, WebKit Review Bot
2012-05-17 13:19 PDT, WebKit Review Bot
2012-05-18 06:06 PDT, Vivek Galatage
2012-05-18 07:30 PDT, WebKit Review Bot
2012-05-18 08:24 PDT, WebKit Review Bot
2012-05-21 06:33 PDT, Vivek Galatage
2012-05-21 07:41 PDT, WebKit Review Bot
2012-05-24 01:40 PDT, Vivek Galatage
2012-05-24 01:57 PDT, Vivek Galatage