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

net/url_request/url_request_job.h

 // Copyright (c) 2006-2008 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.
 
 #ifndef NET_URL_REQUEST_URL_REQUEST_JOB_H_
 #define NET_URL_REQUEST_URL_REQUEST_JOB_H_
 
 #include <string>
 #include <vector>
 
@@ skipping 101 matching lines @@
   // For example, encoding_types[0] = FILTER_TYPE_SDCH and encoding_types[1] =
   // FILTER_TYPE_GZIP, means the content was first encoded by sdch, and then
   // result was encoded by gzip.  To decode, a series of filters must be applied
   // in the reverse order (in the above example, ungzip first, and then sdch
   // expand).
   virtual bool GetContentEncodings(
       std::vector<Filter::FilterType>* encoding_types) {
     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:

 
   // Called to setup stream filter for this request. An example of filter is
   // content encoding/decoding.
   void SetupFilter();
 
   // Called to determine if this response is a redirect.  Only makes sense
   // for some types of requests.  This method returns true if the response
   // is a redirect, and fills in the location param with the URL of the
   // redirect.  The HTTP status code (e.g., 302) is filled into
   // |*http_status_code| to signify the type of redirect.
@@ skipping 151 matching lines @@
  private:
   // Called to query whether there is data available in the filter to be read
   // out.
   bool FilterHasData();
 
   // Indicates that the job is done producing data, either it has completed
   // all the data or an error has been encountered. Set exclusively by
   // 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_;
 
   // If the filter filled its output buffer, then there is a change that it
   // still has internal data to emit, and this flag is set.
   bool filter_needs_more_output_space_;
 
   // When we filter data, we receive data into the filter buffers.  After
   // processing the filtered data, we return the data in the caller's buffer.
   // While the async IO is in progress, we save the user buffer here, and
@@ skipping 10 matching lines @@
 
   // Total number of bytes read from network (or cache) and and typically handed
   // to filter to process.  Used to histogram compression ratios, and error
   // recovery scenarios in filters.
   int64 filter_input_byte_count_;
 
   DISALLOW_COPY_AND_ASSIGN(URLRequestJob);
 };
 
 #endif  // NET_URL_REQUEST_URL_REQUEST_JOB_H_