Add a response_info() accessor to URLRequest to get a reference to the net::H...

Add a response_info() accessor to URLRequest to get a reference to the net::HttpResponseInfo struct in it entirety.

BUG=none
TEST=URLRequestTestHTTP.ResponseHeadersTest

Committed revision 28067

[darin (slow to review), 2009-10-03 14:05:18]

This structure was intentionally hidden.  What fields do you need?

[michaeln, 2009-10-03 16:48:34]

On 2009/10/03 14:05:18, darin wrote:
> This structure was intentionally hidden.  What fields do you need?

My plan was to pickle this struct and save it in the appcache.

I see there are accessors for pretty much all of the constituent fields. Was
just adding this as a convenience to get at all of them at once. I can
reconstruct the struct piecemeal (or define a new struct) given the existing
accessors if need be.

I can understand why its hidden, URLRequest presents a hi level api for what the
net libary does, and this struct is an impl detail that doesn't need to be
'leaked' out in that hi level interface.

The appcache depends on the 'net' library for more than this hi level fetching
api.
- interceptor infrastructure, involving custom url request jobs
- disk cache
Seemed reasonable to expose this net data structure for my use case. Didn't see
the need to define yet another way to represent the response info. The net
struct works for me.

[darin (slow to review), 2009-10-05 06:58:18]

On Sat, Oct 3, 2009 at 9:48 AM, <michaeln@chromium.org> wrote:

> On 2009/10/03 14:05:18, darin wrote:
>
>> This structure was intentionally hidden.  What fields do you need?
>>
>
> My plan was to pickle this struct and save it in the appcache.
>

Oh?  Like HttpCache::Read/WriteResponseInfo?

It seems like those methods should be shared.  There is so much overlap with
the ordinary HTTP cache.  Maybe you should just be instantiating a
HttpCache?
(It would probably need some refactoring.)

-Darin



>
> I see there are accessors for pretty much all of the constituent fields.
> Was
> just adding this as a convenience to get at all of them at once. I can
> reconstruct the struct piecemeal (or define a new struct) given the
> existing
> accessors if need be.
>
> I can understand why its hidden, URLRequest presents a hi level api for
> what the
> net libary does, and this struct is an impl detail that doesn't need to be
> 'leaked' out in that hi level interface.
>
> The appcache depends on the 'net' library for more than this hi level
> fetching
> api.
> - interceptor infrastructure, involving custom url request jobs
> - disk cache
> Seemed reasonable to expose this net data structure for my use case. Didn't
> see
> the need to define yet another way to represent the response info. The net
> struct works for me.
>
>
>
>
>
>
>
> http://codereview.chromium.org/251082
>

[michaeln, 2009-10-05 21:23:04]

> Oh?  Like HttpCache::Read/WriteResponseInfo?

Exactly like those static methods.

  // Helper function for reading response info from the disk cache.  If the
  // cache doesn't have the whole resource *|request_truncated| is set to true.
  static bool ReadResponseInfo(disk_cache::Entry* disk_entry,
                               HttpResponseInfo* response_info,
                               bool* response_truncated);

  // Helper function for writing response info into the disk cache.  If the
  // cache doesn't have the whole resource |request_truncated| should be true.
  static bool WriteResponseInfo(disk_cache::Entry* disk_entry,
                                const HttpResponseInfo* response_info,
                                bool skip_transient_headers,
                                bool response_truncated);

  // Given a header data blob, convert it to a response info object.
  static bool ParseResponseInfo(const char* data, int len,
                                HttpResponseInfo* response_info,
                                bool* response_truncated);
 
> It seems like those methods should be shared.  There is so much overlap with
> the ordinary HTTP cache.  Maybe you should just be instantiating a
> HttpCache?

I've been talking with ricardo about whether to instance disk_cache::Backend or
net::HttpCache. We reached the conclusion to instance disk_cache::Backend
directly. The header pickling/reading/writing methods are all static methods of
HttpCache, no refactoring required to get at those. I just need to feed it a
net::HttpResponseInfo.

[darin (slow to review), 2009-10-05 21:35:29]

On Mon, Oct 5, 2009 at 2:23 PM, <michaeln@chromium.org> wrote:

> Oh?  Like HttpCache::Read/WriteResponseInfo?
>>
>
> Exactly like those static methods.
>
>  // Helper function for reading response info from the disk cache.  If the
>  // cache doesn't have the whole resource *|request_truncated| is set to
> true.
>  static bool ReadResponseInfo(disk_cache::Entry* disk_entry,
>                               HttpResponseInfo* response_info,
>                               bool* response_truncated);
>
>  // Helper function for writing response info into the disk cache.  If the
>  // cache doesn't have the whole resource |request_truncated| should be
> true.
>  static bool WriteResponseInfo(disk_cache::Entry* disk_entry,
>                                const HttpResponseInfo* response_info,
>                                bool skip_transient_headers,
>                                bool response_truncated);
>
>  // Given a header data blob, convert it to a response info object.
>  static bool ParseResponseInfo(const char* data, int len,
>                                HttpResponseInfo* response_info,
>                                bool* response_truncated);
>
>  It seems like those methods should be shared.  There is so much overlap
>> with
>> the ordinary HTTP cache.  Maybe you should just be instantiating a
>> HttpCache?
>>
>
> I've been talking with ricardo about whether to instance
> disk_cache::Backend or
> net::HttpCache. We reached the conclusion to instance disk_cache::Backend
> directly. The header pickling/reading/writing methods are all static
> methods of
> HttpCache, no refactoring required to get at those. I just need to feed it
> a
> net::HttpResponseInfo.
>
>
>
I see.  It feels a bit unfortunate to me that we would have code outside of
net/ reaching into the
internals of HttpCache.  I agree that we should share the code, and it
sounds like your solution
is fairly lightweight.  I just wish we could have a cleaner separation
between implementation and
interface for the net module and its consumers.

-Darin



>
>
> http://codereview.chromium.org/251082
>

[rvargas (doing something else), 2009-10-05 21:37:54]

One of the main issues with having another instance of the http cache would be
how to get data into the cache. There is a lot of code there that make it so
that the cache will automatically decide to fetch data from the network as the
only way to validate and load resources. If that is not the case for AppCache,
that may mean a lot of changes.

[michaeln, 2009-10-05 21:42:10]

> I see.  It feels a bit unfortunate to me that we would have code outside of
> net/ reaching into the
> internals of HttpCache.  I agree that we should share the code, and it
> sounds like your solution
> is fairly lightweight.  I just wish we could have a cleaner separation
> between implementation and
> interface for the net module and its consumers.

I can appreciate that. I can tell you as a consumer net::HttpResponseInfo is a
pretty good class for consumption. In addition to the struct itself, I'm also
interested in the serialization/deserialization of that struct. Maybe those
functions should be methods of HttpResponseInfo instead of HttpCache.

Also, in the back of my head is that if HttpCache decides to change is
serialized format, its free to do so at anytime and drop the existing content of
the cache (on an chrome update for example). That would be uncool for the
appcache use case. We may want to 'version' the serialized format of that
structure. I haven't looked at the how things are inflated/deflated in detail
yet, maybe its already versioned?

[rvargas (doing something else), 2009-10-05 21:47:57]

We have 8 bits for the version of the response info. (currently set to 0x1).

On Mon, Oct 5, 2009 at 2:42 PM, <michaeln@chromium.org> wrote:

> I see.  It feels a bit unfortunate to me that we would have code outside of
>> net/ reaching into the
>> internals of HttpCache.  I agree that we should share the code, and it
>> sounds like your solution
>> is fairly lightweight.  I just wish we could have a cleaner separation
>> between implementation and
>> interface for the net module and its consumers.
>>
>
> I can appreciate that. I can tell you as a consumer net::HttpResponseInfo
> is a
> pretty good class for consumption. In addition to the struct itself, I'm
> also
> interested in the serialization/deserialization of that struct. Maybe those
> functions should be methods of HttpResponseInfo instead of HttpCache.
>
> Also, in the back of my head is that if HttpCache decides to change is
> serialized format, its free to do so at anytime and drop the existing
> content of
> the cache (on an chrome update for example). That would be uncool for the
> appcache use case. We may want to 'version' the serialized format of that
> structure. I haven't looked at the how things are inflated/deflated in
> detail
> yet, maybe its already versioned?
>
>
> http://codereview.chromium.org/251082
>

[michaeln, 2009-10-05 21:48:54]

Just looked inside HttpCache a little...

* format is versioned (great), and still at version 1
* the HttpCache::Read/Write are actually do synchronous IO, so i can't use those
directly

... really i think i just want to reuse the serialize/deserialize code for that
struct

So in addition to making this accessor for URLRequest.reponse_info(), i'll be
looking to move the pickling code out of HttpCache and into HttpResponseInfo.

[michaeln, 2009-10-05 21:50:57]

On 2009/10/05 21:37:54, rvargas wrote:
> One of the main issues with having another instance of the http cache would be
> how to get data into the cache. There is a lot of code there that make it so
> that the cache will automatically decide to fetch data from the network as the
> only way to validate and load resources. If that is not the case for AppCache,
> that may mean a lot of changes.

Right, that is not the case. Things only make their way into the AppCache by way
of an AppCacheUpdateJob, and never implicitly by way of a 'page' loading a
resource.

[darin (slow to review), 2009-10-05 22:26:40]

Factoring out the methods that do serialization/deserialization seems good
to me.  Avoiding a dependency on HttpCache for those methods is even better.
 Adding static methods to HttpResponseInfo for this would be ok.
-darin


On Mon, Oct 5, 2009 at 2:48 PM, <michaeln@chromium.org> wrote:

> Just looked inside HttpCache a little...
>
> * format is versioned (great), and still at version 1
> * the HttpCache::Read/Write are actually do synchronous IO, so i can't use
> those
> directly
>
> ... really i think i just want to reuse the serialize/deserialize code for
> that
> struct
>
> So in addition to making this accessor for URLRequest.reponse_info(), i'll
> be
> looking to move the pickling code out of HttpCache and into
> HttpResponseInfo.
>
>
>
> http://codereview.chromium.org/251082
>

[rvargas (doing something else), 2009-10-05 22:33:42]

LGTM.