Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1044203004:
[Storage] Cache storage inspection on all the frames! (Closed)
|
Created:
5 years, 8 months ago by dmurph Modified:
5 years, 8 months ago CC:
aandrey+blink_chromium.org, apavlov+blink_chromium.org, blink-reviews, caseq+blink_chromium.org, devtools-reviews_chromium.org, falken, horo+watch_chromium.org, jsbell+serviceworker_chromium.org, jsbell, kenjibaheux, kinuko+serviceworker, kozyatinskiy+blink_chromium.org, loislo+blink_chromium.org, lushnikov+blink_chromium.org, malch+blink_chromium.org, michaeln, nhiroki, pfeldman+blink_chromium.org, sergeyv+blink_chromium.org, serviceworker-reviews, tzik, yurys+blink_chromium.org Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
Description[Storage] Cache storage inspection on all the frames!
* Simplified the ServiceWorkerCacheModel & added securityOrigin support
* Renamed protocol to "CacheStorage"
* Hooked up cache using new piping from Josh
* Add agent in the same spot as the IndexedDB agent.
BUG=439389
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=194060
Patch Set 1 #
Total comments: 14
Patch Set 2 : Cleanup, re-adding SWGlobalScope support, tested for pages and SW's #
Total comments: 28
Patch Set 3 : Fully converted to Map and Set #Patch Set 4 : Using Platform instead of searching frame #Patch Set 5 : More cleanup #
Total comments: 24
Patch Set 6 : WIP #Patch Set 7 : Added tests, cleanup, and a fix #Patch Set 8 : Comments #
Total comments: 28
Patch Set 9 : Test harness improvements, and comments #Patch Set 10 : Rebase #
Total comments: 18
Patch Set 11 : Comments and fix #
Total comments: 21
Patch Set 12 : Opaque ID and comments #Patch Set 13 : Tests #
Total comments: 14
Patch Set 14 : comments and rebase #
Total comments: 1
Messages
Total messages: 42 (5 generated)
dmurph@chromium.org changed reviewers: + horo@chromium.org, pfeldman@chromium.org, vsevik@chromium.org
Hello all, Can you take a look at this patch? This adds Cache Storage support to all frames, not just serviceworkers. pfeldman: Source/web/WebDevToolsAgentImpl.cpp horo: Source/modules/serviceworkers/* vsevik: Source/devtools/* Josh and I are trying to get this in before the branch point. Thanks! Daniel This is how I feel about this patch: http://i.imgur.com/wjdfuby.gif
Oh it's wonderful!! https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:65: static DOMWindow* assertWindow(ErrorString* errorString, LocalFrame* frame) remove "static" https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (left): https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:96: workerInspectorController()->registerModuleAgent(InspectorServiceWorkerCacheAgent::create(this)); DevTools for ServiceWorker is communicating with WorkerInspectorController. So, we can't inspect the cache storage from the DevTools for ServiceWorker with this patch. Is this behavior intended? If so, you should not show "Cache Storage" in the DevTools for ServiceWorker.
https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r... Source/devtools/front_end/resources/ResourcesPanel.js:1427: WebInspector.StorageCategoryTreeElement.call(this, storagePanel, WebInspector.UIString("Cache Storage"), "Cache Storage", ["service-worker-cache-storage-tree-item"]); "Cache Storage" -> "CacheStorage" https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r... Source/devtools/front_end/resources/ResourcesPanel.js:1436: for (var target of targets) { Targets can be added later, you should keep the target observer. Also, note that when the Page is a main target, the SW target can be added at a random moment and will belong to the same origin. So you need to make sure you don't dupe cache storages. Make sure to test it on the SW-controlled pages. It might be that you only care about the first target in case of Cache Storage. https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:15: this._cacheNamesBySecurityOrigin = {}; Use Map. Also, please extract the Cache-per-Origin change from this one since it seems to be self-contained. https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/sdk/ServiceWorkerManager.js (right): https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/sdk/ServiceWorkerManager.js:401: this.suppressErrorsForDomains(["Worker", "Page", "CSS", "DOM", "DOMStorage", "Database", "Network", "IndexedDB", "CacheStorage"]); Hm. Not sure why it was here - probably was copied from dedicated workers. Anyhow, we do have handlers for it in SW, lets remove it. https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:346: LocalFrame* frame = findFrameWithSecurityOrigin(m_page, securityOrigin); This one is important: going into the frame, through the global window, script state would be necessary if we were processing a call from the page. But you don't need any of that - assertCacheStorage would work just fine given the security origin.
I sent both of you an email with some longer questions :) On Wed, Apr 1, 2015, 12:01 AM <pfeldman@chromium.org> wrote: > > https://codereview.chromium.org/1044203004/diff/1/Source/ > devtools/front_end/resources/ResourcesPanel.js > File Source/devtools/front_end/resources/ResourcesPanel.js (right): > > https://codereview.chromium.org/1044203004/diff/1/Source/ > devtools/front_end/resources/ResourcesPanel.js#newcode1427 > Source/devtools/front_end/resources/ResourcesPanel.js:1427 > <https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r...> > : > WebInspector.StorageCategoryTreeElement.call(this, storagePanel, > WebInspector.UIString("Cache Storage"), "Cache Storage", > ["service-worker-cache-storage-tree-item"]); > "Cache Storage" -> "CacheStorage" > > https://codereview.chromium.org/1044203004/diff/1/Source/ > devtools/front_end/resources/ResourcesPanel.js#newcode1436 > Source/devtools/front_end/resources/ResourcesPanel.js:1436 > <https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r...>: > for (var > target of targets) { > Targets can be added later, you should keep the target observer. Also, > note that when the Page is a main target, the SW target can be added at > a random moment and will belong to the same origin. So you need to make > sure you don't dupe cache storages. Make sure to test it on the > SW-controlled pages. It might be that you only care about the first > target in case of Cache Storage. > > https://codereview.chromium.org/1044203004/diff/1/Source/ > devtools/front_end/sdk/ServiceWorkerCacheModel.js > File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): > > https://codereview.chromium.org/1044203004/diff/1/Source/ > devtools/front_end/sdk/ServiceWorkerCacheModel.js#newcode15 > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:15 > <https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s...> > : > this._cacheNamesBySecurityOrigin = {}; > Use Map. > > Also, please extract the Cache-per-Origin change from this one since it > seems to be self-contained. > > https://codereview.chromium.org/1044203004/diff/1/Source/ > devtools/front_end/sdk/ServiceWorkerManager.js > File Source/devtools/front_end/sdk/ServiceWorkerManager.js (right): > > https://codereview.chromium.org/1044203004/diff/1/Source/ > devtools/front_end/sdk/ServiceWorkerManager.js#newcode401 > Source/devtools/front_end/sdk/ServiceWorkerManager.js:401 > <https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s...> > : > this.suppressErrorsForDomains(["Worker", "Page", "CSS", "DOM", > "DOMStorage", "Database", "Network", "IndexedDB", "CacheStorage"]); > Hm. Not sure why it was here - probably was copied from dedicated > workers. Anyhow, we do have handlers for it in SW, lets remove it. > > https://codereview.chromium.org/1044203004/diff/1/Source/ > modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp > File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp > (right): > > https://codereview.chromium.org/1044203004/diff/1/Source/ > modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp#newcode346 > Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:346 > <https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke...> > : > LocalFrame* frame = findFrameWithSecurityOrigin(m_page, securityOrigin); > This one is important: going into the frame, through the global window, > script state would be necessary if we were processing a call from the > page. But you don't need any of that - assertCacheStorage would work > just fine given the security origin. > > https://codereview.chromium.org/1044203004/ > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
I believe I addressed the issues here. With the IndexedDB fix this now works. Only the primary model is 'enabled', which will have the caches hookup. The cpp side can now talk to either the document or the SWGlobalScope. I'm hoping to get this in by the branch point if possible :) Thanks! Daniel https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r... Source/devtools/front_end/resources/ResourcesPanel.js:1427: WebInspector.StorageCategoryTreeElement.call(this, storagePanel, WebInspector.UIString("Cache Storage"), "Cache Storage", ["service-worker-cache-storage-tree-item"]); On 2015/04/01 at 07:01:09, pfeldman wrote: > "Cache Storage" -> "CacheStorage" Done. https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/r... Source/devtools/front_end/resources/ResourcesPanel.js:1436: for (var target of targets) { On 2015/04/01 at 07:01:09, pfeldman wrote: > Targets can be added later, you should keep the target observer. Also, note that when the Page is a main target, the SW target can be added at a random moment and will belong to the same origin. So you need to make sure you don't dupe cache storages. Make sure to test it on the SW-controlled pages. It might be that you only care about the first target in case of Cache Storage. I don't need to because we listen for securityOrigins on the model, which get called when targets are added (unless I'm wrong). The cache names are treated as a set, before the 'cache added' or 'removed' events are sent out, so there shouldn't be duplicates. https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:15: this._cacheNamesBySecurityOrigin = {}; On 2015/04/01 at 07:01:09, pfeldman wrote: > Use Map. > > Also, please extract the Cache-per-Origin change from this one since it seems to be self-contained. It's basically exactly the same patch, but without some of the CPP changes. I'd rather have it all in one, especially because we're trying to get it in before the branch point (tomorrow). Is that alright? https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... File Source/devtools/front_end/sdk/ServiceWorkerManager.js (right): https://codereview.chromium.org/1044203004/diff/1/Source/devtools/front_end/s... Source/devtools/front_end/sdk/ServiceWorkerManager.js:401: this.suppressErrorsForDomains(["Worker", "Page", "CSS", "DOM", "DOMStorage", "Database", "Network", "IndexedDB", "CacheStorage"]); On 2015/04/01 at 07:01:09, pfeldman wrote: > Hm. Not sure why it was here - probably was copied from dedicated workers. Anyhow, we do have handlers for it in SW, lets remove it. What am I removing? The "CacheStorage" string? or this whole line? https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:65: static DOMWindow* assertWindow(ErrorString* errorString, LocalFrame* frame) On 2015/04/01 at 03:39:07, horo wrote: > remove "static" Done. https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:346: LocalFrame* frame = findFrameWithSecurityOrigin(m_page, securityOrigin); On 2015/04/01 at 07:01:09, pfeldman wrote: > This one is important: going into the frame, through the global window, script state would be necessary if we were processing a call from the page. But you don't need any of that - assertCacheStorage would work just fine given the security origin. I think I understand what you mean. I removed scriptState. https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (left): https://codereview.chromium.org/1044203004/diff/1/Source/modules/serviceworke... Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:96: workerInspectorController()->registerModuleAgent(InspectorServiceWorkerCacheAgent::create(this)); On 2015/04/01 at 03:39:07, horo wrote: > DevTools for ServiceWorker is communicating with WorkerInspectorController. > So, we can't inspect the cache storage from the DevTools for ServiceWorker with this patch. > > Is this behavior intended? > If so, you should not show "Cache Storage" in the DevTools for ServiceWorker. Ah, you're right. Fixed.
https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/resources/ResourcesPanel.js:1436: for (var target of targets) { You can directly access the WebInspector.targetManager.mainTarget() with this new setup. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/resources/ResourcesPanel.js:1465: for (var target of targets) ditto https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:73: if (!this._cacheNamesBySecurityOrigin[securityOrigin]) Now that this is a map, you should use .set, .get and .delete https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:76: var index = caches.indexOf(cacheId.name); Arrays have .remove() in devtools. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:77: if (index != -1) { no {} around single-line blocks https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:102: for (var securityOrigin in this._cacheNamesBySecurityOrigin) { this would be for (var caches of this._cacheNamesBySecurityOrigin.values()) https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:111: dispose: function() unregister the listeners to resourceTreeModel here. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:113: for (var securityOrigin in this._cacheNamesBySecurityOrigin) { of .keys() https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:116: this._cacheNamesBySecurityOrigin = {}; .clear() https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:120: console.assert(!this._cacheNamesBySecurityOrigin[securityOrigin]); ditto https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:131: for (var cacheName of this._cacheNamesBySecurityOrigin[securityOrigin]) ditto (and may times below) https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/ServiceWorkerManager.js (right): https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerManager.js:448: this.suppressErrorsForDomains(["Worker", "Page", "CSS", "DOM", "DOMStorage", "Database", "Network", "IndexedDB"]); Yes, like this! https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:53: LocalFrame* findFrameWithSecurityOrigin(Page* page, const String& securityOrigin) Sorry, did I miss a reply here? I was suggesting to remove this method. https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:65: DOMWindow* assertWindow(ErrorString* errorString, LocalFrame* frame) And this method https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:77: TrackExceptionState exceptionState; And go straight to WebServiceWorkerCacheStorage here through the platform accessor.
(we definitely don't want to land anything before the branch point, we should land it after and get full 6 weeks of Canary operation)
On 2015/04/02 21:19:39, pfeldman wrote: > (we definitely don't want to land anything before the branch point, we should > land it after and get full 6 weeks of Canary operation) A full 6 weeks of bake time in Canary sounds heavy :/ We've never hesitated to land before the branch point (IMHO, that's the whole point of the branch cut deadline) without concrete concerns. - Is the potential negative impact limited to the presence of Cache Storages? - Would a revert/disabling CL be easy to merge? If the answer to these questions are affirmative, then without concrete concerns I don't see why we should hold on (given a reasonable assessment that a change is safe, it's common practice to monitor for issues while in dev and beta and act appropriately). Thoughts?
On 2015/04/02 21:19:39, pfeldman wrote: > (we definitely don't want to land anything before the branch point, we should > land it after and get full 6 weeks of Canary operation) A full 6 weeks of bake time in Canary sounds heavy :/ We've never hesitated to land before the branch point (IMHO, that's the whole point of the branch cut deadline) without concrete concerns. - Is the potential negative impact limited to the presence of Cache Storages? - Would a revert/disabling CL be easy to merge? If the answer to these questions are affirmative, then without concrete concerns I don't see why we should hold on (given a reasonable assessment that a change is safe, it's common practice to monitor for issues while in dev and beta and act appropriately). Thoughts?
I fixed all the map stuff, and I added Sets instead of arrays. Tested for both inspector instances. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/resources/ResourcesPanel.js:1436: for (var target of targets) { On 2015/04/02 at 21:18:57, pfeldman wrote: > You can directly access the WebInspector.targetManager.mainTarget() with this new setup. done https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/resources/ResourcesPanel.js:1465: for (var target of targets) On 2015/04/02 at 21:18:57, pfeldman wrote: > ditto Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:73: if (!this._cacheNamesBySecurityOrigin[securityOrigin]) On 2015/04/02 at 21:18:57, pfeldman wrote: > Now that this is a map, you should use .set, .get and .delete Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:76: var index = caches.indexOf(cacheId.name); On 2015/04/02 at 21:18:57, pfeldman wrote: > Arrays have .remove() in devtools. Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:77: if (index != -1) { On 2015/04/02 at 21:18:57, pfeldman wrote: > no {} around single-line blocks Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:102: for (var securityOrigin in this._cacheNamesBySecurityOrigin) { On 2015/04/02 at 21:18:58, pfeldman wrote: > this would be for (var caches of this._cacheNamesBySecurityOrigin.values()) Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:111: dispose: function() On 2015/04/02 at 21:18:58, pfeldman wrote: > unregister the listeners to resourceTreeModel here. done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:113: for (var securityOrigin in this._cacheNamesBySecurityOrigin) { On 2015/04/02 at 21:18:57, pfeldman wrote: > of .keys() Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:116: this._cacheNamesBySecurityOrigin = {}; On 2015/04/02 at 21:18:57, pfeldman wrote: > .clear() Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:120: console.assert(!this._cacheNamesBySecurityOrigin[securityOrigin]); On 2015/04/02 at 21:18:58, pfeldman wrote: > ditto Done. https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:131: for (var cacheName of this._cacheNamesBySecurityOrigin[securityOrigin]) On 2015/04/02 at 21:18:58, pfeldman wrote: > ditto (and may times below) Done https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... File Source/devtools/front_end/sdk/ServiceWorkerManager.js (right): https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerManager.js:448: this.suppressErrorsForDomains(["Worker", "Page", "CSS", "DOM", "DOMStorage", "Database", "Network", "IndexedDB"]); On 2015/04/02 at 21:18:58, pfeldman wrote: > Yes, like this! Done. https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:77: TrackExceptionState exceptionState; On 2015/04/02 at 21:18:58, pfeldman wrote: > And go straight to WebServiceWorkerCacheStorage here through the platform accessor. Ah. So calling Platform::current()->cacheStorage(securityOrigin) currently check fails. I'm not sure why. I can add a todo for fixing that and rewiring this (as well as indexeddb)
On 2015/04/02 at 23:23:18, dmurph wrote: > I fixed all the map stuff, and I added Sets instead of arrays. Tested for both inspector instances. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > File Source/devtools/front_end/resources/ResourcesPanel.js (right): > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/resources/ResourcesPanel.js:1436: for (var target of targets) { > On 2015/04/02 at 21:18:57, pfeldman wrote: > > You can directly access the WebInspector.targetManager.mainTarget() with this new setup. > > done > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/resources/ResourcesPanel.js:1465: for (var target of targets) > On 2015/04/02 at 21:18:57, pfeldman wrote: > > ditto > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:73: if (!this._cacheNamesBySecurityOrigin[securityOrigin]) > On 2015/04/02 at 21:18:57, pfeldman wrote: > > Now that this is a map, you should use .set, .get and .delete > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:76: var index = caches.indexOf(cacheId.name); > On 2015/04/02 at 21:18:57, pfeldman wrote: > > Arrays have .remove() in devtools. > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:77: if (index != -1) { > On 2015/04/02 at 21:18:57, pfeldman wrote: > > no {} around single-line blocks > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:102: for (var securityOrigin in this._cacheNamesBySecurityOrigin) { > On 2015/04/02 at 21:18:58, pfeldman wrote: > > this would be for (var caches of this._cacheNamesBySecurityOrigin.values()) > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:111: dispose: function() > On 2015/04/02 at 21:18:58, pfeldman wrote: > > unregister the listeners to resourceTreeModel here. > > done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:113: for (var securityOrigin in this._cacheNamesBySecurityOrigin) { > On 2015/04/02 at 21:18:57, pfeldman wrote: > > of .keys() > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:116: this._cacheNamesBySecurityOrigin = {}; > On 2015/04/02 at 21:18:57, pfeldman wrote: > > .clear() > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:120: console.assert(!this._cacheNamesBySecurityOrigin[securityOrigin]); > On 2015/04/02 at 21:18:58, pfeldman wrote: > > ditto > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:131: for (var cacheName of this._cacheNamesBySecurityOrigin[securityOrigin]) > On 2015/04/02 at 21:18:58, pfeldman wrote: > > ditto (and may times below) > > Done > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > File Source/devtools/front_end/sdk/ServiceWorkerManager.js (right): > > https://codereview.chromium.org/1044203004/diff/20001/Source/devtools/front_e... > Source/devtools/front_end/sdk/ServiceWorkerManager.js:448: this.suppressErrorsForDomains(["Worker", "Page", "CSS", "DOM", "DOMStorage", "Database", "Network", "IndexedDB"]); > On 2015/04/02 at 21:18:58, pfeldman wrote: > > Yes, like this! > > Done. > > https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... > File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): > > https://codereview.chromium.org/1044203004/diff/20001/Source/modules/servicew... > Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:77: TrackExceptionState exceptionState; > On 2015/04/02 at 21:18:58, pfeldman wrote: > > And go straight to WebServiceWorkerCacheStorage here through the platform accessor. > > Ah. So calling Platform::current()->cacheStorage(securityOrigin) currently check fails. I'm not sure why. I can add a todo for fixing that and rewiring this (as well as indexeddb) Nevermind! I figured it out, now we just use Platform::current()->cacheStorage(...)
One last patch with more cleanup, we no longer need the page or the global scope, and removed the idl file changes. Thanks Josh for the tips/help w/ getting the cache hooked up through the platform class :)
https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:49: WebServiceWorkerCacheStorage* cache = Platform::current()->cacheStorage(identifier); nit: unnecessary apace after = https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:49: WebServiceWorkerCacheStorage* cache = Platform::current()->cacheStorage(identifier); Who deletes this |cache|? https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... // The caller is responsible for deleting the returned object. virtual WebServiceWorkerCacheStorage* cacheStorage(const WebString& originIdentifier) { return nullptr; }
jsbell@chromium.org changed reviewers: + jsbell@chromium.org
https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:49: WebServiceWorkerCacheStorage* cache = Platform::current()->cacheStorage(identifier); On 2015/04/03 01:23:46, horo wrote: > Who deletes this |cache|? > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > // The caller is responsible for deleting the returned object. > virtual WebServiceWorkerCacheStorage* cacheStorage(const WebString& > originIdentifier) { return nullptr; } Agreed - this this should keep the PassOwnPtr<WebServiceWorkerCacheStorage> return type, and be captured into (temporary) OwnPtr<> by the callers to clean it up. https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right): https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:98: workerInspectorController()->registerModuleAgent(InspectorServiceWorkerCacheAgent::create()); Is this still necessary, or can the Cache Storage agent be hooked up the same way that the Indexed DB agent is?
> If the answer to these questions are affirmative, then without concrete > concerns I don't see why we should hold on (given a reasonable assessment that a > change is safe, it's common practice to monitor for issues while in dev and beta > and act appropriately). > > Thoughts? I was referring to Daniel's "I'm hoping to get this in by the branch point if possible :)" and was setting the expectations straight. Which is "it is highly unlikely it gets into M43". The branch point is today and rushing with the implementation in order to get something into the branch is officially discouraged. It is ready when it is ready - this patch was clearly not ready back then and it definitely is not safe. Talking about launching before the branch point, I wonder if you are referring to the launches of the large UI surfaces in Chrome. Those never happen without the experiments and significant amount of time on Canary. In DevTools, every new UI needs to be on the Canary for at least a couple of weeks before the branch since that's the time it takes for the feedback to be communicated. The feedback we get while in Beta is too late since these are typically not the end-of-the-world types of the bugs and they end up in stable. We don't like merging into Beta - we have enough critical stuff to merge and more importantly, we are not in a hurry. DevTools is a huge UI surface and UI is poorly testable (layout, focus, other issues other pretty much not covered). This very feature we are talking about has 0 tests, it has literally zero test coverage. Landing it on the last day before the branch point is asking for trouble. We were fine with it while it was in the context of the SW (that version basically has no usage, if it is broken we tell those people to use Canary), but now you are throwing it against the whole stable DevTools user base and that requires testing and Canary trial.
Overall, this looks good now. Please use an opaque key and cover it with the tests. IndexedDB ones could probably be reused. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:68: var caches = this._cacheNamesBySecurityOrigin.get(securityOrigin); get it right away, then bail out if none. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:92: this._cacheNamesBySecurityOrigin.forEach(function(caches, securityOrigin) { We don't use anonymous functions, please declare named function outside. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:93: for (var cacheName of caches) { no {} around single line blocks https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:102: for (var securityOrigin of this._cacheNamesBySecurityOrigin.keys()) { no {} https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:112: _addOrigin: function(securityOrigin) { { on the next line. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:167: if (tempOldCacheNames) { no {} https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... Source/devtools/protocol.json:1713: { "name": "securityOrigin", "type": "string", "description": "Security origin." }, Composite ids is hard to work with for the clients, I'd rather expose cacheId instead of a cache name + sequrity origin that would uniquely identify it. Even at the expense of parseable composite "securityorigin:name" id at the backend that is opaque to the front-end. https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... File Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp (right): https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:98: workerInspectorController()->registerModuleAgent(InspectorServiceWorkerCacheAgent::create()); On 2015/04/03 03:21:08, jsbell wrote: > Is this still necessary, or can the Cache Storage agent be hooked up the same > way that the Indexed DB agent is? Good point, it should only be in WebDevToolsAgentImpl::create now.
New Patch with a few questions. Also: I'm getting errors at the .forEach call on the map from the jsdoc and jslint analyzers. If I have the anon function with 'this' as the second argument, then everything is fine. If I split out the function, then jsdoc complains that 'this' isn't used in the called function. If I remove the second argument (which the spec allows), then I get an error from jslint that I need at least 2 arguments. This seems wrong, but I don't know how to fix it. I resulted in ignoring the jsdoc error. Is that the best thing to do? Thanks, Daniel https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:68: var caches = this._cacheNamesBySecurityOrigin.get(securityOrigin); On 2015/04/03 at 09:15:57, pfeldman wrote: > get it right away, then bail out if none. Done. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:92: this._cacheNamesBySecurityOrigin.forEach(function(caches, securityOrigin) { On 2015/04/03 at 09:15:57, pfeldman wrote: > We don't use anonymous functions, please declare named function outside. I feel like that is normal and readable for forEach, but okay :) https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:93: for (var cacheName of caches) { On 2015/04/03 at 09:15:57, pfeldman wrote: > no {} around single line blocks Done. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:102: for (var securityOrigin of this._cacheNamesBySecurityOrigin.keys()) { On 2015/04/03 at 09:15:58, pfeldman wrote: > no {} Done. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:112: _addOrigin: function(securityOrigin) { On 2015/04/03 at 09:15:57, pfeldman wrote: > { on the next line. Done. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:167: if (tempOldCacheNames) { On 2015/04/03 at 09:15:57, pfeldman wrote: > no {} For an if-else statement? Isn't that super awful? https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... Source/devtools/protocol.json:1713: { "name": "securityOrigin", "type": "string", "description": "Security origin." }, On 2015/04/03 at 09:15:58, pfeldman wrote: > Composite ids is hard to work with for the clients, I'd rather expose cacheId instead of a cache name + sequrity origin that would uniquely identify it. Even at the expense of parseable composite "securityorigin:name" id at the backend that is opaque to the front-end. Just to clarify, you want me to remove the securityOrigin and cacheName part and instead use a cacheId string that would then be parsed in the backend? Just FYI, IndexedDB does it this way. Also, there are cases, like requestCacheNames, where we only need the security origin. Also, how is this hard to work with? https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... File Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/70001/Source/modules/servicew... Source/modules/serviceworkers/InspectorServiceWorkerCacheAgent.cpp:49: WebServiceWorkerCacheStorage* cache = Platform::current()->cacheStorage(identifier); On 2015/04/03 at 03:21:08, jsbell wrote: > On 2015/04/03 01:23:46, horo wrote: > > Who deletes this |cache|? > > > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > // The caller is responsible for deleting the returned object. > > virtual WebServiceWorkerCacheStorage* cacheStorage(const WebString& > > originIdentifier) { return nullptr; } > > Agreed - this this should keep the PassOwnPtr<WebServiceWorkerCacheStorage> return type, and be captured into (temporary) OwnPtr<> by the callers to clean it up. Done.
https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:92: this._cacheNamesBySecurityOrigin.forEach(function(caches, securityOrigin) { On 2015/04/08 19:35:35, dmurph wrote: > On 2015/04/03 at 09:15:57, pfeldman wrote: > > We don't use anonymous functions, please declare named function outside. > > I feel like that is normal and readable for forEach, but okay :) That's JS coding guidelines for Blink, otherwise it won't be checking params during compilation. So this is not a matter of preference. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/front_e... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:167: if (tempOldCacheNames) { On 2015/04/08 19:35:35, dmurph wrote: > On 2015/04/03 at 09:15:57, pfeldman wrote: > > no {} > > For an if-else statement? Isn't that super awful? https://www.chromium.org/blink/coding-style https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... Source/devtools/protocol.json:1702: { "name": "securityOrigin", "type": "string", "description": "Security origin." } Now sure why fetching by origin is important, lets fetch all. https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... Source/devtools/protocol.json:1713: { "name": "securityOrigin", "type": "string", "description": "Security origin." }, > Just to clarify, you want me to remove the securityOrigin and cacheName part and > instead use a cacheId string that would then be parsed in the backend? Yes. > > Just FYI, IndexedDB does it this way. Also, there are cases, like > requestCacheNames, where we only need the security origin. Too bad for IndexedDB. Why would we need requestCacheNames per security origin? > Also, how is this hard to work with? Why composite primary key is bad? Well, because when working with the remote instance, you need a handle to that instance that would identify it. You operate on this instance using this handle to refer to it, you associate updates to the instances based on that handle. So there is inevitably a lookup from the handle to the instance and back. That's why clients of your API will be concatenating the sequrityOrigin and cacheName in one string in order to flatten it and use a single map for a lookup. Or even worse, will use hierarchical maps.
> https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... > Source/devtools/protocol.json:1702: { "name": "securityOrigin", "type": "string", "description": "Security origin." } > Now sure why fetching by origin is important, lets fetch all. We need the security origin to look up the database identifiers. Does that mean you want the agent to transverse the entire frame heirarchy to find all of the security origins? That seems like overkill when we have that data in the inspector, and especially we when have the events for when security origins are added/removed. > > https://codereview.chromium.org/1044203004/diff/70001/Source/devtools/protoco... > Source/devtools/protocol.json:1713: { "name": "securityOrigin", "type": "string", "description": "Security origin." }, > > Just to clarify, you want me to remove the securityOrigin and cacheName part and > > instead use a cacheId string that would then be parsed in the backend? > > Yes. > > > > > Just FYI, IndexedDB does it this way. Also, there are cases, like > > requestCacheNames, where we only need the security origin. > > Too bad for IndexedDB. Why would we need requestCacheNames per security origin? > > > > Also, how is this hard to work with? > > Why composite primary key is bad? Well, because when working with the remote instance, you need a handle to that instance that would identify it. You operate on this instance using this handle to refer to it, you associate updates to the instances based on that handle. So there is inevitably a lookup from the handle to the instance and back. That's why clients of your API will be concatenating the sequrityOrigin and cacheName in one string in order to flatten it and use a single map for a lookup. Or even worse, will use hierarchical maps. We could use the securityorigin:cachename pair for all of the other API calls, but I don't think we should remove the securityOrigin from the getCacheNames call.
> We need the security origin to look up the database identifiers. Does that mean > you want the agent to transverse the entire frame heirarchy to find all of the > security origins? That seems like overkill when we have that data in the > inspector, and especially we when have the events for when security origins are > added/removed. Traversing frames is actually fine. In fact you should not rely upon security origin events, ideally your agent should be self-contained. > We could use the securityorigin:cachename pair for all of the other API calls, > but I don't think we should remove the securityOrigin from the getCacheNames > call. I like it when agents are self-contained, we would get a whole list of caches and get notified when new ones are created or the old ones are being removed. But it is hard to tell at this moment which model would work better in case your agent moves into content/. If we are to implement a single cache browser that would list all the origins, your model would actually work better. So lets leave it your way.
https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-data-expected.txt (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data-expected.txt:1: CONSOLE MESSAGE: line 156: InspectorTest.CacheStorage_callback1Resolve Lets not use console.log, but rather InspectorTest.addResult so that we did not have to rebaseline tests every time we add a line into a harness. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-data.html (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data.html:16: console.error(error); 4 space indent. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data.html:22: return caches.keys().then(function(keys) { These are the caches of the inspector front-end, not the page under test. I assume this is important. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data.html:23: return Promise.all(keys.map(function(key) { 4 space indent. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html:12: InspectorTest.addSniffer(WebInspector.ServiceWorkerCacheModel.prototype, "_updateCacheNames", main, false); Since used more than once, it could be a part of cache-storage-test.js https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html:23: return caches.keys().then(function(keys) { Since used more than once, it could be a part of cache-storage-test.js https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:1: var initialize_CacheStorageTest = function() { Copyright, please also use Blink coding guidelines: 4 space indentation. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:5: var lastCallbackId = 0; Section below is cryptic. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:15: var getAndDeleteCallbackFromId = function(callbackId) { Lets move InspectorTest.evaluateWithPromise along with the helper methods into inspector-test.js https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:39: }; drop ; https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:93: if (!view) { no {} https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:156: console.log(callbackId); InspectorTest.addResult https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/inspector-test.js (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:435: var original = receiver[methodName]; poor indent. https://codereview.chromium.org/1044203004/diff/130001/Source/devtools/front_... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/130001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:93: var mapEntriesToResults = function(caches, securityOrigin) { Your patch does not apply to ToT. But you should: 1) make this a named function, as per out style. 2) annotate its parameters 3) do not pass this into forEach since you don't use it.
Source/modules/serviceworkers/* lgtm
https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-data-expected.txt (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data-expected.txt:1: CONSOLE MESSAGE: line 156: InspectorTest.CacheStorage_callback1Resolve On 2015/04/10 at 12:35:33, pfeldman wrote: > Lets not use console.log, but rather InspectorTest.addResult so that we did not have to rebaseline tests every time we add a line into a harness. Hm, ok. That's how IndexedDB did callbacks. I took a shot at changing this and supporting it in the base InspectorTest. It uses promises, and now supports sending json-able arguments. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-data.html (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data.html:16: console.error(error); On 2015/04/10 at 12:35:33, pfeldman wrote: > 4 space indent. Done. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data.html:22: return caches.keys().then(function(keys) { On 2015/04/10 at 12:35:33, pfeldman wrote: > These are the caches of the inspector front-end, not the page under test. I assume this is important. Fixed. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-data.html:23: return Promise.all(keys.map(function(key) { On 2015/04/10 at 12:35:33, pfeldman wrote: > 4 space indent. Done. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html:12: InspectorTest.addSniffer(WebInspector.ServiceWorkerCacheModel.prototype, "_updateCacheNames", main, false); On 2015/04/10 at 12:35:33, pfeldman wrote: > Since used more than once, it could be a part of cache-storage-test.js Done. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html:23: return caches.keys().then(function(keys) { On 2015/04/10 at 12:35:33, pfeldman wrote: > Since used more than once, it could be a part of cache-storage-test.js Done. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:1: var initialize_CacheStorageTest = function() { On 2015/04/10 at 12:35:33, pfeldman wrote: > Copyright, please also use Blink coding guidelines: 4 space indentation. Done. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:5: var lastCallbackId = 0; On 2015/04/10 at 12:35:33, pfeldman wrote: > Section below is cryptic. Removed https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:15: var getAndDeleteCallbackFromId = function(callbackId) { On 2015/04/10 at 12:35:33, pfeldman wrote: > Lets move InspectorTest.evaluateWithPromise along with the helper methods into inspector-test.js Done, as 'invokePageFunctionPromise' https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:39: }; On 2015/04/10 at 12:35:33, pfeldman wrote: > drop ; Done. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:93: if (!view) { On 2015/04/10 at 12:35:33, pfeldman wrote: > no {} Done. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:156: console.log(callbackId); On 2015/04/10 at 12:35:33, pfeldman wrote: > InspectorTest.addResult Removed. https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/inspector-test.js (right): https://codereview.chromium.org/1044203004/diff/130001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:435: var original = receiver[methodName]; On 2015/04/10 at 12:35:33, pfeldman wrote: > poor indent. Removed. https://codereview.chromium.org/1044203004/diff/130001/Source/devtools/front_... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/130001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:93: var mapEntriesToResults = function(caches, securityOrigin) { Ended up being unneeded.
https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html (right): https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html:14: console.error(error); nit: InspectorTest.addResult https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js (right): https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:14: InspectorTest.addSnifferPromise(WebInspector.ServiceWorkerCacheModel.prototype, "_updateCacheNames").then(function() { Devtools uses named functions everywhere. I know it is not Promise-friendly. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:19: function queryView(i) { Here and below, blank lines around named functions. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:22: function nextOrResolve() { Here and below, { on the next line please. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:60: var promise = new Promise(function(resolve, reject) { Same on named functions and blank lines. This is hard to read. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:127: }).then(resolve) Here and below: weird indent. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/inspector-test.js (right): https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:84: function evalCallbackReject(result) Blank lines around functions. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:93: console.error(e); Inspector.addResult, no console.error please, they are not what you think (see the beginning of this file) https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:489: var original = receiver[methodName]; Poor indent.
PTAL :) https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html (right): https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-deletion.html:14: console.error(error); On 2015/04/15 at 17:50:08, pfeldman wrote: > nit: InspectorTest.addResult Done. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js (right): https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:14: InspectorTest.addSnifferPromise(WebInspector.ServiceWorkerCacheModel.prototype, "_updateCacheNames").then(function() { On 2015/04/15 at 17:50:08, pfeldman wrote: > Devtools uses named functions everywhere. I know it is not Promise-friendly. It looks like it's already used this way here: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... and elsewhere: https://code.google.com/p/chromium/codesearch#search/&q=%5C.then%5C(%20file:(... All promise users are putting functions as arguments. I would argue that this should continue for consistency both within the codebase and with pretty much all external use of promises. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:19: function queryView(i) { On 2015/04/15 at 17:50:08, pfeldman wrote: > Here and below, blank lines around named functions. Done. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:22: function nextOrResolve() { On 2015/04/15 at 17:50:08, pfeldman wrote: > Here and below, { on the next line please. Done. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:60: var promise = new Promise(function(resolve, reject) { On 2015/04/15 at 17:50:08, pfeldman wrote: > Same on named functions and blank lines. This is hard to read. Done. I reordered the functions so it's hopefully a little more readable. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/cache-storage/cache-storage-test.js:127: }).then(resolve) On 2015/04/15 at 17:50:08, pfeldman wrote: > Here and below: weird indent. Done. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... File LayoutTests/http/tests/inspector/inspector-test.js (right): https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:84: function evalCallbackReject(result) On 2015/04/15 at 17:50:08, pfeldman wrote: > Blank lines around functions. Done. https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:93: console.error(e); On 2015/04/15 at 17:50:08, pfeldman wrote: > Inspector.addResult, no console.error please, they are not what you think (see the beginning of this file) Done. (This was copied from below, so I changed invokePageFunctionAsync as well) https://codereview.chromium.org/1044203004/diff/170001/LayoutTests/http/tests... LayoutTests/http/tests/inspector/inspector-test.js:489: var original = receiver[methodName]; On 2015/04/15 at 17:50:08, pfeldman wrote: > Poor indent. Done.
https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/resources/ResourcesPanel.js:1432: var target = WebInspector.targetManager.mainTarget(); Nit: var target = this._storagePanel._target; https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/resources/ResourcesPanel.js:1458: var target = WebInspector.targetManager.mainTarget(); ditto https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:17: this._caches = new Map(); Sorry, I did not notice you migrated to cacheIds, so was only reviewing the test-related part. I am not sure I follow this bit. What I would understand is if there was a WebInspector.ServiceWorkerCache that would contain a this._securityOrigin, this._cacheId fields along with cache-related methods and a map from opaque cacheId to such a cache instance. Mapping a JSON-stringified id object to itself in order to fetch sequrityOrigin + flattening cache-related methods here does not look particularly good. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:48: for (var securityOrigin of securityOrigins) { drop {} https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:91: for (var cache of this._caches.values()) { drop {} https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:92: caches[i++] = cache; We typically just .push for simplicity. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... Source/devtools/protocol.json:1703: "type": "object", Identifiers must be opaque strings, you should do parsing on the backend. https://codereview.chromium.org/1044203004/diff/190001/Source/modules/cachest... File Source/modules/cachestorage/CacheStorage.h (right): https://codereview.chromium.org/1044203004/diff/190001/Source/modules/cachest... Source/modules/cachestorage/CacheStorage.h:35: WebServiceWorkerCacheStorage* webCacheStorage() const { return m_webCacheStorage.get(); } I thought this was no longer necessary.
On 2015/04/16 at 10:22:53, pfeldman wrote: > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > File Source/devtools/front_end/resources/ResourcesPanel.js (right): > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > Source/devtools/front_end/resources/ResourcesPanel.js:1432: var target = WebInspector.targetManager.mainTarget(); > Nit: var target = this._storagePanel._target; > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > Source/devtools/front_end/resources/ResourcesPanel.js:1458: var target = WebInspector.targetManager.mainTarget(); > ditto > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:17: this._caches = new Map(); > Sorry, I did not notice you migrated to cacheIds, so was only reviewing the test-related part. > > I am not sure I follow this bit. What I would understand is if there was a WebInspector.ServiceWorkerCache that would contain a this._securityOrigin, this._cacheId fields along with cache-related methods and a map from opaque cacheId to such a cache instance. > > Mapping a JSON-stringified id object to itself in order to fetch sequrityOrigin + flattening cache-related methods here does not look particularly good. > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:48: for (var securityOrigin of securityOrigins) { > drop {} > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:91: for (var cache of this._caches.values()) { > drop {} > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... > Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:92: caches[i++] = cache; > We typically just .push for simplicity. > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... > File Source/devtools/protocol.json (right): > > https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... > Source/devtools/protocol.json:1703: "type": "object", > Identifiers must be opaque strings, you should do parsing on the backend. > > https://codereview.chromium.org/1044203004/diff/190001/Source/modules/cachest... > File Source/modules/cachestorage/CacheStorage.h (right): > > https://codereview.chromium.org/1044203004/diff/190001/Source/modules/cachest... > Source/modules/cachestorage/CacheStorage.h:35: WebServiceWorkerCacheStorage* webCacheStorage() const { return m_webCacheStorage.get(); } > I thought this was no longer necessary. The problem with an opaque string is that we need to have some sort of delimiter to combine content. This is a bit hacky because cache names can be *any* string. What's wrong with having the object as an ID? The only reason I have a map on the JS-side is because you can't have custom object equality in a Set (this is in ES7 I believe). So I just use a map and JSONify the object for the key. That basically gives us a reliable opaque key that's not too hacky. If we went with the 'id' approach, I guess our cacheId object returned from the listCaches call could contain the opaque string that would then be used later for other calls. I could also redo the storage so it's a map again, from security origin to our internal cache data representation. Let me know what you think Daniel
> The problem with an opaque string is that we need to have some sort of delimiter > to combine content. This is a bit hacky because cache names can be *any* > string. origin is not any though, so origin|cache_name could work > What's wrong with having the object as an ID? - ID is a handle, you should not expose the internal of this handle over the protocol - Protocol multiplexors could intercept messages and decorate the ids, etc. > The only reason I have a map on the JS-side is because you can't have custom > object equality in a Set (this is in ES7 I believe). So I just use a map and > JSONify the object for the key. That basically gives us a reliable opaque key > that's not too hacky. > If we went with the 'id' approach, I guess our cacheId object returned from the > listCaches call could contain the opaque string that would then be used later > for other calls. I could also redo the storage so it's a map again, from > security origin to our internal cache data representation. > I don't think you should go with the 'id' approach, I'd prefer if ids were strings managed by the backend. You could either concat origin and name or manage a map from id to the cache metadata on the backend. > Let me know what you think > Daniel
On 2015/04/16 at 20:12:01, pfeldman wrote: > > The problem with an opaque string is that we need to have some sort of delimiter > > to combine content. This is a bit hacky because cache names can be *any* > > string. > > origin is not any though, so origin|cache_name could work > > > What's wrong with having the object as an ID? > > - ID is a handle, you should not expose the internal of this handle over the protocol > - Protocol multiplexors could intercept messages and decorate the ids, etc. > > > The only reason I have a map on the JS-side is because you can't have custom > > object equality in a Set (this is in ES7 I believe). So I just use a map and > > JSONify the object for the key. That basically gives us a reliable opaque key > > that's not too hacky. > > If we went with the 'id' approach, I guess our cacheId object returned from the > > listCaches call could contain the opaque string that would then be used later > > for other calls. I could also redo the storage so it's a map again, from > > security origin to our internal cache data representation. > > > > I don't think you should go with the 'id' approach, I'd prefer if ids were strings managed by the backend. You could either concat origin and name or manage a map from id to the cache metadata on the backend. > > > Let me know what you think > > Daniel Whoops, by 'id' approach I meant the opaque string. Ok, but we still need the name of the cache in the frontend for display purposes, and we don't want to dissect the string to get that, so we'll be returning an object with that opaque id and then the cache name (and possibly security origin, but this would be redundant) for each cache. Does that make sense? Do you prefer having these caches stored in a flattened way (a map of the opaque string -> cache info) or by first keying by the security origin? So it would then be securityOrigin => (array of cache info) I personally prefer the first approach, as we have nice set constrains on the caches. We could also do: securityOrigin => (cacheId => cacheInfo)
Please see my comments, I think with some minor changes, you can bring it to the state that matches my comments and is in line with how other models work in DevTools. Start looking at my comments from protocol.json. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:17: this._caches = new Map(); This would actually be totally fine if the key is an opaque cacheId string that came from the backend and the value is the Cache metadata that came from the backend (see protocol.json comments below). Flat map would work best for this. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... Source/devtools/protocol.json:1702: "id": "CacheId", Rename this to "Cache" https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... Source/devtools/protocol.json:1706: { "name": "securityOrigin", "type": "string", "description": "Security origin of the cache." }, You already have an origin, cache name here. Add cacheId of type CacharId (string) to use as a handle here.
Thanks! That was my favorite approach as well. This is how I feel about this patch: http://i.imgur.com/VjRDv3d.gif https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... File Source/devtools/front_end/resources/ResourcesPanel.js (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/resources/ResourcesPanel.js:1432: var target = WebInspector.targetManager.mainTarget(); On 2015/04/16 at 10:22:52, pfeldman wrote: > Nit: var target = this._storagePanel._target; Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/resources/ResourcesPanel.js:1458: var target = WebInspector.targetManager.mainTarget(); On 2015/04/16 at 10:22:52, pfeldman wrote: > ditto Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... File Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:17: this._caches = new Map(); On 2015/04/17 at 07:57:36, pfeldman wrote: > This would actually be totally fine if the key is an opaque cacheId string that came from the backend and the value is the Cache metadata that came from the backend (see protocol.json comments below). Flat map would work best for this. Awesome, that's what I was thinking. Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:48: for (var securityOrigin of securityOrigins) { On 2015/04/16 at 10:22:52, pfeldman wrote: > drop {} Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:91: for (var cache of this._caches.values()) { On 2015/04/16 at 10:22:52, pfeldman wrote: > drop {} Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/front_... Source/devtools/front_end/sdk/ServiceWorkerCacheModel.js:92: caches[i++] = cache; On 2015/04/16 at 10:22:53, pfeldman wrote: > We typically just .push for simplicity. Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... Source/devtools/protocol.json:1702: "id": "CacheId", On 2015/04/17 at 07:57:36, pfeldman wrote: > Rename this to "Cache" Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... Source/devtools/protocol.json:1703: "type": "object", On 2015/04/16 at 10:22:53, pfeldman wrote: > Identifiers must be opaque strings, you should do parsing on the backend. Done. https://codereview.chromium.org/1044203004/diff/190001/Source/devtools/protoc... Source/devtools/protocol.json:1706: { "name": "securityOrigin", "type": "string", "description": "Security origin of the cache." }, On 2015/04/17 at 07:57:36, pfeldman wrote: > You already have an origin, cache name here. Add cacheId of type CacharId (string) to use as a handle here. Cone. https://codereview.chromium.org/1044203004/diff/190001/Source/modules/cachest... File Source/modules/cachestorage/CacheStorage.h (right): https://codereview.chromium.org/1044203004/diff/190001/Source/modules/cachest... Source/modules/cachestorage/CacheStorage.h:35: WebServiceWorkerCacheStorage* webCacheStorage() const { return m_webCacheStorage.get(); } On 2015/04/16 at 10:22:53, pfeldman wrote: > I thought this was no longer necessary. Bad merge, removed.
lgtm % comments! https://codereview.chromium.org/1044203004/diff/230001/Source/devtools/protoc... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/230001/Source/devtools/protoc... Source/devtools/protocol.json:1731: { "name": "cacheId", "type": "string", "description": "An opaque unique id of the cache." } Move this above to make it the first field, introduce type "CacheId" that would inherit from "string" as we do in other places. https://codereview.chromium.org/1044203004/diff/230001/Source/devtools/protoc... Source/devtools/protocol.json:1743: { "name": "cacheIds", "type": "array", "items": { "$ref": "Cache" }, "description": "Cache ids for the security origin." } name: caches. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... File Source/modules/cachestorage/InspectorCacheStorageAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:46: String composeCacheId(const String& securityOrigin, const String& cacheName) super nit: we typically use build* and parse*, but that's fine. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:58: *errorString = "Cannot find the | character to decompose the cache id"; nit: you can just say that this was a poor id, no need to expose the fact that we concatenate strings. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:70: WebServiceWorkerCacheStorage* cache = Platform::current()->cacheStorage(identifier); Adopt right away, you'll still be able to check for nullptr the same way below. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:348: String securityOrigin, cacheName; Declaration per line, please! https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:353: OwnPtr<WebServiceWorkerCacheStorage> cache = assertCacheStorage(errorString, securityOrigin); Extract assertCacheStorageForId(errorString, cacheId) ?
https://codereview.chromium.org/1044203004/diff/230001/Source/devtools/protoc... File Source/devtools/protocol.json (right): https://codereview.chromium.org/1044203004/diff/230001/Source/devtools/protoc... Source/devtools/protocol.json:1731: { "name": "cacheId", "type": "string", "description": "An opaque unique id of the cache." } On 2015/04/20 at 18:14:31, pfeldman wrote: > Move this above to make it the first field, introduce type "CacheId" that would inherit from "string" as we do in other places. Done. https://codereview.chromium.org/1044203004/diff/230001/Source/devtools/protoc... Source/devtools/protocol.json:1743: { "name": "cacheIds", "type": "array", "items": { "$ref": "Cache" }, "description": "Cache ids for the security origin." } On 2015/04/20 at 18:14:31, pfeldman wrote: > name: caches. Done. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... File Source/modules/cachestorage/InspectorCacheStorageAgent.cpp (right): https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:46: String composeCacheId(const String& securityOrigin, const String& cacheName) On 2015/04/20 at 18:14:31, pfeldman wrote: > super nit: we typically use build* and parse*, but that's fine. Done. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:58: *errorString = "Cannot find the | character to decompose the cache id"; On 2015/04/20 at 18:14:31, pfeldman wrote: > nit: you can just say that this was a poor id, no need to expose the fact that we concatenate strings. Done. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:70: WebServiceWorkerCacheStorage* cache = Platform::current()->cacheStorage(identifier); On 2015/04/20 at 18:14:31, pfeldman wrote: > Adopt right away, you'll still be able to check for nullptr the same way below. Done. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:348: String securityOrigin, cacheName; On 2015/04/20 at 18:14:31, pfeldman wrote: > Declaration per line, please! Done. https://codereview.chromium.org/1044203004/diff/230001/Source/modules/cachest... Source/modules/cachestorage/InspectorCacheStorageAgent.cpp:353: OwnPtr<WebServiceWorkerCacheStorage> cache = assertCacheStorage(errorString, securityOrigin); On 2015/04/20 at 18:14:31, pfeldman wrote: > Extract assertCacheStorageForId(errorString, cacheId) ? Done.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from horo@chromium.org, pfeldman@chromium.org Link to the patchset: https://codereview.chromium.org/1044203004/#ps250001 (title: "comments and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1044203004/250001
Message was sent while issue was closed.
Committed patchset #14 (id:250001) as https://src.chromium.org/viewvc/blink?view=rev&revision=194060
Message was sent while issue was closed.
thakis@chromium.org changed reviewers: + thakis@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1044203004/diff/250001/Source/core/event_idl_... File Source/core/event_idl_files_list.tmp (right): https://codereview.chromium.org/1044203004/diff/250001/Source/core/event_idl_... Source/core/event_idl_files_list.tmp:10: events/CustomEvent.idl Did you mean to check in this file?
Message was sent while issue was closed.
On 2015/04/23 at 01:10:26, thakis wrote: > https://codereview.chromium.org/1044203004/diff/250001/Source/core/event_idl_... > File Source/core/event_idl_files_list.tmp (right): > > https://codereview.chromium.org/1044203004/diff/250001/Source/core/event_idl_... > Source/core/event_idl_files_list.tmp:10: events/CustomEvent.idl > Did you mean to check in this file? Whoops, no. Patch for deletion: https://codereview.chromium.org/1082723010 |
