Use FilterContext to allow filters to access URLRequestJob data...

Use FilterContext to allow filters to access URLRequestJob data

r=wtc,darin,huanr

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=11248

[jar (doing other things), 2009-03-06 04:00:56]

I have some more cleanup to do, but this CL was getting very large. This portion
is very mechanical, and was a pain to get right (I used the compile to identify
the MANY places where I needed to propogate the const-ness of the GetMimeType()
method.  ... so I'm quite sure I got them all).

In addition to the TODO comments, I'd also expect to be able to simplify the
filter factory.

The real goal is to be able to gather stats from within a filter, involving more
global info.  The FilterContext interface class will facilitate that.

You might also want to suggest better names for the FilterContext class, and for
the interface methods contained there.  My current strategy was to either make a
dead-on match with an existing method, or create a name that I felt confident
would not collide with existing methods.

Jim

[huanr, 2009-03-06 20:30:09]

I gave a try on the naming of filter context class and its methods. Reviewing
the rest in the mean time.

http://codereview.chromium.org/40138/diff/1283/134
File net/base/filter.h (right):

http://codereview.chromium.org/40138/diff/1283/134#newcode45
Line 45: class FilterContext {
Maybe FilterStreamContext? It is the contextual information of the data stream,
not the owner/caller. Is also sounds consistent with names used in filter,
stream_buffer_, etc.

http://codereview.chromium.org/40138/diff/1283/134#newcode58
Line 58: virtual base::Time GetContextRequestTime() const = 0;
GetStreamRequestTime?

http://codereview.chromium.org/40138/diff/1283/134#newcode61
Line 61: virtual bool IsCachedContentContext() const = 0;
Or just IsCachedContent?

http://codereview.chromium.org/40138/diff/1283/134#newcode70
Line 70: 
Is this the size of the input buffer of filter that caller writes to? If so,
GetInputStreambufferSize?

[wtc, 2009-03-07 01:50:14]

LGTM.

Just some minor nits below.

http://codereview.chromium.org/40138/diff/1283/156
File chrome/browser/automation/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/40138/diff/1283/156#newcode165
Line 165: const_cast<URLRequestSlowDownloadJob*>(this)->GetResponseInfo(&info);
Would be nice to avoid the const_cast.

http://codereview.chromium.org/40138/diff/1283/138
File net/base/filter.cc (right):

http://codereview.chromium.org/40138/diff/1283/138#newcode56
Line 56: filter_context.GetMimeType(&mime_type);
Nit: should we check the return values of GetMimeType and
GetContextURL?

http://codereview.chromium.org/40138/diff/1283/134
File net/base/filter.h (right):

http://codereview.chromium.org/40138/diff/1283/134#newcode47
Line 47: FilterContext() {}
I think we probably don't need to define the constructor
and destructor for an interface class.

It is important, however, the declare the destructor as
virtual.

http://codereview.chromium.org/40138/diff/1283/134#newcode54
Line 54: // Return false if gurl is not present.
Nit: what do you mean by "if gurl is not present"?

Nit: why do many of these methods have "Context" in their
names? It seems redundant because the class's name already
has "Context" in it.

http://codereview.chromium.org/40138/diff/1283/134#newcode138
Line 138: // If the function returns 0 then all data has been filtered ,and the
caller
Nit: the space should be after the comma.

http://codereview.chromium.org/40138/diff/1283/127
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/40138/diff/1283/127#newcode89
Line 89: bool URLRequestJob::GetContextURL(GURL* gurl) const {
Nit: in these three methods, use request_ instead of
request().  No need to use an accessor to access a class's
own members.

http://codereview.chromium.org/40138/diff/1283/127#newcode92
Line 92: (*gurl) = request()->url();
Nit: remove the parentheses around *gurl.

http://codereview.chromium.org/40138/diff/1283/130
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/40138/diff/1283/130#newcode94
Line 94: virtual bool GetMimeType(std::string* mime_type) const { return false;
}
Move the GetMimeType to the section of FilterContext methods
below, because GetMimeType is now part of the FilterContext
interface.  (It seems a little strange though.)

http://codereview.chromium.org/40138/diff/1283/130#newcode197
Line 197: // Provide some restricted info to filters.
Our convention is to use a comment like this for interface
methods that this class must implements:

  // FilterContext methods:

[jar (doing other things), 2009-03-07 03:55:38]

Changes made per huanr and wtc suggestions.  See comment below.

http://codereview.chromium.org/40138/diff/1283/156
File chrome/browser/automation/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/40138/diff/1283/156#newcode165
Line 165: const_cast<URLRequestSlowDownloadJob*>(this)->GetResponseInfo(&info);
On 2009/03/07 01:50:14, wtc wrote:
> Would be nice to avoid the const_cast.

Done.

http://codereview.chromium.org/40138/diff/1283/138
File net/base/filter.cc (right):

http://codereview.chromium.org/40138/diff/1283/138#newcode56
Line 56: filter_context.GetMimeType(&mime_type);
On 2009/03/07 01:50:14, wtc wrote:
> Nit: should we check the return values of GetMimeType and
> GetContextURL?

I put in DCHECKs on the return value of success.

http://codereview.chromium.org/40138/diff/1283/134
File net/base/filter.h (right):

http://codereview.chromium.org/40138/diff/1283/134#newcode45
Line 45: class FilterContext {
On 2009/03/06 20:30:09, huanr wrote:
> Maybe FilterStreamContext? It is the contextual information of the data
stream,
> not the owner/caller. Is also sounds consistent with names used in filter,
> stream_buffer_, etc.

hmmm... we don't call Filter classes FilterStream or StreamFilter, so I
*thought* that Filter might be the right word.... and then added context.

I still think there may be something better... but I'm not convinecd that adding
Stream will help.

http://codereview.chromium.org/40138/diff/1283/134#newcode47
Line 47: FilterContext() {}
On 2009/03/07 01:50:14, wtc wrote:
> I think we probably don't need to define the constructor
> and destructor for an interface class.
> 
> It is important, however, the declare the destructor as
> virtual.

I couldn't get the linker to be happy without giving the definitions.

http://codereview.chromium.org/40138/diff/1283/134#newcode54
Line 54: // Return false if gurl is not present.
On 2009/03/07 01:50:14, wtc wrote:
> Nit: what do you mean by "if gurl is not present"?
> 
> Nit: why do many of these methods have "Context" in their
> names? It seems redundant because the class's name already
> has "Context" in it.

All names changed to remove "Context" from them (now that I have a way to ensure
I wasn't colliding with anything).

I was fearful that in *some* context, there might not be a url, such as in a
testing context.  In the implementation in URLRequestJob(), there is a chance(?)
that there is no request_ member, and hence we can't provide a gurl.

http://codereview.chromium.org/40138/diff/1283/134#newcode58
Line 58: virtual base::Time GetContextRequestTime() const = 0;
On 2009/03/06 20:30:09, huanr wrote:
> GetStreamRequestTime?

I changed it to GetRequestTime().

note that the time significantly predates the creation of the stream that is fed
to the filter, so I'm not sure that Stream should be a part of the name.

http://codereview.chromium.org/40138/diff/1283/134#newcode61
Line 61: virtual bool IsCachedContentContext() const = 0;
On 2009/03/06 20:30:09, huanr wrote:
> Or just IsCachedContent?

Done.

http://codereview.chromium.org/40138/diff/1283/134#newcode70
Line 70: 
On 2009/03/06 20:30:09, huanr wrote:
> Is this the size of the input buffer of filter that caller writes to? If so,
> GetInputStreambufferSize? 

Done.

http://codereview.chromium.org/40138/diff/1283/134#newcode138
Line 138: // If the function returns 0 then all data has been filtered ,and the
caller
On 2009/03/07 01:50:14, wtc wrote:
> Nit: the space should be after the comma.

Done.

http://codereview.chromium.org/40138/diff/1283/127
File net/url_request/url_request_job.cc (right):

http://codereview.chromium.org/40138/diff/1283/127#newcode89
Line 89: bool URLRequestJob::GetContextURL(GURL* gurl) const {
On 2009/03/07 01:50:14, wtc wrote:
> Nit: in these three methods, use request_ instead of
> request().  No need to use an accessor to access a class's
> own members.

Done.

http://codereview.chromium.org/40138/diff/1283/127#newcode92
Line 92: (*gurl) = request()->url();
On 2009/03/07 01:50:14, wtc wrote:
> Nit: remove the parentheses around *gurl.

Done.

http://codereview.chromium.org/40138/diff/1283/130
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/40138/diff/1283/130#newcode94
Line 94: virtual bool GetMimeType(std::string* mime_type) const { return false;
}
On 2009/03/07 01:50:14, wtc wrote:
> Move the GetMimeType to the section of FilterContext methods
> below, because GetMimeType is now part of the FilterContext
> interface.  (It seems a little strange though.)

Done.

http://codereview.chromium.org/40138/diff/1283/130#newcode197
Line 197: // Provide some restricted info to filters.
On 2009/03/07 01:50:14, wtc wrote:
> Our convention is to use a comment like this for interface
> methods that this class must implements:
> 
>   // FilterContext methods:

Done.

[wtc, 2009-03-09 17:24:08]

LGTM on Patch Set 14.

http://codereview.chromium.org/40138/diff/1283/130
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/40138/diff/1283/130#newcode91
Line 91: // Called to fetch the mime_type for this request.  Only makes sense
for some
Please copy this comment to filter.h.

http://codereview.chromium.org/40138/diff/191/1296
File net/base/filter.h (right):

http://codereview.chromium.org/40138/diff/191/1296#newcode47
Line 47: FilterContext() = 0;
We should not need to declare and define (with = 0) this constructor.

http://codereview.chromium.org/40138/diff/191/1292
File net/url_request/url_request_job.h (right):

http://codereview.chromium.org/40138/diff/191/1292#newcode198
Line 198: virtual int GetInputStreambufferSize() const { return kFilterBufSize;
}
Should we capitalize "buffer"?

[huanr, 2009-03-09 17:50:43]

LGTM

http://codereview.chromium.org/40138/diff/191/1320
File chrome/browser/automation/url_request_mock_http_job.cc (right):

http://codereview.chromium.org/40138/diff/191/1320#newcode71
Line 71: void URLRequestMockHTTPJob::GetResponseInfoConst(net::HttpResponseInfo*
info) const {
this line exceeds 80 characters. same for some other GetResponseInfoConst
functions

http://codereview.chromium.org/40138/diff/191/1318
File chrome/browser/automation/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/40138/diff/191/1318#newcode141
Line 141: void
URLRequestSlowDownloadJob::GetResponseInfoConst(net::HttpResponseInfo* info)
const {
80 characters.

http://codereview.chromium.org/40138/diff/191/1300
File net/base/filter.cc (right):

http://codereview.chromium.org/40138/diff/191/1300#newcode54
Line 54: // TODO(jar): These settings should go into the derived classes, on an
as-needed basis.
Per our discussion, would be helpful to mention that the settings will be
consolidated with FilterContext.

http://codereview.chromium.org/40138/diff/191/1296
File net/base/filter.h (right):

http://codereview.chromium.org/40138/diff/191/1296#newcode44
Line 44: // supplied by the owner of this filter.
Can you also comment on filter chain? If the filter is within the filter chain,
is it the contextual information of the owner of the chain, or the upper filter
of this filter?

http://codereview.chromium.org/40138/diff/191/1296#newcode47
Line 47: FilterContext() = 0;
+1
On 2009/03/09 17:24:09, wtc wrote:
> We should not need to declare and define (with = 0) this constructor.

[jar (doing other things), 2009-03-09 19:00:41]

Responses to comments by huanr and wtc.

Note that this CL was submitted, so I'll formulate a smaller 
CL containing the commented changes listed below, and send it around for a code
review.

http://codereview.chromium.org/40138/diff/191/1320
File chrome/browser/automation/url_request_mock_http_job.cc (right):

http://codereview.chromium.org/40138/diff/191/1320#newcode71
Line 71: void URLRequestMockHTTPJob::GetResponseInfoConst(net::HttpResponseInfo*
info) const {
On 2009/03/09 17:50:44, huanr wrote:
> this line exceeds 80 characters. same for some other GetResponseInfoConst
> functions

Done.

http://codereview.chromium.org/40138/diff/191/1318
File chrome/browser/automation/url_request_slow_download_job.cc (right):

http://codereview.chromium.org/40138/diff/191/1318#newcode141
Line 141: void
URLRequestSlowDownloadJob::GetResponseInfoConst(net::HttpResponseInfo* info)
const {
On 2009/03/09 17:50:44, huanr wrote:
> 80 characters.

Done.

http://codereview.chromium.org/40138/diff/191/1300
File net/base/filter.cc (right):

http://codereview.chromium.org/40138/diff/191/1300#newcode54
Line 54: // TODO(jar): These settings should go into the derived classes, on an
as-needed basis.
On 2009/03/09 17:50:44, huanr wrote:
> Per our discussion, would be helpful to mention that the settings will be
> consolidated with FilterContext.

Done.

http://codereview.chromium.org/40138/diff/191/1296
File net/base/filter.h (right):

http://codereview.chromium.org/40138/diff/191/1296#newcode44
Line 44: // supplied by the owner of this filter.
On 2009/03/09 17:50:44, huanr wrote:
> Can you also comment on filter chain? If the filter is within the filter
chain,
> is it the contextual information of the owner of the chain, or the upper
filter
> of this filter?

Done.

http://codereview.chromium.org/40138/diff/191/1296#newcode47
Line 47: FilterContext() = 0;
On 2009/03/09 17:24:09, wtc wrote:
> We should not need to declare and define (with = 0) this constructor.

The problem was apparently that my use of
DISALLOW_COPY_AND_ASSIGN(FilterContext) at the end of the class provided *some*
constructor, and that made it a requirement that I have the constructor.  When I
got rid of the tail macro, I was able to get rid of the constructor (I didn't
realize this when I got the compile error when attempting to comply with your
request initially.)

I've gotten rid of both... and it is ok now.