Give the filter setup more context so it can figure out whether it's download...

net/url_request/url_request_job.h

Index: net/url_request/url_request_job.h
===================================================================
--- net/url_request/url_request_job.h	(revision 13526)
+++ net/url_request/url_request_job.h	(working copy)
@@ -119,6 +119,9 @@
     return false;
   }
 
+  // Find out if this is a download.
+  virtual bool IsDownload() const;
+
   // Find out if this is a response to a request that advertised an SDCH
   // dictionary.  Only makes sense for some types of requests.
   virtual bool IsSdchResponse() const { return false; }

[jar (doing other things), 2009/04/10 20:27:14]

One element that I think is questionable is the difference in implementation of
IsSdchResponse() and IsDownload().  Both methods are implemented for use only by
the filter.cc as a context for a derived class.  I suspect that IsDownload()
also "only makes sense for some types of requests."  

As I result, I'd suggest that IsDownload() should also have a trivial (return
false) definition here, and then should have a precise (variable) definition in
the class where it is applicable (probably along side of IsSdchResponse().

I had pulled WTC in to try to get a judgement call on where this should be done
(perhaps it should be the case that IsSdchResponse() should be fully defined
here... but I didn't think so).

WTC didn't weigh in strongly on this, but he agreed that these two methods
should be implemented using the same patter in the same places.

Which place do you think is better, and why?  If you don't have a justification
for being different from IsSdchResponse(), I'd suggest following its pattern.

[Lei Zhang, 2009/04/10 21:25:21]

SDCH only makes sense for http right? To me, download is more general and
happens for many different types of requests.

[jar (doing other things), 2009/04/10 22:10:09]

SDCH is currently outlawed by the spec for simplicity/security reasons in other
schemes (like ftp and https).  Conceptually, it could appear anywhere (i.e. The
general implementation could have been pushed to the base class... and it would
have returned false based on the flags exactly as your accessor gets a bool).

Note that in both cases, the only current consumer of these accessors are in
filter.cc, and hence it is only "really used"  (read: needs to be implemented)
in the derived class.

It is also interesting that the IsSdchResponse() could plausibly be renamed to
IsSdchRequest() (it really flags the fact that the request was a full sdch
request with an applicable dictionary), and IsDownload() might plausibly be
renamed IsDownloadRequest(), making them more clearly similar.

As a result, I go back and forth about where it is best to define these... but 
I suspect they should be defined next to each other, as they both derive from
LOAD_ flags. 


On 2009/04/10 21:25:21, Lei Zhang wrote:

@@ -293,6 +296,9 @@
   // NotifyDone so that it is kept in sync with the request.
   bool done_;
 
+  // Cache the load flags from request_ because it might go away.
+  int load_flags_;
+
   // The data stream filter which is enabled on demand.
   scoped_ptr<Filter> filter_;