Implement offline mode behind a flag

content/browser/loader/offline_manager.h

+// Copyright (c) 2013 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+//
+// This class manages the switch between offline and online for resource

[darin (slow to review), 2013/03/27 18:33:59]

I think it'd be helpful to describe the goal of this class, which is to remember
if requests for a particular route (e.g., "tab") is unable to reach the server,
so that other requests made with the same route will just be loaded from cache,
etc.

[Randy Smith (Not in Mondays), 2013/04/03 19:20:34]

Hmmm.  I had considered that in the realm of policy, i.e. I was trying to design
this class so that it could implement that behavior (as current) or some other
behavior that we came up with in the future.  I'll add that behavior as an
example; let me know what you think.

+// loading.
+
+#ifndef CONTENT_BROWSER_LOADER_OFFLINE_MANAGER
+#define CONTENT_BROWSER_LOADER_OFFLINE_MANAGER
+
+#include <map>
+
+#include "base/basictypes.h"
+
+struct ResourceHostMsg_Request;
+
+namespace net {
+class HttpResponseInfo;
+class URLRequest;
+}
+
+namespace content {
+
+class OfflineManager {

[darin (slow to review), 2013/03/27 18:33:59]

nit: manager is such a generic term.  I wonder if "policy" would be helpful
here.  OfflinePolicy?

[Randy Smith (Not in Mondays), 2013/04/03 19:20:34]

Done.

+ public:
+  OfflineManager();
+  ~OfflineManager();
+
+  // Return any additional load flags to be |'d for the current request.
+  int OfflineLoadFlags(int child_id, int route_id,

[darin (slow to review), 2013/03/27 18:33:59]

nit: maybe call this GetAdditionalLoadFlags?  What if conflicting load flags are
already set for the request?  Do we need to clean them out?

I could imagine calling this method RequestStarting, which would pass a mutable
URLRequest allowing the load flags to be manipulated.  Maybe that is giving the
OfflinePolicy class too much access to the URLRequest.  I suggested it for
consistency with the RequestCompleted class.

Note:  You can get the child_id and route_id from the URLRequest too, but maybe
they are better as explicit parameters.

[Randy Smith (Not in Mondays), 2013/04/03 19:20:34]

Done.
I think, and have implemented, that conflicting load flags win; we don't want to
second-guess the explicit request of the consumer.
Yeah, I wince at passing a mutable URLRequest; I much prefer interfaces where
there inputs and outputs are clear and "minimal".  

After chewing on the interface design question, what I"ve come up with is:
* I'll pass in the ResourceType::Type, which is what I was looking for from the
request_data.
* I'll also pass in the current load flags (so that we can detect the conflict
case).
* I won't pass in the URLRequest either here or in RequestCompleted() (that gets
the response info, and is dependent on what Ricardo thinks of the flag I added
to the response info).

Let me know what you think.
Yeah, I'm inclined to keep them as explicit parameters, for the "clear inputs
and outputs" reason above.  If the arg list gets too long, I'll rethink that.

+                       const ResourceHostMsg_Request& request_data);
+
+  // Incorporate online/offline information from a completed request.
+  // Note that the response info must be available through
+  // |request->response_info()|.
+  void RequestCompleted(int child_id, int route_id,
+                        const net::URLRequest* request);
+
+  void RouteDeleted(int child_id, int route_id);

[darin (slow to review), 2013/03/27 18:33:59]

should there be an explicit RouteAdded?  RouteDeleted -> RouteRemoved?

[Randy Smith (Not in Mondays), 2013/04/03 19:20:34]

Sorry to reflect this back, but I'd be inclined to go with your judgement. 
RouteDeleted() is here for garbage collection/leak avoidance only, and I don't
know if there any way to detect in RDHI that we've added a route until we start
getting message from it.  Is there one?  I'd be happy to have RouteAdded() just
for symmetry if there's some easy way to hook it in; if not, I don't think it's
worth creating a way to hook it in.
Done.

+
+private:
+  typedef enum { ROUTE_INIT, ROUTE_ONLINE, ROUTE_OFFLINE } RouteOfflineState;
+
+  // child_id, route_id -> state.
+  typedef std::map<std::pair<int, int>, RouteOfflineState> RouteMap;
+
+  RouteMap offline_state_map_;
+
+  DISALLOW_COPY_AND_ASSIGN(OfflineManager);
+};
+
+}  // namespace content
+
+#endif  // CONTENT_BROWSER_LOADER_OFFLINE_MANAGER