From 78ea88d757ecf3a0a75b786ae343d746617ba46b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Konstantin=20K=C3=A4fer?= <mail@kkaefer.com>
Date: Mon, 9 Oct 2017 12:59:27 +0200
Subject: [core] make forcing cache/network only more explicit

Previously, we used the existence of a `prior*` field in the Resource object as an indication for whether we should consult the cache or not. However, this is prone to error, since a failed cache lookup won't set any prior fields. Therefore, we manually set `priorExpires` to 0. This in turn triggered another bug where generated wrong expiration timestamps when the server response we got was expired (or expired between sending and receiving).

This commit changes the flags so that we can now explicitly request CacheOnly/NetworkOnly (or All) loading methods, rather than the implicit Optional/Required naming scheme.
---
 platform/default/default_file_source.cpp | 50 ++++++++++++++------------------
 1 file changed, 22 insertions(+), 28 deletions(-)

(limited to 'platform/default/default_file_source.cpp')

diff --git a/platform/default/default_file_source.cpp b/platform/default/default_file_source.cpp
index 3fdb03e6b4..608b782ab9 100644
--- a/platform/default/default_file_source.cpp
+++ b/platform/default/default_file_source.cpp
@@ -126,47 +126,41 @@ public:
             tasks[req] = localFileSource->request(resource, callback);
         } else {
             // Try the offline database
-            const bool hasPrior = resource.priorEtag || resource.priorModified ||
-                                  resource.priorExpires || resource.priorData;
-            if (!hasPrior || resource.necessity == Resource::Optional) {
+            if (resource.hasLoadingMethod(Resource::LoadingMethod::Cache)) {
                 auto offlineResponse = offlineDatabase->get(resource);
 
-                if (resource.necessity == Resource::Optional && !offlineResponse) {
-                    // Ensure there's always a response that we can send, so the caller knows that
-                    // there's no optional data available in the cache.
-                    offlineResponse.emplace();
-                    offlineResponse->noContent = true;
-                    offlineResponse->error = std::make_unique<Response::Error>(
-                            Response::Error::Reason::NotFound, "Not found in offline database");
-                }
-
-                if (offlineResponse) {
+                if (resource.loadingMethod == Resource::LoadingMethod::CacheOnly) {
+                    if (!offlineResponse) {
+                        // Ensure there's always a response that we can send, so the caller knows that
+                        // there's no optional data available in the cache, when it's the only place
+                        // we're supposed to load from.
+                        offlineResponse.emplace();
+                        offlineResponse->noContent = true;
+                        offlineResponse->error = std::make_unique<Response::Error>(
+                                Response::Error::Reason::NotFound, "Not found in offline database");
+                    } else if (!offlineResponse->isUsable()) {
+                        // Don't return resources the server requested not to show when they're stale.
+                        // Even if we can't directly use the response, we may still use it to send a
+                        // conditional HTTP request, which is why we're saving it above.
+                        offlineResponse->error = std::make_unique<Response::Error>(
+                            Response::Error::Reason::NotFound, "Cached resource is unusable");
+                    }
+                    callback(*offlineResponse);
+                } else if (offlineResponse) {
+                    // Copy over the fields so that we can use them when making a refresh request.
                     resource.priorModified = offlineResponse->modified;
                     resource.priorExpires = offlineResponse->expires;
                     resource.priorEtag = offlineResponse->etag;
+                    resource.priorData = offlineResponse->data;
 
-                    // Don't return resources the server requested not to show when they're stale.
-                    // Even if we can't directly use the response, we may still use it to send a
-                    // conditional HTTP request.
                     if (offlineResponse->isUsable()) {
                         callback(*offlineResponse);
-                    } else if (resource.necessity == Resource::Optional) {
-                        // Instead of the data that we got, return a not found error so that
-                        // underlying implementations know about the fact that we couldn't find
-                        // usable cache data.
-                        offlineResponse->error = std::make_unique<Response::Error>(
-                            Response::Error::Reason::NotFound, "Cached resource is unusable");
-                        callback(*offlineResponse);
-                    } else {
-                        // Since we can't return the data immediately, we'll have to hold on so that
-                        // we can return it later in case we get a 304 Not Modified response.
-                        resource.priorData = offlineResponse->data;
                     }
                 }
             }
 
             // Get from the online file source
-            if (resource.necessity == Resource::Required) {
+            if (resource.hasLoadingMethod(Resource::LoadingMethod::Network)) {
                 tasks[req] = onlineFileSource.request(resource, [=] (Response onlineResponse) mutable {
                     this->offlineDatabase->put(resource, onlineResponse);
                     callback(onlineResponse);
-- 
cgit v1.2.1