[android_webview] Make intercepted URLRequests have status codes.

[android_webview] Make intercepted URLRequests have status codes.

This changes the way AndroidStreamReaderUrlRequestJob handles requests which
don't have an InputStream. Rather than immediately failing the Job we pretend
to have HTTP response headers that say 404 Not Found and empty contents (much
like fetching the data from a real server would).

BUG=180542
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=187270

[mnaganov (inactive), 2013-03-06 13:33:10]

LGTM % nits

Do the test failures correlate with your changes?

https://codereview.chromium.org/12531002/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/12531002/diff/1/android_webview/browser/net/a...
android_webview/browser/net/android_stream_reader_url_request_job.cc:248: int
status_code,
nit: please move the first arg in line with the function name

https://codereview.chromium.org/12531002/diff/1/android_webview/browser/net/a...
android_webview/browser/net/android_stream_reader_url_request_job.cc:264:
base::Int64ToString(expected_content_size()));
nit: is this wrap needed?

[benm (inactive), 2013-03-06 17:05:11]

https://codereview.chromium.org/12531002/diff/1/android_webview/browser/net/a...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://codereview.chromium.org/12531002/diff/1/android_webview/browser/net/a...
android_webview/browser/net/android_stream_reader_url_request_job.cc:40: const
int kHTTPOk = 200;
these surely must be defined somewhere in chromium already?

https://codereview.chromium.org/12531002/diff/1/android_webview/browser/net/a...
android_webview/browser/net/android_stream_reader_url_request_job.cc:250:
std::string status("HTTP/1.1 ");
It feels weird to me we need to do this from scratch.. no helpers int net/ or
something?

[mkosiba (inactive), 2013-03-07 18:52:01]

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:40: const
int kHTTPOk = 200;
On 2013/03/06 17:05:11, benm wrote:
> these surely must be defined somewhere in chromium already?

yeath.. somewhere in uh.. drive_url_request_job. I don't think this is enough
duplication that creating a separate component / spending the effort to add this
to the content/ layer is worth it.

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:248: int
status_code,
On 2013/03/06 13:33:10, Mikhail Naganov (Chromium) wrote:
> nit: please move the first arg in line with the function name

No can do. The chromium style guide says that you need to do either:

  // 1)
  void class::f(a,b,c)
  // 2)
  void class::f(a,
                b,
                c)
  // 3)
  void class:f(
      a,
      b,
      c)

In my case 2) is not possible since the second line would be longer than the
line limit, so have to stick with 3)

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:250:
std::string status("HTTP/1.1 ");
On 2013/03/06 17:05:11, benm wrote:
> It feels weird to me we need to do this from scratch.. no helpers int net/ or
> something?

nope, I did spend a while searching. The Chromium net stack doesn't really care
about http headers, to it the headers are just data. Even the http job treats
them in a rather generic way.

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:264:
base::Int64ToString(expected_content_size()));
On 2013/03/06 13:33:10, Mikhail Naganov (Chromium) wrote:
> nit: is this wrap needed?

yes. If I join the lines they don't fit in the line length limit.

[joth, 2013-03-07 21:27:34]

On 7 March 2013 10:52, <mkosiba@chromium.org> wrote:

>
> https://chromiumcodereview.**appspot.com/12531002/diff/1/**
>
android_webview/browser/net/**android_stream_reader_url_**request_job.cc<https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browser/net/android_stream_reader_url_request_job.cc>
> File
> android_webview/browser/net/**android_stream_reader_url_**request_job.cc
> (right):
>
> https://chromiumcodereview.**appspot.com/12531002/diff/1/**
> android_webview/browser/net/**android_stream_reader_url_**
>
request_job.cc#newcode40<https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browser/net/android_stream_reader_url_request_job.cc#newcode40>
>
> android_webview/browser/net/**android_stream_reader_url_**
> request_job.cc:40:
> const int kHTTPOk = 200;
> On 2013/03/06 17:05:11, benm wrote:
>
>> these surely must be defined somewhere in chromium already?
>>
>
> yeath.. somewhere in uh.. drive_url_request_job. I don't think this is
> enough duplication that creating a separate component / spending the
> effort to add this to the content/ layer is worth it.
>

net/http would be the place to put them.
But I agree, not worth the effort here, if the net folks haven't already
found a need for such a thing


> https://chromiumcodereview.**appspot.com/12531002/diff/1/**
> android_webview/browser/net/**android_stream_reader_url_**
>
request_job.cc#newcode248<https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browser/net/android_stream_reader_url_request_job.cc#newcode248>
> android_webview/browser/net/**android_stream_reader_url_**
> request_job.cc:248:
> int status_code,
>
> On 2013/03/06 13:33:10, Mikhail Naganov (Chromium) wrote:
>
>> nit: please move the first arg in line with the function name
>>
>
> No can do. The chromium style guide says that you need to do either:
>
>   // 1)
>   void class::f(a,b,c)
>   // 2)
>   void class::f(a,
>                 b,
>                 c)
>   // 3)
>   void class:f(
>       a,
>       b,
>       c)
>
> In my case 2) is not possible since the second line would be longer than
> the line limit, so have to stick with 3)
>
> https://chromiumcodereview.**appspot.com/12531002/diff/1/**
> android_webview/browser/net/**android_stream_reader_url_**
>
request_job.cc#newcode250<https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browser/net/android_stream_reader_url_request_job.cc#newcode250>
>
> android_webview/browser/net/**android_stream_reader_url_**
> request_job.cc:250:
> std::string status("HTTP/1.1 ");
> On 2013/03/06 17:05:11, benm wrote:
>
>> It feels weird to me we need to do this from scratch.. no helpers int
>>
> net/ or
>
>> something?
>>
>
> nope, I did spend a while searching. The Chromium net stack doesn't
> really care about http headers, to it the headers are just data. Even
> the http job treats them in a rather generic way.
>
> https://chromiumcodereview.**appspot.com/12531002/diff/1/**
> android_webview/browser/net/**android_stream_reader_url_**
>
request_job.cc#newcode264<https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browser/net/android_stream_reader_url_request_job.cc#newcode264>
> android_webview/browser/net/**android_stream_reader_url_**
> request_job.cc:264:
> base::Int64ToString(expected_**content_size()));
>
> On 2013/03/06 13:33:10, Mikhail Naganov (Chromium) wrote:
>
>> nit: is this wrap needed?
>>
>
> yes. If I join the lines they don't fit in the line length limit.
>
>
https://chromiumcodereview.**appspot.com/12531002/<https://chromiumcodereview...
>

[mnaganov (inactive), 2013-03-08 09:15:47]

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
File android_webview/browser/net/android_stream_reader_url_request_job.cc
(right):

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:248: int
status_code,
On 2013/03/07 18:52:01, Martin Kosiba wrote:
> On 2013/03/06 13:33:10, Mikhail Naganov (Chromium) wrote:
> > nit: please move the first arg in line with the function name
> 
> No can do. The chromium style guide says that you need to do either:
> 
>   // 1)
>   void class::f(a,b,c)
>   // 2)
>   void class::f(a,
>                 b,
>                 c)
>   // 3)
>   void class:f(
>       a,
>       b,
>       c)
> 
> In my case 2) is not possible since the second line would be longer than the
> line limit, so have to stick with 3)

Ah, sorry. The second line visually looks as if it should fit, but perhaps it's
just longer by 1 char.

https://chromiumcodereview.appspot.com/12531002/diff/1/android_webview/browse...
android_webview/browser/net/android_stream_reader_url_request_job.cc:264:
base::Int64ToString(expected_content_size()));
On 2013/03/07 18:52:01, Martin Kosiba wrote:
> On 2013/03/06 13:33:10, Mikhail Naganov (Chromium) wrote:
> > nit: is this wrap needed?
> 
> yes. If I join the lines they don't fit in the line length limit.

Ah, it's Rietveld fooling me--it displays a lot of whitespace on the right, so
visually it looks like as if it would fit. Sorry for giving wrong suggestions!

[benm (inactive), 2013-03-08 18:12:56]

lgtm

thanks!

[commit-bot: I haz the power, 2013-03-08 18:22:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/12531002/22001

[commit-bot: I haz the power, 2013-03-08 18:31:44]

Step "update" is always a major failure.
Look at the try server FAQ for more details.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-03-11 10:28:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mkosiba@chromium.org/12531002/22001

[commit-bot: I haz the power, 2013-03-11 10:46:58]

Change committed as 187270