issue #660 - Switch to version 2 of the HTTP cache API

chrome/content/ui/sidebar.js

Index: chrome/content/ui/sidebar.js
===================================================================
--- a/chrome/content/ui/sidebar.js
+++ b/chrome/content/ui/sidebar.js
@@ -16,6 +16,11 @@
  */
 
 Cu.import("resource://gre/modules/XPCOMUtils.jsm");
+let LoadContextInfo = null;
+try
+{
+  ({LoadContextInfo}) = Cu.import("resource://gre/modules/LoadContextInfo.jsm", {});

[Wladimir Palant, 2014/07/01 11:58:33]

Please use null rather than {} for the context parameter, no point having a
temporary object here.

[saroyanm, 2014/07/01 16:45:47]

Done.

+} catch (e) {}
 
 // Main browser window
 var mainWin = parent;
@@ -23,7 +28,7 @@
 // The window handler currently in use
 var requestNotifier = null;
 
-var cacheSession = null;
+var cacheStorageSession = null;

[saroyanm, 2014/07/01 08:37:37]

Not sure if I should use more general name for this variable.

[Wladimir Palant, 2014/07/01 11:58:33]

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.

[saroyanm, 2014/07/01 16:45:47]

Done.

 var noFlash = false;
 
 // Matcher for disabled filters
@@ -295,43 +300,58 @@
   var showPreview = Prefs.previewimages && !("tooltip" in item);
   showPreview = showPreview && item.typeDescr == "IMAGE";
   showPreview = showPreview && (!item.filter || item.filter.disabled || item.filter instanceof WhitelistFilter);
+  E("tooltipPreviewBox").hidden = true;

[saroyanm, 2014/07/01 08:37:37]

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.

   if (showPreview)
   {
-    // Check whether image is in cache (stolen from ImgLikeOpera)
-    if (!cacheSession)
+    if (!cacheStorageSession)
     {
-      var cacheService = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
-      cacheSession = cacheService.createSession("HTTP", Ci.nsICache.STORE_ANYWHERE, true);
+      //Use cache V2 API from Gecko 32

[Wladimir Palant, 2014/07/01 11:58:33]

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.

[saroyanm, 2014/07/01 16:45:47]

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.

[Wladimir Palant, 2014/07/02 05:00:03]

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.

+      if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0)
+      {
+        let cacheService = Cc["@mozilla.org/netwerk/cache-storage-service;1"].getService(Ci.nsICacheStorageService);
+        cacheStorageSession = cacheService.diskCacheStorage(LoadContextInfo.default, false);

[Wladimir Palant, 2014/07/01 11:58:33]

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.

[Wladimir Palant, 2014/07/01 12:00:46]

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.

[saroyanm, 2014/07/01 16:45:47]

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.

[Wladimir Palant, 2014/07/02 05:00:03]

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.

[saroyanm, 2014/07/02 07:03:41]

Ohh yes sure, thanks..

+      }
+      else
+      {
+        let cacheService = Cc["@mozilla.org/network/cache-service;1"].getService(Ci.nsICacheService);
+        cacheStorageSession = cacheService.createSession("HTTP", Ci.nsICache.STORE_ANYWHERE, true);
+      }
     }
 
     let cacheListener =
     {
-       onCacheEntryAvailable: function(descriptor, accessGranted, status)
+       onCacheEntryAvailable: function(entry, accessGranted, status)
        {
-          if (!descriptor)
+          if (!entry)
             return;
 
-          descriptor.close();
+          entry.close();
           // Show preview here since this is asynchronous now
-          // and we have a valid descriptor
+          // and we have a valid entry
           E("tooltipPreview").setAttribute("src", item.location);
           E("tooltipPreviewBox").hidden = false;
        },
+       onCacheEntryCheck: function (entry, appcache) {
+         return Ci.nsICacheEntryOpenCallback.ENTRY_WANTED;
+       },
        onCacheEntryDoomed: function(status)
        {
        }
     };

[Wladimir Palant, 2014/07/01 11:58:33]

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)
    {
    }
  });

[saroyanm, 2014/07/01 16:45:47]

What you think do we need onCacheEntryDoomed here ?

[Wladimir Palant, 2014/07/02 05:00:03]

Yes, it is part of the interface - we might get error messages if we don't
implement it.

[saroyanm, 2014/07/02 07:03:41]

I see.

+    
     try
     {
-      cacheSession.asyncOpenCacheEntry(item.location, Ci.nsICache.ACCESS_READ, cacheListener);
+      //Use cache V2 API from Gecko 32
+      if (Services.vc.compare(Utils.platformVersion, "32.0") >= 0)

[Wladimir Palant, 2014/07/01 11:58:33]

Please don't duplicate the branch condition here. Instead you can check the
interface:

if (cacheStorageSession instanceof Ci.nsICacheStorage)

[saroyanm, 2014/07/01 16:45:47]

Done.

+        cacheStorageSession.asyncOpenURI(Utils.makeURI(item.location), "", Ci.nsICacheStorage.OPEN_READONLY, cacheListener);
+      else
+        cacheStorageSession.asyncOpenCacheEntry(item.location, Ci.nsICache.ACCESS_READ, cacheListener);
     }
     catch (e)
     {
       Cu.reportError(e);
     }
   }
-
-  E("tooltipPreviewBox").hidden = true;
 }
 
 const visual = {