Implement offline mode behind a flag

Implement offline mode behind a flag.

BUG=2204
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=195374

[Randy Smith (Not in Mondays), 2013-03-22 21:03:16]

Darin: Could you give me your opinion of the approach taken in this CL?  It's
not yet ready for landing, but I think that's primarily that I need to add
tests.  But if I'm going the wrong way, I'd like to know that.

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

just some initial thoughts...

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
File content/browser/loader/offline_manager.cc (right):

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.cc:19: int child_id, int route_id, const
ResourceHostMsg_Request& request_data) {
how will you use the request_data parameter?

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
File content/browser/loader/offline_manager.h (right):

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:5: // This class manages the switch
between offline and online for resource
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.

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:24: class OfflineManager {
nit: manager is such a generic term.  I wonder if "policy" would be helpful
here.  OfflinePolicy?

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:30: int OfflineLoadFlags(int child_id,
int route_id,
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.

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:39: void RouteDeleted(int child_id, int
route_id);
should there be an explicit RouteAdded?  RouteDeleted -> RouteRemoved?

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

Darin: Incorporated your comments.  Gonna work on tests next; you're welcome to
respond now, or after I have a test in place.

Ricardo: Could I get early feedback from you on the new flag?  I'm specifically
wondering if that's the right place to put setting it to true, but I'd love
feedback and the idea as a whole as well.

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
File content/browser/loader/offline_manager.cc (right):

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.cc:19: int child_id, int route_id, const
ResourceHostMsg_Request& request_data) {
On 2013/03/27 18:33:59, darin wrote:
> how will you use the request_data parameter?

Ooops :-}.  I original put it in so that I'd have access to the MAIN_FRAME
parameter, but I'm not using that, but I should be.  I think based on the other
conversations we've had, I'm going to modify this arg to be
request_data.resource_type.

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
File content/browser/loader/offline_manager.h (right):

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:5: // This class manages the switch
between offline and online for resource
On 2013/03/27 18:33:59, darin wrote:
> 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.

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.

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:24: class OfflineManager {
On 2013/03/27 18:33:59, darin wrote:
> nit: manager is such a generic term.  I wonder if "policy" would be helpful
> here.  OfflinePolicy?

Done.

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:30: int OfflineLoadFlags(int child_id,
int route_id,
On 2013/03/27 18:33:59, darin wrote:
> nit: maybe call this GetAdditionalLoadFlags?  

Done.

> What if conflicting load flags are
> already set for the request?  Do we need to clean them out?

I think, and have implemented, that conflicting load flags win; we don't want to
second-guess the explicit request of the consumer.

> 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.

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.

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

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.

https://codereview.chromium.org/12886022/diff/20001/content/browser/loader/of...
content/browser/loader/offline_manager.h:39: void RouteDeleted(int child_id, int
route_id);
On 2013/03/27 18:33:59, darin wrote:
> should there be an explicit RouteAdded?  

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.

> RouteDeleted -> RouteRemoved?

Done.

[rvargas (doing something else), 2013-04-03 21:48:19]

sounds reasonable.

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
content/browser/loader/offline_policy.h:5: // This class controls under what
conditions resources will be fetched
nit: move the comment to just before the class.

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
content/browser/loader/offline_policy.h:33: // Return any additional load flags
to be |'d for the current request.
nit: ORed ? OR'd ? added?. By the way, flags should be unsigned ints (but I see
that the caller is currently using ints :(  )

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
content/browser/loader/offline_policy.h:44: typedef enum { ROUTE_INIT,
ROUTE_ONLINE, ROUTE_OFFLINE } RouteOfflineState;
nit: enum RouteOfflineState {}

https://codereview.chromium.org/12886022/diff/44002/net/http/http_response_in...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/12886022/diff/44002/net/http/http_response_in...
net/http/http_response_info.h:69: bool network_verified;
We have two options:
1. Have a signal about network connectivity. (network_was_accessed,
was_network_fetched, network_used etc)
2. Have a signal about caching details.
(cache_verified or something like that)

I'm inclined to go with the first option, and in that case, the flag should be
set by the network transaction, not the cache... and that should be a more
reliable indicator (as in not everything goes through the cache).

[Randy Smith (Not in Mondays), 2013-04-04 19:42:49]

Ricardo: One question for you below.

Status: Have OfflinePolicy unit tests, haven't written unit tests for new
response info variable yet.  Will repost when ready for final review.

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
content/browser/loader/offline_policy.h:5: // This class controls under what
conditions resources will be fetched
On 2013/04/03 21:48:19, rvargas wrote:
> nit: move the comment to just before the class.

Done.

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
content/browser/loader/offline_policy.h:33: // Return any additional load flags
to be |'d for the current request.
On 2013/04/03 21:48:19, rvargas wrote:
> nit: ORed ? OR'd ? added?. 

Done.

> By the way, flags should be unsigned ints (but I see
> that the caller is currently using ints :(  )

I was going to just fix this in RDHI, but
URLRequest::load_flags/set_load_flags() uses int, and that I don't want to take
on :-{.

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
content/browser/loader/offline_policy.h:44: typedef enum { ROUTE_INIT,
ROUTE_ONLINE, ROUTE_OFFLINE } RouteOfflineState;
On 2013/04/03 21:48:19, rvargas wrote:
> nit: enum RouteOfflineState {}

Done.

https://codereview.chromium.org/12886022/diff/44002/net/http/http_response_in...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/12886022/diff/44002/net/http/http_response_in...
net/http/http_response_info.h:69: bool network_verified;
On 2013/04/03 21:48:19, rvargas wrote:
> We have two options:
> 1. Have a signal about network connectivity. (network_was_accessed,
> was_network_fetched, network_used etc)
> 2. Have a signal about caching details.
> (cache_verified or something like that)
> 
> I'm inclined to go with the first option, and in that case, the flag should be
> set by the network transaction, not the cache... and that should be a more
> reliable indicator (as in not everything goes through the cache).

Done, but just confirming: Do we actually use the response_info_ from the
network when we're just verifying the cache entry?  Specifically, I don't see
response_ being overwritten with new_response_ in
HttpCache::Transacation::DoSuccessfulSendRequest() in the 304 case.

[rvargas (doing something else), 2013-04-04 20:13:12]

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/44002/content/browser/loader/of...
content/browser/loader/offline_policy.h:33: // Return any additional load flags
to be |'d for the current request.
On 2013/04/04 19:42:49, rdsmith wrote:
> On 2013/04/03 21:48:19, rvargas wrote:
> > nit: ORed ? OR'd ? added?. 
> 
> Done.
> 
> > By the way, flags should be unsigned ints (but I see
> > that the caller is currently using ints :(  )
> 
> I was going to just fix this in RDHI, but
> URLRequest::load_flags/set_load_flags() uses int, and that I don't want to
take
> on :-{.

Yeah, that's what I meant... we are using int everywhere. I think it should be
fixed, but definitely not in this CL.

https://codereview.chromium.org/12886022/diff/44002/net/http/http_response_in...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/12886022/diff/44002/net/http/http_response_in...
net/http/http_response_info.h:69: bool network_verified;
On 2013/04/04 19:42:49, rdsmith wrote:
> On 2013/04/03 21:48:19, rvargas wrote:
> > We have two options:
> > 1. Have a signal about network connectivity. (network_was_accessed,
> > was_network_fetched, network_used etc)
> > 2. Have a signal about caching details.
> > (cache_verified or something like that)
> > 
> > I'm inclined to go with the first option, and in that case, the flag should
be
> > set by the network transaction, not the cache... and that should be a more
> > reliable indicator (as in not everything goes through the cache).
> 
> Done, but just confirming: Do we actually use the response_info_ from the
> network when we're just verifying the cache entry?  Specifically, I don't see
> response_ being overwritten with new_response_ in
> HttpCache::Transacation::DoSuccessfulSendRequest() in the 304 case.

The field has to be updated on HttpCache::Transaction::DoUpdateCachedResponse()

[droger_google, 2013-04-05 10:55:24]

We're looking into the offline mode on iOS, and as Randy suggested, it may be
useful to use the OfflinePolicy on iOS to share some code and logic.

However, in the current state, there are high level issues that would prevent us
from using it. I have highlighted these issues in my comments below. I'd like to
see if these issue can be solved or if we should have a different code on iOS.

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
File content/browser/loader/offline_policy.cc (right):

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.cc:8: #include
"content/common/resource_messages.h"
Why do you need this?

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.h:12: #include
"webkit/glue/resource_type.h"
This file should probably moved out of webkit/glue.
We don't want to introduce any new use of webkit/glue.

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.h:28: class CONTENT_EXPORT OfflinePolicy {
OfflinePolicy is in content/. This may not be a blocker for now, but if there is
another place to put this file it could be better for us.
We try not to use content/ on iOS.

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.h:34: int GetAdditionalLoadFlags(int
child_id, int route_id, int current_flags,
I have two issues with this API:

1) On iOS, we don't have the concept of route_id and child_id.
It seems that these are only used to generate a key (as a pair of int), so that
should be abstracted as a generic key.

2) This class seems to manage all Chrome connections (as it keeps a map of all
the route_id x child_id). This is not something that fits very well in our
architecture, as it would be more natural for us to have one OfflinePolicy per
tab. If we have only one instance of this for all the tabs, I don't really know
what can own it.

[Randy Smith (Not in Mondays), 2013-04-05 17:56:38]

(No source changes; just discussion with David.)

Darin: Question for you on proper use of webkit types in the context of RDHI and
iOS below.  You might also have an opinion on some of the other issues.

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
File content/browser/loader/offline_policy.cc (right):

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.cc:8: #include
"content/common/resource_messages.h"
On 2013/04/05 10:55:24, droger_google wrote:
> Why do you need this?

I think it was a a leftover from when I had a different set of arguments.  I'll
remove it.

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.h:12: #include
"webkit/glue/resource_type.h"
On 2013/04/05 10:55:24, droger_google wrote:
> This file should probably moved out of webkit/glue.
> We don't want to introduce any new use of webkit/glue.

Darin, what's your perspective on this?  I feel like I have no good sense of the
appropriate webkit/RDH relationship.

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.h:28: class CONTENT_EXPORT OfflinePolicy {
On 2013/04/05 10:55:24, droger_google wrote:
> OfflinePolicy is in content/. This may not be a blocker for now, but if there
is
> another place to put this file it could be better for us.
> We try not to use content/ on iOS.

Wow, that's a stranger and more alien world than I thought :-}.  Can you give me
a quick sketch of what you do use on iOS and what the architectural relationship
is between Chrome for iOS and Chromium standard?  

There's not really any place *besides* content I can think of to put this, since
it needs to be accessible from the ResourceDispatcherHostImpl (which
specifically rules out chrome/) and it doesn't really have to do with the
network.   Do you have any thoughts about where it should go?

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.h:34: int GetAdditionalLoadFlags(int
child_id, int route_id, int current_flags,
On 2013/04/05 10:55:24, droger_google wrote:
> I have two issues with this API:
> 
> 1) On iOS, we don't have the concept of route_id and child_id.
> It seems that these are only used to generate a key (as a pair of int), so
that
> should be abstracted as a generic key.
> 
> 2) This class seems to manage all Chrome connections (as it keeps a map of all
> the route_id x child_id). This is not something that fits very well in our
> architecture, as it would be more natural for us to have one OfflinePolicy per
> tab. If we have only one instance of this for all the tabs, I don't really
know
> what can own it.

My thought about using a single class was that it would make it easy for us to
play with policies that spanned tabs, e.g. the entire browser going online or
offline at once.  But we could also do that with static variables on a shared
class, so I don't think it's a very high priority preference.  And given that I
suspect the alternative is to suggest you use a global variable, a per-tab class
probably makes sense :-}.  And onn a implementation level, it's a pretty simple
refactor to hoist the child_id,route_id mapping up into
ResourceDispatcherHostImpl and making the class per-tab.  I'll do that and put
up a new patchset.

[Randy Smith (Not in Mondays), 2013-04-05 19:40:31]

Made offline policy per-tab rather than global.  David, let me know what you
think.

[Randy Smith (Not in Mondays), 2013-04-08 16:43:47]

Ok, I think I'm ready for review.

Darin: chrome/*, content/*.
Ricardo: net/*

Let me know what you think.

David: I'm very interested in your comments, and will incorporate them as you
give them, but if I get LGTMs from Darin and Ricardo I'm going to land this and
immediate start a new CL to incorporate your comments, as getting this patch in
is critical path for the policy/planning side.

[droger_google, 2013-04-08 16:45:46]

On 2013/04/05 19:40:31, rdsmith wrote:
> Made offline policy per-tab rather than global.  David, let me know what you
> think.

This new design would fit better on iOS. There would still be some ifdefs, as we
can't have the same logic for the INIT/ONLINE/OFFLINE states. If we go with my
initial implementation, the ONLINE state would not exist on iOS.

An other idea (not necessarily better) would be to keep the old design, extract
the few lines of cache logic into some utility function and then share only that
function and not the whole class.

[droger_google, 2013-04-08 16:49:29]

I am still not happy with webkit/glue/resource_type.h, and I don't know what can
be done for content/.

The content/ issue is not blocking though.

[blundell, 2013-04-08 16:52:04]

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/52001/content/browser/loader/of...
content/browser/loader/offline_policy.h:28: class CONTENT_EXPORT OfflinePolicy {
We're currently determining (with darin and jam) what the exact policy is going
to be re: Chrome for iOS's usage of content/. I'll bring up this example in
those discussions, and am also going to write up a design document explaining
Chrome for iOS's usage of content/ once the dust settles. In the meantime please
feel free to keep this in content/, and I or David will loop back around once we
have resolution on this issue.

On 2013/04/05 17:56:38, rdsmith wrote:
> On 2013/04/05 10:55:24, droger_google wrote:
> > OfflinePolicy is in content/. This may not be a blocker for now, but if
there
> is
> > another place to put this file it could be better for us.
> > We try not to use content/ on iOS.
> 
> Wow, that's a stranger and more alien world than I thought :-}.  Can you give
me
> a quick sketch of what you do use on iOS and what the architectural
relationship
> is between Chrome for iOS and Chromium standard?  
> 
> There's not really any place *besides* content I can think of to put this,
since
> it needs to be accessible from the ResourceDispatcherHostImpl (which
> specifically rules out chrome/) and it doesn't really have to do with the
> network.   Do you have any thoughts about where it should go?

[blundell, 2013-04-08 16:56:09]

On 2013/04/08 16:49:29, droger_google wrote:
> I am still not happy with webkit/glue/resource_type.h, and I don't know what
can
> be done for content/.
> 
> The content/ issue is not blocking though.

I'll discuss the resource_type issue with darin and jam as well. We can
definitely come back to these issues after the fact.

[rvargas (doing something else), 2013-04-08 19:11:51]

net LGTM

https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unitt...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:506:
RunTransactionTestWithResponseInfo(cache.http_cache(), kSimpleGET_Transaction,
SimpleGET_NetworkAccessed_Cache is a superset of what you're adding to this
test, so it's probably better to leave this test alone, testing just the bare
minimum functionality of a simple get.

https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:969: AddMockTransaction(&transaction);
The predefined transactions are already registered so no need for this.

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
File net/http/http_network_layer_unittest.cc (right):

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:293: TEST_F(HttpNetworkLayerTest,
ConfirmNetworkVerified) {
nit: remove "Confirm" from the name of both tests?

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:301: "Host: www.google.com\r\n"
nit: we usually don't extra indent when splitting strings

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:306: data_writes,
arraysize(data_writes));
nit: indent four less spaces

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:324: rv = callback.WaitForResult();
nit: callback.GetResult(rv)

https://codereview.chromium.org/12886022/diff/73001/net/http/http_response_in...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/12886022/diff/73001/net/http/http_response_in...
net/http/http_response_info.h:68: // direct load or by confirming the current
cached copy is good.
This comment is stale. This is not about verification, or the cache.

[Randy Smith (Not in Mondays), 2013-04-08 21:03:10]

Incorporated Ricardo's comments.  Ricardo, you might or might not want to glance
at the changes--they involved creating a new test, and fixing some existing
code.

https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unitt...
File net/http/http_cache_unittest.cc (right):

https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:506:
RunTransactionTestWithResponseInfo(cache.http_cache(), kSimpleGET_Transaction,
On 2013/04/08 19:11:52, rvargas wrote:
> SimpleGET_NetworkAccessed_Cache is a superset of what you're adding to this
> test, so it's probably better to leave this test alone, testing just the bare
> minimum functionality of a simple get.

SimpleGET_NetworkAccessed_Cache doesn't test to make sure a read with an empty
cache (i.e. a read from network) does indeed set network_accessed.  But I'll
create a new test to do that and clean this one up.  Done.

https://codereview.chromium.org/12886022/diff/73001/net/http/http_cache_unitt...
net/http/http_cache_unittest.cc:969: AddMockTransaction(&transaction);
On 2013/04/08 19:11:52, rvargas wrote:
> The predefined transactions are already registered so no need for this.

Done.

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
File net/http/http_network_layer_unittest.cc (right):

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:293: TEST_F(HttpNetworkLayerTest,
ConfirmNetworkVerified) {
On 2013/04/08 19:11:52, rvargas wrote:
> nit: remove "Confirm" from the name of both tests?

Done.

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:301: "Host: www.google.com\r\n"
On 2013/04/08 19:11:52, rvargas wrote:
> nit: we usually don't extra indent when splitting strings

Ok.  I'll fix the GET test as well (which is where this was copied from).

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:306: data_writes,
arraysize(data_writes));
On 2013/04/08 19:11:52, rvargas wrote:
> nit: indent four less spaces

Done (in GET test as well).

https://codereview.chromium.org/12886022/diff/73001/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:324: rv = callback.WaitForResult();
On 2013/04/08 19:11:52, rvargas wrote:
> nit: callback.GetResult(rv)

Done (GET as well).

https://codereview.chromium.org/12886022/diff/73001/net/http/http_response_in...
File net/http/http_response_info.h (right):

https://codereview.chromium.org/12886022/diff/73001/net/http/http_response_in...
net/http/http_response_info.h:68: // direct load or by confirming the current
cached copy is good.
On 2013/04/08 19:11:52, rvargas wrote:
> This comment is stale. This is not about verification, or the cache.

Done.

[rvargas (doing something else), 2013-04-08 21:54:46]

still looks good (PS15)

https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_lay...
File net/http/http_network_layer_unittest.cc (right):

https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:323: ASSERT_EQ(OK, rv);
nit: merge this line with the previous one.

[Randy Smith (Not in Mondays), 2013-04-08 22:00:13]

Thanks!

https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_lay...
File net/http/http_network_layer_unittest.cc (right):

https://codereview.chromium.org/12886022/diff/72003/net/http/http_network_lay...
net/http/http_network_layer_unittest.cc:323: ASSERT_EQ(OK, rv);
On 2013/04/08 21:54:46, rvargas wrote:
> nit: merge this line with the previous one.

Done.

[darin (slow to review), 2013-04-11 20:01:22]

I just focused on content/ code.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
File content/browser/loader/offline_policy.cc (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.cc:24: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
it seems like this switch could just be checked once in the constructor.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:36: // Return any additional load flags
to be ORed for the current request.
nit: "for the current request" ... since the parameters do not include an
URLRequest object, might want to revise this text.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:41: void RequestCompleted(const
net::HttpResponseInfo& response_info);
nit: how about method name that better describes the action this method will
take?  how about calling it UpdateStateForCompletedRequest?

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:44: enum OfflineState { INIT, ONLINE,
OFFLINE };
nit: since this class is named OfflinePolicy, you could just name this enum
"State".  No need to repeat the "namespace".

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:46: OfflineState offline_state_;
ditto: could just be named "state_"

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:235: const
ResourceHostMsg_Request& request_data, int child_id,
nit: I think the style guide says to put one argument per line when the
arguments start on the line following the function name.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:706: // There should be
an entry in the map created when we dispatched the
DCHECK that this is true?  operator[] will create an entry if one doesn't
already exist.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:708:
offline_map_[ProcessRouteIDs(info->GetChildID(), info->GetRouteID())]->
It could also be reasonable for ResourceRequestInfoImpl to have a
GetGlobalRoutingID function.  Hmm, that might be useful elsewhere.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.h:241: typedef
std::pair<int, int> ProcessRouteIDs;
nit: maybe call this GlobalRoutingID?  maybe it should be an actual struct since
"first" and "second" are really terrible member variable names.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.h:459: OfflineMap
offline_map_;
nit: I think it'd read better in the .cc file if this variable were named
offline_policy_map_.

[darin (slow to review), 2013-04-11 20:10:38]

It is OK to use webkit/glue/resource_type.h.  It is true that it should probably
move elsewhere, but that's a non-trivial change at this point.  I wouldn't try
to avoid it in this CL.

[Randy Smith (Not in Mondays), 2013-04-12 14:06:44]

Darin: PTAL.

Note on change to use of ResourceType::Type: The original comment came from
discussion between droger@ and I around making this as usable as possible for
Chrome for IOS, which tries to avoid dependencies on WebKit because (I presume)
they have an arms length relationship with the actual WebKit rendering engine. 
Based on this, I decided to do a bit of a refactor to get the webkit glue
inclusion out of that file--it may be that I'll want more data for future
variations in OfflinePolicy, but I figure I can cross that bridge when I come to
it, and that this'll make the file more usable for iOS.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
File content/browser/loader/offline_policy.cc (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.cc:24: if
(!CommandLine::ForCurrentProcess()->HasSwitch(
On 2013/04/11 20:01:23, darin wrote:
> it seems like this switch could just be checked once in the constructor.

Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
File content/browser/loader/offline_policy.h (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:36: // Return any additional load flags
to be ORed for the current request.
On 2013/04/11 20:01:23, darin wrote:
> nit: "for the current request" ... since the parameters do not include an
> URLRequest object, might want to revise this text.

Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:41: void RequestCompleted(const
net::HttpResponseInfo& response_info);
On 2013/04/11 20:01:23, darin wrote:
> nit: how about method name that better describes the action this method will
> take?  how about calling it UpdateStateForCompletedRequest?

Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:44: enum OfflineState { INIT, ONLINE,
OFFLINE };
On 2013/04/11 20:01:23, darin wrote:
> nit: since this class is named OfflinePolicy, you could just name this enum
> "State".  No need to repeat the "namespace".

Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/of...
content/browser/loader/offline_policy.h:46: OfflineState offline_state_;
On 2013/04/11 20:01:23, darin wrote:
> ditto: could just be named "state_"

Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:235: const
ResourceHostMsg_Request& request_data, int child_id,
On 2013/04/11 20:01:23, darin wrote:
> nit: I think the style guide says to put one argument per line when the
> arguments start on the line following the function name.

Wow, right you are.  I wonder if that changed recently--I remembered decls as
being strict and defns as being more freeform (as in the original for this
function).  Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:706: // There should be
an entry in the map created when we dispatched the
On 2013/04/11 20:01:23, darin wrote:
> DCHECK that this is true?  operator[] will create an entry if one doesn't
> already exist.

So my basic response to this is "Sure" and I'll put it in.  However, I'll also
call out the reason I didn't originally put it in, which is that I've seen
guidance on chromium-dev (pkasting@ comes to mind) that if you're going to
dereference a variable, you shouldn't add in the extra DCHECK, and if the
reference creates an entry when it's not there, I believe it'll be NULL, and
that'll result in a crash on the line below.

Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.cc:708:
offline_map_[ProcessRouteIDs(info->GetChildID(), info->GetRouteID())]->
On 2013/04/11 20:01:23, darin wrote:
> It could also be reasonable for ResourceRequestInfoImpl to have a
> GetGlobalRoutingID function.  Hmm, that might be useful elsewhere.

Done, though it was a bit of a yak shave.  If you have anything more than nits
about this implementation, I'd like to pull it out into a separate CL, just so
that discussion around it doesn't delay this CL.  

One note:  The replacement of "struct GlobalRequestID;" with the inclusion of
the file it was in was the belief that if GlobalRequestID was being used as a
key in maps in rdhi.h the actual structure needed to be known by rdhi.h,
semi-confirmed by successfully compiling with the "struct GlobalRequestID;"
removed, followed by putting the include in for IWYU.  If you object to any of
that I can reverse it, but it made sense at the time :-}.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.h:241: typedef
std::pair<int, int> ProcessRouteIDs;
On 2013/04/11 20:01:23, darin wrote:
> nit: maybe call this GlobalRoutingID?  maybe it should be an actual struct
since
> "first" and "second" are really terrible member variable names.

Happy to take your guidance on this; I was just copying (moving) what was
already present in this file.  Done.

https://codereview.chromium.org/12886022/diff/89001/content/browser/loader/re...
content/browser/loader/resource_dispatcher_host_impl.h:459: OfflineMap
offline_map_;
On 2013/04/11 20:01:23, darin wrote:
> nit: I think it'd read better in the .cc file if this variable were named
> offline_policy_map_.

Done.

[darin (slow to review), 2013-04-18 05:03:02]

I think you made a yak happy.  LGTM for content/ bits.

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/g...
File content/browser/loader/global_routing_id.h (right):

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/g...
content/browser/loader/global_routing_id.h:36: return child_id != other.child_id
||
nit: usually best implemented as the generic !(*this == other)

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/o...
File content/browser/loader/offline_policy.cc (right):

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/o...
content/browser/loader/offline_policy.cc:17:
switches::kEnableOfflineCacheAccess)),
nit: 4 space indent for line continuations

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/o...
content/browser/loader/offline_policy.cc:22: int
OfflinePolicy::GetAdditionalLoadFlags(
nit: style guide suggests this formatting:

int OfflinePolicy::GetAdditionalLoadFlags(int current_flags,
                                          bool reset_state) {

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1390: delete
offline_policy_map_[GlobalRoutingID(child_id, route_id)];
nit: consider doing this with an iterator so you only have to do one map lookup?

[Randy Smith (Not in Mondays), 2013-04-18 20:04:56]

Thanks!  I'll merge this to LKGR and throw it at the CQ.

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/g...
File content/browser/loader/global_routing_id.h (right):

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/g...
content/browser/loader/global_routing_id.h:36: return child_id != other.child_id
||
On 2013/04/18 05:03:02, darin wrote:
> nit: usually best implemented as the generic !(*this == other)

Done, but note that this was based on copying
content/public/browser/global_request_id.h; not fixing that because you gotta
leave a *little* bit of hair on the yak (and I don't want to gratuitously change
the content API files).

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/o...
File content/browser/loader/offline_policy.cc (right):

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/o...
content/browser/loader/offline_policy.cc:17:
switches::kEnableOfflineCacheAccess)),
On 2013/04/18 05:03:02, darin wrote:
> nit: 4 space indent for line continuations

Um ... done, but I'll note that I'm already indented four spaces from the ":",
and I thought that's where indentation was counted from (first non-blank
character on previous line).  But I'll follow what I think you're asking for
(which is four spaces from the "e") since if I had multiple initializations of
member data variables, that's what would make them consistent.

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/o...
content/browser/loader/offline_policy.cc:22: int
OfflinePolicy::GetAdditionalLoadFlags(
On 2013/04/18 05:03:02, darin wrote:
> nit: style guide suggests this formatting:
> 
> int OfflinePolicy::GetAdditionalLoadFlags(int current_flags,
>                                           bool reset_state) {

Done.

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_impl.cc:1390: delete
offline_policy_map_[GlobalRoutingID(child_id, route_id)];
On 2013/04/18 05:03:02, darin wrote:
> nit: consider doing this with an iterator so you only have to do one map
lookup?

Done.

[darin (slow to review), 2013-04-18 22:57:29]

On Thu, Apr 18, 2013 at 1:04 PM, <rdsmith@chromium.org> wrote:

> Thanks!  I'll merge this to LKGR and throw it at the CQ.
>
>
>
>
> https://codereview.chromium.**org/12886022/diff/113001/**
>
content/browser/loader/global_**routing_id.h<https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/global_routing_id.h>
> File content/browser/loader/global_**routing_id.h (right):
>
> https://codereview.chromium.**org/12886022/diff/113001/**
>
content/browser/loader/global_**routing_id.h#newcode36<https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/global_routing_id.h#newcode36>
> content/browser/loader/global_**routing_id.h:36: return child_id !=
> other.child_id ||
> On 2013/04/18 05:03:02, darin wrote:
>
>> nit: usually best implemented as the generic !(*this == other)
>>
>
> Done, but note that this was based on copying
> content/public/browser/global_**request_id.h; not fixing that because you
> gotta leave a *little* bit of hair on the yak (and I don't want to
> gratuitously change the content API files).
>
>
Ah...


>
> https://codereview.chromium.**org/12886022/diff/113001/**
>
content/browser/loader/**offline_policy.cc<https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/offline_policy.cc>
> File content/browser/loader/**offline_policy.cc (right):
>
> https://codereview.chromium.**org/12886022/diff/113001/**
>
content/browser/loader/**offline_policy.cc#newcode17<https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/offline_policy.cc#newcode17>
> content/browser/loader/**offline_policy.cc:17:
> switches::**kEnableOfflineCacheAccess)),
> On 2013/04/18 05:03:02, darin wrote:
>
>> nit: 4 space indent for line continuations
>>
>
> Um ... done, but I'll note that I'm already indented four spaces from
> the ":", and I thought that's where indentation was counted from (first
> non-blank character on previous line).  But I'll follow what I think
> you're asking for (which is four spaces from the "e") since if I had
> multiple initializations of member data variables, that's what would
> make them consistent.
>
>
Yeah, that's what I meant.



>
> https://codereview.chromium.**org/12886022/diff/113001/**
>
content/browser/loader/**offline_policy.cc#newcode22<https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/offline_policy.cc#newcode22>
> content/browser/loader/**offline_policy.cc:22: int
> OfflinePolicy::**GetAdditionalLoadFlags(
> On 2013/04/18 05:03:02, darin wrote:
>
>> nit: style guide suggests this formatting:
>>
>
>  int OfflinePolicy::**GetAdditionalLoadFlags(int current_flags,
>>                                            bool reset_state) {
>>
>
> Done.
>
>
> https://codereview.chromium.**org/12886022/diff/113001/**
>
content/browser/loader/**resource_dispatcher_host_impl.**cc<https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/resource_dispatcher_host_impl.cc>
> File content/browser/loader/**resource_dispatcher_host_impl.**cc (right):
>
> https://codereview.chromium.**org/12886022/diff/113001/**
>
content/browser/loader/**resource_dispatcher_host_impl.**cc#newcode1390<https://codereview.chromium.org/12886022/diff/113001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode1390>
> content/browser/loader/**resource_dispatcher_host_impl.**cc:1390: delete
> offline_policy_map_[**GlobalRoutingID(child_id, route_id)];
> On 2013/04/18 05:03:02, darin wrote:
>
>> nit: consider doing this with an iterator so you only have to do one
>>
> map lookup?
>
> Done.
>
>
https://codereview.chromium.**org/12886022/<https://codereview.chromium.org/1...
>

[Randy Smith (Not in Mondays), 2013-04-19 21:27:32]

Ricardo: Could you review the PS20->PS21 change to
resource_dispatcher_host_impl.cc?  (Darin: I'm happy to take a review from you
instead, just thinking you're busy enough that I shouldn't bother you with
something moderately close to trivial.)

Mostly I'm giving someone an opportunity to tell me I should move the duplicated
3 lines of code into a separate private methods if they want to.

[rvargas (doing something else), 2013-04-19 21:38:25]

lgtm

[commit-bot: I haz the power, 2013-04-19 21:41:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rdsmith@chromium.org/12886022/130001

[commit-bot: I haz the power, 2013-04-20 02:53:10]

Change committed as 195374