EOM
Created attachment 152997 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Comment on attachment 152997 [details] Patch Looks OK to me, but I'd like michaeln@ to review this as well since it pertains to DOMStorage.
Comment on attachment 152997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review > Source/WebCore/inspector/InspectorMemoryAgent.cpp:639 > + size_t size = instrumentingAgents->inspectorDOMStorageAgent()->memoryBytesUsedByStorageCache(); You should never access agents in instrumentingAgents, only InspecorInstrumentation can do that.
Thnx for cc'ing me on the patch. Seems fine and good to me. View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review > Source/Platform/chromium/public/WebStorageArea.h:88 > + virtual size_t memoryBytesUsedByCache() const { return 0; } Do you have a CL for the chromium-side of this in the works? > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:226 > + size += it->second->storageArea()->memoryBytesUsedByCache(); I see that it's not possible for multiple area's in this collection to refer to the same <origin,type> because <host,type> duplicates are detected and weeded out when the collection is added to. I was going to ask, "can there be <origin,type> dups such that usage is over counted"... but now i'll ask, why filter on <host,type> instead of <origin,type>... given that filtering i think usage can be under counted since if both https://host and http://host are in use, you'll only tally the usage of which ever was first added to the inspector agency. > Source/WebCore/storage/StorageAreaImpl.cpp:269 > + return 0; Is the plan to return a non-zero value in a later patch?
(In reply to comment #5) > Thnx for cc'ing me on the patch. Seems fine and good to me. > > View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review > > > Source/Platform/chromium/public/WebStorageArea.h:88 > > + virtual size_t memoryBytesUsedByCache() const { return 0; } > > Do you have a CL for the chromium-side of this in the works? https://chromiumcodereview.appspot.com/10799008/
Created attachment 155544 [details] Patch
Comment on attachment 152997 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review >> Source/WebCore/inspector/InspectorMemoryAgent.cpp:639 >> + size_t size = instrumentingAgents->inspectorDOMStorageAgent()->memoryBytesUsedByStorageCache(); > > You should never access agents in instrumentingAgents, only InspecorInstrumentation can do that. ok. fixed.
(In reply to comment #5) > Thnx for cc'ing me on the patch. Seems fine and good to me. > > View in context: https://bugs.webkit.org/attachment.cgi?id=152997&action=review > > > Source/Platform/chromium/public/WebStorageArea.h:88 > > + virtual size_t memoryBytesUsedByCache() const { return 0; } > > Do you have a CL for the chromium-side of this in the works? https://chromiumcodereview.appspot.com/10799008/ > > > Source/WebCore/inspector/InspectorDOMStorageAgent.cpp:226 > > + size += it->second->storageArea()->memoryBytesUsedByCache(); > > I see that it's not possible for multiple area's in this collection to refer to the same <origin,type> because <host,type> duplicates are detected and weeded out when the collection is added to. I was going to ask, "can there be <origin,type> dups such that usage is over counted"... but now i'll ask, why filter on <host,type> instead of <origin,type>... given that filtering i think usage can be under counted since if both https://host and http://host are in use, you'll only tally the usage of which ever was first added to the inspector agency. It should be fixed with https://bugs.webkit.org/show_bug.cgi?id=92740 > > > Source/WebCore/storage/StorageAreaImpl.cpp:269 > > + return 0; > > Is the plan to return a non-zero value in a later patch? Yep.
Comment on attachment 155544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155544&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:1254 > +inline size_t InspectorInstrumentation::memoryBytesUsedByStorageCache(Page* page) InspectorInstrumentation is common place for inspector hooks called from the rest of WebCore and you shouldn't go though it for the operations that can be performed in the inspector code solely. In your case you can pass InspectorDOMStorageAgent to the InspectorMemoryAgent constructor so that it can be accessed when the memory snapshot is requested.
Comment on attachment 155544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155544&action=review > Source/Platform/ChangeLog:6 > + Reviewed by NOBODY (OOPS!). Please add a description.
Created attachment 155579 [details] Patch
Comment on attachment 155544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=155544&action=review >> Source/Platform/ChangeLog:6 >> + Reviewed by NOBODY (OOPS!). > > Please add a description. done >> Source/WebCore/inspector/InspectorInstrumentation.h:1254 >> +inline size_t InspectorInstrumentation::memoryBytesUsedByStorageCache(Page* page) > > InspectorInstrumentation is common place for inspector hooks called from the rest of WebCore and you shouldn't go though it for the operations that can be performed in the inspector code solely. In your case you can pass InspectorDOMStorageAgent to the InspectorMemoryAgent constructor so that it can be accessed when the memory snapshot is requested. Thanks for the explanation. Done.
Created attachment 155764 [details] Patch
Rebaselined
Comment on attachment 155764 [details] Patch Clearing flags on attachment: 155764 Committed r124332: <http://trac.webkit.org/changeset/124332>
All reviewed patches have been landed. Closing bug.