Issue 6241284884791296: issue #660 - Switch to version 2 of the HTTP cache API (Closed)
|
Created:
July 1, 2014, 8:12 a.m. by saroyanm Modified:
July 3, 2014, 5:18 p.m. Reviewers:
Wladimir Palant Visibility:
Public. |
DescriptionThis issue is related to current ticket:
https://issues.adblockplus.org/ticket/660
Patch Set 1 #
Total comments: 20
Patch Set 2 : #
Total comments: 5
Patch Set 3 : #MessagesTotal messages: 7
Wladimir can you please have a look. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:31: var cacheStorageSession = null; Not sure if I should use more general name for this variable. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:303: E("tooltipPreviewBox").hidden = true; Moved tooltip IMG wrapper here because, during the test in Firefox Nightly (FF V33.0a1) it's seems like that the cacheStorageSession.asyncOpenURI works synchronous, while in other versions (FF V30 and V31b6) method works async.
http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:22: ({LoadContextInfo}) = Cu.import("resource://gre/modules/LoadContextInfo.jsm", {}); Please use null rather than {} for the context parameter, no point having a temporary object here. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:31: var cacheStorageSession = null; This variable name is now a mix of the old and new interface names (nsICacheSession and nsICacheStorage). Given that the backwards compat code will be removed eventually I'd vote for cacheStorage as variable name here. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:308: //Use cache V2 API from Gecko 32 This implies that a) we will always use Cache v2 API and b) that this API was introduced in Gecko 32. Both is wrong, please change that comment into: // Cache v2 API is enabled by default starting with Gecko 32 I wonder whether we should do feature detection rather than hardcoding the version here (as done in http://hg.mozilla.org/mozilla-central/annotate/18c21532848a/netwerk/test/unit...). However, feature detection doesn't seem to be any more reliable in this case. The approach there looks like it might stop working in future and cause issues for us. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:312: cacheStorageSession = cacheService.diskCacheStorage(LoadContextInfo.default, false); You only need LoadContextInfo here, meaning that you can load it here - unconditionally. Question: will this also work to look up cache entries only stored in memory (images with Cache-Control: no-store header)? This needs to be tested. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:340: }; I don't think that using the same listener for both APIs makes sense, the interfaces are very different. For example, nsICacheEntry.close() is deprecated, that's V1 API. Also, the parameter names for onCacheEntryAvailable are wrong now. In other words: function showPreview() { E("tooltipPreview").setAttribute("src", item.location); E("tooltipPreviewBox").hidden = false; } ... cacheStorage.asyncOpenURI(..., { onCacheEntryCheck: function (entry, appCache) { return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED; }, onCacheEntryAvailable: function (entry, isNew, appCache, status) { if (!isNew) showPreview(); } }); ... cacheStorage.asyncOpenCacheEntry(..., { onCacheEntryAvailable: function(descriptor, accessGranted, status) { if (!descriptor) return; descriptor.close(); showPreview(); }, onCacheEntryDoomed: function(status) { } }); http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:345: if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0) Please don't duplicate the branch condition here. Instead you can check the interface: if (cacheStorageSession instanceof Ci.nsICacheStorage)
http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:312: cacheStorageSession = cacheService.diskCacheStorage(LoadContextInfo.default, false); From the source code it seems that diskCacheStorage will allow us to look up both in disk and memory storage. We might want to set the second parameter to true however, to look up in app cache as well.
Uploaded a new patch. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:22: ({LoadContextInfo}) = Cu.import("resource://gre/modules/LoadContextInfo.jsm", {}); On 2014/07/01 11:58:33, Wladimir Palant wrote: > Please use null rather than {} for the context parameter, no point having a > temporary object here. Done. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:31: var cacheStorageSession = null; On 2014/07/01 11:58:33, Wladimir Palant wrote: > This variable name is now a mix of the old and new interface names > (nsICacheSession and nsICacheStorage). Given that the backwards compat code will > be removed eventually I'd vote for cacheStorage as variable name here. Done. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:308: //Use cache V2 API from Gecko 32 On 2014/07/01 11:58:33, Wladimir Palant wrote: > This implies that a) we will always use Cache v2 API and b) that this API was > introduced in Gecko 32. Both is wrong, please change that comment into: > > // Cache v2 API is enabled by default starting with Gecko 32 > > I wonder whether we should do feature detection rather than hardcoding the > version here (as done in > http://hg.mozilla.org/mozilla-central/annotate/18c21532848a/netwerk/test/unit...). > However, feature detection doesn't seem to be any more reliable in this case. > The approach there looks like it might stop working in future and cause issues > for us. Thought to use hardcoding solution after reading comments in ticket. Not sure if it's good approach, but maybe we can check if the service class exist in Components.classes: if (Cc["@mozilla.org/netwerk/cache-storage-service;1"]) Also maybe we can check similarly for getService and diskCacheStorage methods. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:312: cacheStorageSession = cacheService.diskCacheStorage(LoadContextInfo.default, false); On 2014/07/01 12:00:46, Wladimir Palant wrote: > From the source code it seems that diskCacheStorage will allow us to look up > both in disk and memory storage. We might want to set the second parameter to > true however, to look up in app cache as well. I tried to avoid loading LoadContextInfo for each item, thought could cause performance issue, Can you please give your thought about that ? Hope I understood your comment correctly. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:340: }; On 2014/07/01 11:58:33, Wladimir Palant wrote: > I don't think that using the same listener for both APIs makes sense, the > interfaces are very different. For example, nsICacheEntry.close() is deprecated, > that's V1 API. Also, the parameter names for onCacheEntryAvailable are wrong > now. In other words: > > function showPreview() > { > E("tooltipPreview").setAttribute("src", item.location); > E("tooltipPreviewBox").hidden = false; > } > > ... > cacheStorage.asyncOpenURI(..., { > onCacheEntryCheck: function (entry, appCache) > { > return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED; > }, > onCacheEntryAvailable: function (entry, isNew, appCache, status) { > if (!isNew) > showPreview(); > } > }); > ... > cacheStorage.asyncOpenCacheEntry(..., { > onCacheEntryAvailable: function(descriptor, accessGranted, status) > { > if (!descriptor) > return; > descriptor.close(); > showPreview(); > }, > onCacheEntryDoomed: function(status) > { > } > }); What you think do we need onCacheEntryDoomed here ? http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:345: if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0) On 2014/07/01 11:58:33, Wladimir Palant wrote: > Please don't duplicate the branch condition here. Instead you can check the > interface: > > if (cacheStorageSession instanceof Ci.nsICacheStorage) Done.
http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:308: //Use cache V2 API from Gecko 32 Unfortunately, checking for component existence doesn't help - the component was added a while ago, it isn't enabled however. So we'll have to live with the version check here. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:312: cacheStorageSession = cacheService.diskCacheStorage(LoadContextInfo.default, false); On 2014/07/01 16:45:47, saroyanm wrote: > I tried to avoid loading LoadContextInfo for each item, thought could cause > performance issue, It won't be loaded for each item - setting the cacheStorage variable is a one-time thing, and that's the only occasion where you need to load LoadContextInfo. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:340: }; On 2014/07/01 16:45:47, saroyanm wrote: > What you think do we need onCacheEntryDoomed here ? Yes, it is part of the interface - we might get error messages if we don't implement it. http://codereview.adblockplus.org/6241284884791296/diff/5685265389584384/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5685265389584384/chro... chrome/content/ui/sidebar.js:308: cacheStorage = cacheService.diskCacheStorage(LoadContextInfo.default, true); Apparently, this interface was added to Services.jsm (https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Serv...), we should use it. Please import Services.jsm and use Services.cache2 here. http://codereview.adblockplus.org/6241284884791296/diff/5685265389584384/chro... chrome/content/ui/sidebar.js:313: cacheStorage = cacheService.createSession("HTTP", Ci.nsICache.STORE_ANYWHERE, true); We should use Services.cache here. http://codereview.adblockplus.org/6241284884791296/diff/5685265389584384/chro... chrome/content/ui/sidebar.js:321: } Missing semicolon after } here.
New patch uploaded http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:312: cacheStorageSession = cacheService.diskCacheStorage(LoadContextInfo.default, false); On 2014/07/02 05:00:03, Wladimir Palant wrote: > On 2014/07/01 16:45:47, saroyanm wrote: > > I tried to avoid loading LoadContextInfo for each item, thought could cause > > performance issue, > > It won't be loaded for each item - setting the cacheStorage variable is a > one-time thing, and that's the only occasion where you need to load > LoadContextInfo. Ohh yes sure, thanks.. http://codereview.adblockplus.org/6241284884791296/diff/5629499534213120/chro... chrome/content/ui/sidebar.js:340: }; On 2014/07/02 05:00:03, Wladimir Palant wrote: > On 2014/07/01 16:45:47, saroyanm wrote: > > What you think do we need onCacheEntryDoomed here ? > > Yes, it is part of the interface - we might get error messages if we don't > implement it. I see. http://codereview.adblockplus.org/6241284884791296/diff/5685265389584384/chro... File chrome/content/ui/sidebar.js (right): http://codereview.adblockplus.org/6241284884791296/diff/5685265389584384/chro... chrome/content/ui/sidebar.js:308: cacheStorage = cacheService.diskCacheStorage(LoadContextInfo.default, true); On 2014/07/02 05:00:03, Wladimir Palant wrote: > Apparently, this interface was added to Services.jsm > (https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Serv...), > we should use it. Please import Services.jsm and use Services.cache2 here. Good point. http://codereview.adblockplus.org/6241284884791296/diff/5685265389584384/chro... chrome/content/ui/sidebar.js:321: } On 2014/07/02 05:00:03, Wladimir Palant wrote: > Missing semicolon after } here. Done.
LGTM 
|
