Make sure the job state matches the return value....

Fix ReadRawData to make the job state match the return value.

The comment for ReadRawData reads:
  // If returning true, data was read from the job.  buf will contain
  // the data, and bytes_read will receive the number of bytes read.
...
  // If returning false, an error occurred or an async IO is now pending.
  // If async IO is pending, the status of the request will be
  // URLRequestStatus::IO_PENDING, and buf must remain available until the
  // operation is completed.  See comments on URLRequest::Read for more info.

I interpret this to mean that an implementer should never set status to IO_PENDING and return true, but that's what the slow download job does:
...
    SetStatus(URLRequestStatus(URLRequestStatus::IO_PENDING, 0));
    DCHECK(!is_done());
    return true;
...

Unfortunately this class is part of the Download UI tests which are already flaky/broken so it's not possible to verify that this change doesn't further break those tests. But the current implementation is clearly broken and should be fixed.

BUG=50399
TEST=ran ui_tests, verified no additional breakage

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

[tony, 2010-07-27 22:48:48]

LGTM.  Are the download tests the only tests that use this url request type?

[bmcquade1, 2010-07-27 23:53:49]

I did a quick source grep:

grepsvn -l --exclude-dir=out URLRequestSlowDownloadJob *
chrome/browser/net/url_request_slow_download_job.h
chrome/browser/net/url_request_mock_util.cc
chrome/browser/net/url_request_slow_download_job.cc
chrome/browser/download/download_uitest.cc

So we need to look at
chrome/browser/net/url_request_mock_util.cc
chrome/browser/download/download_uitest.cc

url_request_mock_util.cc provides a method SetUrlRequestMocksEnabled which
registers the job's handler, and is called in a few places:
chrome/browser/automation/automation_resource_message_filter.cc (doesn't use
the slow download class)
chrome/test/in_process_browser_test.cc (doesn't use the slow download class)

download_uitest I've already tested.

So I think it's safe to assume that no other tests are impacted by this
change.

Thanks for reviewing the change!

-bryan


On Tue, Jul 27, 2010 at 6:48 PM, <tony@chromium.org> wrote:

> LGTM.  Are the download tests the only tests that use this url request
> type?
>
> http://codereview.chromium.org/3052016/show
>

[eroman, 2010-07-27 23:56:52]

lgtm