Naming and refactoring changes to SdchDictionaryFetcher.

Naming and refactoring changes to SdchDictionaryFetcher.

Suggested from review in CL https://codereview.chromium.org/864923002/.

BUG=None
R=mmenke@chromium.org

Committed: https://crrev.com/43def17e41e0f4a8aa0e5bf134c2182f5555f198
Cr-Commit-Position: refs/heads/master@{#314076}

[Randy Smith (Not in Mondays), 2015-01-25 00:09:42]

Matt: These are the changes you suggested; PTAL?  I wasn't able to completely
implement the pattern you had sketched because there's state in
OnReadCompleted() (and in DoReadBody() if the read returns synchronously) that's
needed for figuring out state transition mechanics and errors, specifically the
value of bytes_read.  I thought about creating a state variable on the class to
hold that value between OnReadCompleted/DoReadBody and DoReadBodyComplete, but
that felt less clean to me than just handling the state/error handling directly
in a helper function.  Let me know what you think.

[mmenke, 2015-01-26 16:40:12]

https://codereview.chromium.org/877443002/diff/20001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:114: int
SdchDictionaryFetcher::HandleReadCompletion(int bytes_read) {
I suggest moving the append and state transition into DoReadBodyComplete, and
instead just make this map a bytes_read and status() to a single result code.

Something like:

// static
GetReadResult(bytes_read, request) {
  if (!request->status().is_success()) {
    if (request->status().status() == URLReuqestStatus::CANCELED)
      return ERR_ABORTED;
    if (request->status().status() == URLReuqestStatus::FAILED)
      return request->status().error();
    // Can't currently happen.
    return ERR_FAILED;
  }
  return bytes_read;
}

That puts most of the logic in DoReadComplete.  One day, I'd really like to make
it so that Read() returns the equivalent of net error code, rather than having 3
places one has to check to reassemble the original net error code...And then we
can completely get rid of the method.

https://codereview.chromium.org/877443002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:116: if
(current_request_->status().is_success() && bytes_read < 0) {
Can this even happen?

[Randy Smith (Not in Mondays), 2015-01-26 19:52:23]

Incorporated comments; PTAL.

https://codereview.chromium.org/877443002/diff/20001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:114: int
SdchDictionaryFetcher::HandleReadCompletion(int bytes_read) {
On 2015/01/26 16:40:12, mmenke wrote:
> I suggest moving the append and state transition into DoReadBodyComplete, and
> instead just make this map a bytes_read and status() to a single result code.
> 
> Something like:
> 
> // static
> GetReadResult(bytes_read, request) {
>   if (!request->status().is_success()) {
>     if (request->status().status() == URLReuqestStatus::CANCELED)
>       return ERR_ABORTED;
>     if (request->status().status() == URLReuqestStatus::FAILED)
>       return request->status().error();
>     // Can't currently happen.
>     return ERR_FAILED;
>   }
>   return bytes_read;
> }
> 
> That puts most of the logic in DoReadComplete.  One day, I'd really like to
make
> it so that Read() returns the equivalent of net error code, rather than having
3
> places one has to check to reassemble the original net error code...And then
we
> can completely get rid of the method.

Done.

https://codereview.chromium.org/877443002/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:116: if
(current_request_->status().is_success() && bytes_read < 0) {
On 2015/01/26 16:40:12, mmenke wrote:
> Can this even happen?  

That's a fascinating question.  I thought the answer was "yes", but when I went
over my notes the actual answer was "The behavior of URLRequest depends on the
URLRequestJob implementation (of which there are many), this is the way that
ResourceLoader is written, and I'm reluctant to deviate from that as we don't
have any guarantee that behavior other than what ResourceLoader expects is
actually held to by all URLRequestJob implementations."  I'll also note that
this is what the old code did; it's not new in this CL.

https://code.google.com/p/chromium/issues/detail?id=423484 in case you want a
pointer to the URLRequest::Read interface specification and regularization bug
(ah, good intentions :-{).

[mmenke, 2015-01-26 20:14:25]

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:114: int
SdchDictionaryFetcher::GetReadResult(int bytes_read) {
optional:  May be clearer as a static method that takes the request as an
argument (Seems kinda weird to max and match a local with an argument).

If you prefer it as-is, that's fine, just make it const.

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:119: : ERR_FAILED;
This code is strange.  If current_request_->status().is_success(), the status
cannot be URLRequestStatus::CANCELED (Look at URLRequestStatus::is_success()). 
If this ever happens, should always be using ERR_FAILED...Or bytes_read,
actually, since if it's negative, it's presumably a net error, I guess?

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:121:
NetLog::TYPE_SDCH_DICTIONARY_FETCH_IMPLIED_ERROR, error);
Shouldn't we be logging all errors, not just ones where the status is success? 
Note:  I still don't think this happens.

Also, seems like the logging should also be moved into the "rv < 0" case in
DoReadBodyComplete.

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:125: error = bytes_read;
"error = bytes_read" seems a little weird.  Maybe just "return bytes_read"?  Or
rename "error" to "result" or "rv".

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:241: } else if (rv == 0) {
Should that be a DCHECK_EQ instead?  Up to you, but if we reach the else clause
and it's not 0, something's gone badly wrong.

[mmenke, 2015-01-26 20:25:44]

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:119: : ERR_FAILED;
On 2015/01/26 20:14:25, mmenke wrote:
> This code is strange.  If current_request_->status().is_success(), the status
> cannot be URLRequestStatus::CANCELED (Look at URLRequestStatus::is_success()).

> If this ever happens, should always be using ERR_FAILED...Or bytes_read,
> actually, since if it's negative, it's presumably a net error, I guess?

Oh...you're right...It can actually do that, with a sufficiently bizarre
UrlRequestJob.  If we're not using a filter, an a UrlRequestJob calls
URLRequestJob::NotifyReadSuccess(<negative number>), then we'll tell the
NetworkDelegate the request completed, but then pass the negative value on to
the URLRequest::Delegate.

I think the solution is to have a DCHECK for that, rather than to create new
NetLog events for that one particular case.

[Randy Smith (Not in Mondays), 2015-01-26 22:39:54]

Responded to your comments; PTAL?

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:114: int
SdchDictionaryFetcher::GetReadResult(int bytes_read) {
On 2015/01/26 20:14:25, mmenke wrote:
> optional:  May be clearer as a static method that takes the request as an
> argument (Seems kinda weird to max and match a local with an argument).
> 
> If you prefer it as-is, that's fine, just make it const.

Done.

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:119: : ERR_FAILED;
On 2015/01/26 20:25:44, mmenke wrote:
> On 2015/01/26 20:14:25, mmenke wrote:
> > This code is strange.  If current_request_->status().is_success(), the
status
> > cannot be URLRequestStatus::CANCELED (Look at
URLRequestStatus::is_success()).
> 
> > If this ever happens, should always be using ERR_FAILED...Or bytes_read,
> > actually, since if it's negative, it's presumably a net error, I guess?
> 
> Oh...you're right...It can actually do that, with a sufficiently bizarre
> UrlRequestJob.  If we're not using a filter, an a UrlRequestJob calls
> URLRequestJob::NotifyReadSuccess(<negative number>), then we'll tell the
> NetworkDelegate the request completed, but then pass the negative value on to
> the URLRequest::Delegate.
> 
> I think the solution is to have a DCHECK for that, rather than to create new
> NetLog events for that one particular case.

So if you want me to skip the netlog event I'm happy to do so (breadcrumbs from
previous debugging, but do ask if you want it gone), but we know that the
is_success() && bytes_read < 0 happens in the field (see notes on
http://crbug.com/416639) so if that's what you want to put in a DCHECK for, that
won't work.  If what you're asking for the DCHECK for is for status() !=
CANCELED, having looked at the implementation of URLRequestStatus::is_success,
I'm happy to just remove that (and have done so).

I'll note with some slight amusement that the check was originally put in at
your request (see https://codereview.chromium.org/723133003/#msg4), or, at
least, your pointer to AsyncResourceHandler faked me into thinking you were
requesting it :-}.

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:119: : ERR_FAILED;
On 2015/01/26 20:14:25, mmenke wrote:
> This code is strange.  If current_request_->status().is_success(), the status
> cannot be URLRequestStatus::CANCELED (Look at URLRequestStatus::is_success()).

> If this ever happens, should always be using ERR_FAILED...Or bytes_read,
> actually, since if it's negative, it's presumably a net error, I guess?

Acknowledged.

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:121:
NetLog::TYPE_SDCH_DICTIONARY_FETCH_IMPLIED_ERROR, error);
On 2015/01/26 20:14:25, mmenke wrote:
> Shouldn't we be logging all errors, not just ones where the status is success?

> Note:  I still don't think this happens.
> 
> Also, seems like the logging should also be moved into the "rv < 0" case in
> DoReadBodyComplete.

This logging was breadcrumbs from trying to figure out http://crbug.com/416639,
and I'm happy to remove it if you'd like.  I think I'll still wait for a second
request, as I have the impression you don't have 416639 at top of mind--this
wasn't just any error, this was a weird, how the heck can this be happening
error that showed up fairly rarely in the field and not at all locally.

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:125: error = bytes_read;
On 2015/01/26 20:14:25, mmenke wrote:
> "error = bytes_read" seems a little weird.  Maybe just "return bytes_read"? 
Or
> rename "error" to "result" or "rv".

Done.

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:241: } else if (rv == 0) {
On 2015/01/26 20:14:25, mmenke wrote:
> Should that be a DCHECK_EQ instead?  Up to you, but if we reach the else
clause
> and it's not 0, something's gone badly wrong.

Looking at the logic, it was a bit too obvious to me that it had to be 0 (I'm
willing to trust that the union of the sets <0, ==0, and >0 encompasses all
possible integers), so I didn't bother with a DCHECK.  I did rewrite the logic
to use early returns and keep the state transitions as linear in the code as
possible.

[mmenke, 2015-01-30 16:20:44]

LGTM

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/40001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:119: : ERR_FAILED;
On 2015/01/26 22:39:54, rdsmith wrote:
> On 2015/01/26 20:25:44, mmenke wrote:
> > On 2015/01/26 20:14:25, mmenke wrote:
> > > This code is strange.  If current_request_->status().is_success(), the
> status
> > > cannot be URLRequestStatus::CANCELED (Look at
> URLRequestStatus::is_success()).
> > 
> > > If this ever happens, should always be using ERR_FAILED...Or bytes_read,
> > > actually, since if it's negative, it's presumably a net error, I guess?
> > 
> > Oh...you're right...It can actually do that, with a sufficiently bizarre
> > UrlRequestJob.  If we're not using a filter, an a UrlRequestJob calls
> > URLRequestJob::NotifyReadSuccess(<negative number>), then we'll tell the
> > NetworkDelegate the request completed, but then pass the negative value on
to
> > the URLRequest::Delegate.
> > 
> > I think the solution is to have a DCHECK for that, rather than to create new
> > NetLog events for that one particular case.
> 
> So if you want me to skip the netlog event I'm happy to do so (breadcrumbs
from
> previous debugging, but do ask if you want it gone), but we know that the
> is_success() && bytes_read < 0 happens in the field (see notes on
> http://crbug.com/416639) so if that's what you want to put in a DCHECK for,
that
> won't work.  If what you're asking for the DCHECK for is for status() !=
> CANCELED, having looked at the implementation of URLRequestStatus::is_success,
> I'm happy to just remove that (and have done so).

I thought the issue was URLRequest::Read returning false in weird cases, not
status being SUCCESS with negative read sizes?

> I'll note with some slight amusement that the check was originally put in at
> your request (see https://codereview.chromium.org/723133003/#msg4), or, at
> least, your pointer to AsyncResourceHandler faked me into thinking you were
> requesting it :-}.

In my defense, neither of my suggestions used status.is_success() - I was
suggesting replacing the block entirely, and checking the cancelled and failed
statuses directly.  I also hadn't realized this was turning success cases into
errors, rather than turning failure cases that may have no error code set into
errors.

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:67: if ((!fetch_queue_.empty() &&
fetch_queue_.back() == dictionary_url) ||
Need to include the GURL header.

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.h:40: const BoundNetLog& net_log)>
While you're here, should include net/base/net_log.h, ref_counted.h, and
macros.h.

Should also forward declare GURL,

[Randy Smith (Not in Mondays), 2015-01-30 19:21:58]

Thanks!

(I implemented the missing declaration call outs a bit differently than what you
suggested; I'll pause before throwing at the CQ to give you a chance to correct
if you want, but I think this is fine.  See below.)

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:67: if ((!fetch_queue_.empty() &&
fetch_queue_.back() == dictionary_url) ||
On 2015/01/30 16:20:43, mmenke wrote:
> Need to include the GURL header.

Done in header file.

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.h:40: const BoundNetLog& net_log)>
On 2015/01/30 16:20:44, mmenke wrote:
> While you're here, should include net/base/net_log.h, ref_counted.h, and
> macros.h.

ref_counted.h & macros.h: Done.  I couldn't figure out why I should include
net_log.h here instead of forward declaring BoundNetLog and including net_log.h
in the .cc file, so I did that instead (which may have been what you meant).

> Should also forward declare GURL, 

Actually, I think I need to include the header file for GURL, since I've got
std::queue<GURL> and std::set<GURL> below.

[mmenke, 2015-01-30 19:27:52]

LGTM

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
File net/url_request/sdch_dictionary_fetcher.h (right):

https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.h:40: const BoundNetLog& net_log)>
On 2015/01/30 19:21:58, rdsmith wrote:
> On 2015/01/30 16:20:44, mmenke wrote:
> > While you're here, should include net/base/net_log.h, ref_counted.h, and
> > macros.h.
> 
> ref_counted.h & macros.h: Done.  I couldn't figure out why I should include
> net_log.h here instead of forward declaring BoundNetLog and including
net_log.h
> in the .cc file, so I did that instead (which may have been what you meant).

You're right.  Almost everything in net/ keeps a BoundNetLog a class variable,
so just assumed it was the case here as well

> 
> > Should also forward declare GURL, 
> 
> Actually, I think I need to include the header file for GURL, since I've got
> std::queue<GURL> and std::set<GURL> below.
> 

Works for me, but I don't think it's needed - the compiler doesn't need to know
the size of the GURL to interpret that, which is the same reason you can forward
declare stuff you store in a scoped_ptr<Foo>.

[mmenke, 2015-01-30 19:28:56]

On 2015/01/30 19:27:52, mmenke wrote:
> LGTM
> 
>
https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
> File net/url_request/sdch_dictionary_fetcher.h (right):
> 
>
https://codereview.chromium.org/877443002/diff/80001/net/url_request/sdch_dic...
> net/url_request/sdch_dictionary_fetcher.h:40: const BoundNetLog& net_log)>
> On 2015/01/30 19:21:58, rdsmith wrote:
> > On 2015/01/30 16:20:44, mmenke wrote:
> > > While you're here, should include net/base/net_log.h, ref_counted.h, and
> > > macros.h.
> > 
> > ref_counted.h & macros.h: Done.  I couldn't figure out why I should include
> > net_log.h here instead of forward declaring BoundNetLog and including
> net_log.h
> > in the .cc file, so I did that instead (which may have been what you meant).
> 
> You're right.  Almost everything in net/ keeps a BoundNetLog a class variable,
> so just assumed it was the case here as well
> 
> > 
> > > Should also forward declare GURL, 
> > 
> > Actually, I think I need to include the header file for GURL, since I've got
> > std::queue<GURL> and std::set<GURL> below.
> > 
> 
> Works for me, but I don't think it's needed - the compiler doesn't need to
know
> the size of the GURL to interpret that, which is the same reason you can
forward
> declare stuff you store in a scoped_ptr<Foo>.

Erm...  "To interpret that" == "To figure out the size of that"

[Randy Smith (Not in Mondays), 2015-01-31 02:22:22]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2015-01-31 02:23:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877443002/120001

[commit-bot: I haz the power, 2015-01-31 09:28:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-31 09:28:48]

Try jobs failed on following builders:
  win8_chromium_gn_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_gn_...)

[Randy Smith (Not in Mondays), 2015-01-31 23:37:38]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2015-01-31 23:38:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/877443002/120001

[commit-bot: I haz the power, 2015-02-01 01:18:40]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2015-02-01 01:19:22]

Patchset 7 (id:??) landed as
https://crrev.com/43def17e41e0f4a8aa0e5bf134c2182f5555f198
Cr-Commit-Position: refs/heads/master@{#314076}