Improve modularity of recent SDCH dictionary load checkin...

Improve modularity of recent SDCH dictionary load checkin

I felt guilty that I allowed url_request_job to have some logic
in it about whether it can load an SDCH dictinary or not.  This
patch pushed the logic back into sdch_manager (where it belongs).

r=wtc

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

[wtc, 2009-02-17 18:33:17]

As we discussed in person, you should not need the
sdch_request_url_ member.

http://codereview.chromium.org/20379/diff/1007/8
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/20379/diff/1007/8#newcode464
Line 464: sdch_request_url_ = request_->url();
Aha, this should be
    sdch_request_url_ = request_info_.url;

http://codereview.chromium.org/20379/diff/1007/9
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/20379/diff/1007/9#newcode86
Line 86: // Copy of the request's URL (whose response suggested the SDCH
dictionary).
Nit: please add a blank line above this line.

http://codereview.chromium.org/20379/diff/1007/9#newcode88
Line 88: GURL sdch_request_url_;
If sdch_request_url_ is a copy of request_info_.url, you
should be able to just use request_info_.url, even in the
URLRequestHttpJob destructor.  request_info_.url is valid
as long as the Start method has been called.  You can add
a CHECK or DCHECK to verify that sdch_request_url_ still
equals request_info_.url in the URLRequestHttpJob destructor.

[jar (doing other things), 2009-02-17 18:47:08]

http://codereview.chromium.org/20379/diff/1007/8
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/20379/diff/1007/8#newcode464
Line 464: sdch_request_url_ = request_->url();
On 2009/02/17 18:33:17, wtc wrote:
> Aha, this should be
>     sdch_request_url_ = request_info_.url;

Done.

http://codereview.chromium.org/20379/diff/1007/9
File net/url_request/url_request_http_job.h (right):

http://codereview.chromium.org/20379/diff/1007/9#newcode86
Line 86: // Copy of the request's URL (whose response suggested the SDCH
dictionary).
On 2009/02/17 18:33:17, wtc wrote:
> Nit: please add a blank line above this line.

I removed the field.

http://codereview.chromium.org/20379/diff/1007/9#newcode88
Line 88: GURL sdch_request_url_;
On 2009/02/17 18:33:17, wtc wrote:
> If sdch_request_url_ is a copy of request_info_.url, you
> should be able to just use request_info_.url, even in the
> URLRequestHttpJob destructor.  request_info_.url is valid
> as long as the Start method has been called.  You can add
> a CHECK or DCHECK to verify that sdch_request_url_ still
> equals request_info_.url in the URLRequestHttpJob destructor.

Done.

[wtc, 2009-02-17 18:53:16]

LGTM.

http://codereview.chromium.org/20379/diff/15/16
File net/url_request/url_request_http_job.cc (right):

http://codereview.chromium.org/20379/diff/15/16#newcode68
Line 68: SdchManager::Global()->FetchDictionary(request_info_.url,
I guess you can add a DCHECK above this line:
    DCHECK(request_info_.url.is_valid());

http://codereview.chromium.org/20379/diff/15/16#newcode459
Line 459: sdch_dictionary_url_ = request_info_.url.Resolve(url_text);
By the way, I found that we are always using request_->url()
in this class.  request_info_ is only created for the
HttpTransaction layer below this class.  So you should be
able to continue to use request_->url() here.  It's just that
request_ is reset to a NULL pointer before the destructor
is called, so in the destructor we will need to use
request_info_.url.

[wtc, 2009-02-17 19:02:02]

LGTM.  Thanks!