From 57dd2e1c1aea6fcfe68e6bb1539c62ae4ddefdbd Mon Sep 17 00:00:00 2001 From: Matěj Cepl Date: Sat, 24 Sep 2011 11:47:40 +0200 Subject: Starting to work on incorporating myk's code review --- lib/cached-request.js | 136 +++++++++++++++++++++++++++++++++----------------- 1 file 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(); }; -- cgit