Shift URLRequest::Read API contract used by fetcher to ResourceLoader's.

Shift URLRequest::Read API contract used by fetcher to ResourceLoader's.

BUG=433036
R=mmenke@chromium.org

Committed: https://crrev.com/651c52db32c31da522c7694d716bbb1dc74d734c
Cr-Commit-Position: refs/heads/master@{#304641}

[Randy Smith (Not in Mondays), 2014-11-13 21:18:18]

Bence: CC'd FYI, but this is more about the URLRequest::Read() interface, so
making Matt primary reviewer.

Matt: PTAL?

[mmenke, 2014-11-13 21:55:06]

quick comments.

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

https://codereview.chromium.org/723133003/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:206: return
current_request_->status().error();
Hrm...  if it's possible for is_success() is true, but bytes_read  to be less
than 0, I don't think error() will have a meaningful value.

https://codereview.chromium.org/723133003/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:206: return
current_request_->status().error();
Should have a comment by the old enum that it's obsolete, or comment it out with
a comment, or something.

[Randy Smith (Not in Mondays), 2014-11-13 22:05:22]

PTAL?

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

https://codereview.chromium.org/723133003/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:206: return
current_request_->status().error();
On 2014/11/13 21:55:05, mmenke wrote:
> Hrm...  if it's possible for is_success() is true, but bytes_read  to be less
> than 0, I don't think error() will have a meaningful value.

It's a fair concern, but ResourceLoader has the same problem:

void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read) {
  DCHECK_EQ(request_.get(), unused);
  VLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
          << " bytes_read = " << bytes_read;

  // bytes_read == -1 always implies an error.
  if (bytes_read == -1 || !request_->status().is_success()) {
    ResponseCompleted();
    return;
  }

I'm not sure how to fix it, either.  Set bytes_read to zero if is_success() is
true?

https://codereview.chromium.org/723133003/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:206: return
current_request_->status().error();
On 2014/11/13 21:55:05, mmenke wrote:
> Should have a comment by the old enum that it's obsolete, or comment it out
with
> a comment, or something.

Whoops, sorry, updated histograms.xml, but not the enum.

[mmenke, 2014-11-13 22:23:44]

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

https://codereview.chromium.org/723133003/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:206: return
current_request_->status().error();
On 2014/11/13 22:05:22, rdsmith wrote:
> On 2014/11/13 21:55:05, mmenke wrote:
> > Hrm...  if it's possible for is_success() is true, but bytes_read  to be
less
> > than 0, I don't think error() will have a meaningful value.
> 
> It's a fair concern, but ResourceLoader has the same problem:
> 
> void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int bytes_read)
{
>   DCHECK_EQ(request_.get(), unused);
>   VLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
>           << " bytes_read = " << bytes_read;
> 
>   // bytes_read == -1 always implies an error.
>   if (bytes_read == -1 || !request_->status().is_success()) {
>     ResponseCompleted();
>     return;
>   }
> 
> I'm not sure how to fix it, either.  Set bytes_read to zero if is_success() is
> true?

I suggest we channel AsyncResourceHandler here
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...)

  int error_code = status.error();
  if (status.status() == net::URLRequestStatus::CANCELED &&
      error_code == net::OK) {
    error_code = net::ERR_ABORTED;
  } else if (status.status() == net::URLRequestStatus::FAILED &&
             error_code == net::OK) {
    error_code = net::ERR_FAILED;
  }

Or perhaps just:

  if (status.error() == net::OK) {
    if (status.status() == net::URLRequestStatus::CANCELED)
      return ERR_ABORTED;
    return ERR_URL_REQUEST_IS_WEIRD;  // Also known is ERR_FAILED
  }
  return status.error();

[Randy Smith (Not in Mondays), 2014-11-15 21:25:30]

PTAL?

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

https://codereview.chromium.org/723133003/diff/20001/net/url_request/sdch_dic...
net/url_request/sdch_dictionary_fetcher.cc:206: return
current_request_->status().error();
On 2014/11/13 22:23:43, mmenke wrote:
> On 2014/11/13 22:05:22, rdsmith wrote:
> > On 2014/11/13 21:55:05, mmenke wrote:
> > > Hrm...  if it's possible for is_success() is true, but bytes_read  to be
> less
> > > than 0, I don't think error() will have a meaningful value.
> > 
> > It's a fair concern, but ResourceLoader has the same problem:
> > 
> > void ResourceLoader::OnReadCompleted(net::URLRequest* unused, int
bytes_read)
> {
> >   DCHECK_EQ(request_.get(), unused);
> >   VLOG(1) << "OnReadCompleted: \"" << request_->url().spec() << "\""
> >           << " bytes_read = " << bytes_read;
> > 
> >   // bytes_read == -1 always implies an error.
> >   if (bytes_read == -1 || !request_->status().is_success()) {
> >     ResponseCompleted();
> >     return;
> >   }
> > 
> > I'm not sure how to fix it, either.  Set bytes_read to zero if is_success()
is
> > true?
> 
> I suggest we channel AsyncResourceHandler here
>
(https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/lo...)
> 
>   int error_code = status.error();
>   if (status.status() == net::URLRequestStatus::CANCELED &&
>       error_code == net::OK) {
>     error_code = net::ERR_ABORTED;
>   } else if (status.status() == net::URLRequestStatus::FAILED &&
>              error_code == net::OK) {
>     error_code = net::ERR_FAILED;
>   }
> 
> Or perhaps just:
> 
>   if (status.error() == net::OK) {
>     if (status.status() == net::URLRequestStatus::CANCELED)
>       return ERR_ABORTED;
>     return ERR_URL_REQUEST_IS_WEIRD;  // Also known is ERR_FAILED
>   }
>   return status.error();

Sounds good.  I've tried to duplicate that logic's effect in a somewhat clearer
flow for this routine; let me know what you think.

[mmenke, 2014-11-17 15:24:58]

LGTM

https://codereview.chromium.org/723133003/diff/100001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/723133003/diff/100001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher.cc:229:
dictionary_fetched_callback_.Run(dictionary_, current_request_->url());
Should we be recording an error in UMA here?

[Randy Smith (Not in Mondays), 2014-11-17 18:10:27]

One more review for the new net logging?

[mmenke, 2014-11-17 18:12:31]

LGTM

https://codereview.chromium.org/723133003/diff/140001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/723133003/diff/140001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher.cc:213: net::Error error(
optional:  Think "=" is a little more common when initializing simple types.

[Randy Smith (Not in Mondays), 2014-11-17 18:14:46]

Thanks!

https://codereview.chromium.org/723133003/diff/140001/net/url_request/sdch_di...
File net/url_request/sdch_dictionary_fetcher.cc (right):

https://codereview.chromium.org/723133003/diff/140001/net/url_request/sdch_di...
net/url_request/sdch_dictionary_fetcher.cc:213: net::Error error(
On 2014/11/17 18:12:31, mmenke wrote:
> optional:  Think "=" is a little more common when initializing simple types.

Done.

[Randy Smith (Not in Mondays), 2014-11-17 20:12:01]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-11-17 20:13:06]

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

[commit-bot: I haz the power, 2014-11-17 20:20:34]

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

[commit-bot: I haz the power, 2014-11-17 20:20:35]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Randy Smith (Not in Mondays), 2014-11-17 21:16:01]

rdsmith@chromium.org changed reviewers:
+ asvitkine@chromium.org

[Randy Smith (Not in Mondays), 2014-11-17 21:16:01]

Whoops, forgot about histograms.xml.  Alexei, would you take a look?

[Alexei Svitkine (slow), 2014-11-17 22:11:13]

lgtm with a suggestion

https://codereview.chromium.org/723133003/diff/160001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/723133003/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:53558: +  <int value="38"
label="defunct">
Nit: I'd keep the old name (so old data can be decoded), but you can add
(defunct) at the end of it instead.

[Randy Smith (Not in Mondays), 2014-11-18 17:02:39]

Thanks!

https://codereview.chromium.org/723133003/diff/160001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/723133003/diff/160001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:53558: +  <int value="38"
label="defunct">
On 2014/11/17 22:11:13, Alexei Svitkine wrote:
> Nit: I'd keep the old name (so old data can be decoded), but you can add
> (defunct) at the end of it instead.

Done.

[Randy Smith (Not in Mondays), 2014-11-18 17:02:47]

The CQ bit was checked by rdsmith@chromium.org

[commit-bot: I haz the power, 2014-11-18 17:03:28]

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

[commit-bot: I haz the power, 2014-11-18 18:10:11]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2014-11-18 18:11:57]

Patchset 10 (id:??) landed as
https://crrev.com/651c52db32c31da522c7694d716bbb1dc74d734c
Cr-Commit-Position: refs/heads/master@{#304641}