Starting to work on incorporating myk's code reviewcachedRequest

1 files changed, 90 insertions, 46 deletions

diff --git a/lib/cached-request.js b/lib/cached-request.js
index fa6c592..f12365e 100644
--- a/lib/cached-request.js
+++ b/lib/cached-request.js
@@ -25,6 +25,14 @@ function debug(str) {
  */
 function CachedResponse (url) {
   if (!myStorage.cachedRequest[url]) {
+    // FIXME myk's review comment
+    // This creates a record in the datastore for a URL if one doesn't exist,
+    // but it doesn't initialize the record.  So if some code calls
+    // CachedResponse["http://example.com/"] twice without calling
+    // CachedResponse.saveCached() in between, it'll get different results the
+    // second time.  Better to avoid creating the record until
+    // CachedResponse.savedCached() is called (alternately: initialize the record
+    // when creating it).
     myStorage.cachedRequest[url] = {};
     this.url = url;
     this.text = null;
@@ -93,6 +101,12 @@ CachedResponse.prototype.__defineGetter__("lastModified", function() {
  *
  * Limitations: does only GET requests, so it doesn't even have
  *   .post(), .get() or any other methods.
+FIXME myk's request
+I would still require consumers to call .get(), since the constructor's name
+suggests it is a subclass of Request, and hewing to the original API (except
+for the methods that aren't implemented) seems like the most understandable 
+implementation of such a subclass.
+
  * Contrary to Request() opts can have onError callback, and
  * getExpiredAnyway property to allow using expired cached value,
  * if the remote connection returns error.
@@ -103,55 +117,85 @@ exports.CachedRequest = function CachedRequest(opts) {
   var req = new xhrMod.XMLHttpRequest();
   req.open("HEAD", opts.url, true);
   req.onreadystatechange = function () {
-    if (req.readyState == 4) {
-      if(req.status == 200) {
-        var curETag = req.getResponseHeader("ETag");
-        var curLastModified = new Date(req.getResponseHeader("Last-Modified"));
-        if (crStorage && crStorage.headers &&
-             ((curETag == crStorage.headers.eTag) ||
-             (curLastModified <= crStorage.lastModified))) {
-          debug("Loading from cache!");
-          // use cached values
-        }
-        else { // cache is not up-to-date
-          new Request({
-            url: opts.url,
-            onComplete: function(resp) {
-              if (resp.status == 200) {
-                debug("Too old cache! Reloaded");
-                crStorage.headers = resp.headers;
-                crStorage.text = resp.text;
-                crStorage.status = resp.status;
-                crStorage.statusText = resp.statusText;
-                crStorage.saveCached();
-              }
-              else {
-                // If we cannot get response from the real URL,
-                // we may be happy with getting our fix from
-                // anywhere, including (possibly) expired cache
-                if (opts.getExpiredAnyway && crStorage && crStorage.text) {
-                  debug("Nothing better to do! Returning expired cache.");
-                }
-                else {
-                  // We definitively lost, just call .onComplete
-                  // with what we have.
-                  opts.onComplete({
-                    text: resp.text,
-                    json: null,
-                    status: resp.status,
-                    statusText: resp.statusText,
-                    headers: resp.headers
-                  });
-                  // to avoid call opts.onComplete below
-                  return ;
-                }
-              }
+    if ((req.readyState != 4) || (req.status != 200)) {
+      return ;
+    }
+    var curETag = req.getResponseHeader("ETag");
+    // FIXME myk's review
+    // In bug 643156, bz discouraged use of HEAD requests due to server-side
+    // bugs and suggested using nsIURIChecker instead, which itself does a HEAD
+    // request for HTTP URLs but apparently works around those bugs.  I'm not
+    // sure if you can use that interface here, but it would be worth looking
+    // into.
+    // http://www.oxymoronical.com/experiments/apidocs/interface/nsIURIChecker
+    /*
+     * var linkChecker = Cc["@mozilla.org/network/urichecker;1"].
+     *                   createInstance(Ci.nsIURIChecker);
+     * linkChecker.init(ioService.newURI(url, null, null));
+     * linkChecker.loadFlags = Ci.nsIRequest.LOAD_BACKGROUND;
+     * linkChecker.asyncCheck(new AutoDownloader(url, savedTo, aWindow), null);
+     *
+     * or
+     * var linkChecker = gLinksBeingChecked[i].
+     *     QueryInterface(Components.interfaces.nsIURIChecker);
+     * // nsIURIChecker returns:
+     * // NS_BINDING_SUCCEEDED     link is valid
+     * // NS_BINDING_FAILED        link is invalid (gave an error)
+     * // NS_BINDING_ABORTED       timed out, or cancelled
+     * var status = linkChecker.status;
+     * if (status ==0x804b0001)        // NS_BINDING_FAILED
+     *   dump(">> " + linkChecker.name + " is broken\n");
+     * else if (status == 0x804b0002)   // NS_BINDING_ABORTED
+     *   dump(">> " + linkChecker.name + " timed out\n");
+     * else if (status == 0)             // NS_BINDING_SUCCEEDED
+     *   dump("   " + linkChecker.name + " OK!\n");
+     * else
+     *   dump(">> " + linkChecker.name + " not checked\n");
+     */
+    var curLastModified = new Date(req.getResponseHeader("Last-Modified"));
+    if (crStorage && crStorage.headers &&
+         ((curETag == crStorage.headers.eTag) ||
+         (curLastModified <= crStorage.lastModified))) {
+      debug("Loading from cache!");
+      // use cached values
+    }
+    else { // cache is not up-to-date
+      new Request({
+        url: opts.url,
+        onComplete: function(resp) {
+          if (resp.status == 200) {
+            debug("Too old cache! Reloaded");
+            crStorage.headers = resp.headers;
+            crStorage.text = resp.text;
+            crStorage.status = resp.status;
+            crStorage.statusText = resp.statusText;
+            crStorage.saveCached();
+          }
+          else {
+            // If we cannot get response from the real URL,
+            // we may be happy with getting our fix from
+            // anywhere, including (possibly) expired cache
+            if (opts.getExpiredAnyway && crStorage && crStorage.text) {
+              debug("Nothing better to do! Returning expired cache.");
+            }
+            else {
+              // We definitively lost, just call .onComplete
+              // with what we have.
+              opts.onComplete({
+                text: resp.text,
+                json: null,
+                status: resp.status,
+                statusText: resp.statusText,
+                headers: resp.headers
+              });
+              // to avoid call opts.onComplete below
+              return ;
             }
-          }).get();
+          }
         }
-        opts.onComplete(crStorage);
-      }
+      }).get();
     }
+    opts.onComplete(crStorage);
   };
   req.send();
 };