Fixed playing video in component extensions.

Fixed playing video in component extensions.

Added support of range request to URLRequestSimpleJob.
BufferedDataSource uses range requests for buffering video.
But extension protocol performs these requests
using URLRequestResourceBundleJob, which is not support range requests.

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

[kirr, 2013-12-05 09:24:13]

Please review my code.
The problem is video was not played if its source file was inside a component
extension.
I've added support of range request in URLRequestSimpleJob. May be there is a
better way to do this, for example modify the URLRequestResourceBundleJob.

[kirr, 2013-12-06 14:23:42]

ping :)

[willchan no longer on Chromium, 2013-12-06 18:46:25]

The two previous engineers you emailed no longer work on the project.
Sending this to mmenke.
On Dec 6, 2013 6:23 AM, <kirr@yandex-team.ru> wrote:

> ping :)
>
> https://codereview.chromium.org/104513002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[pivanof, 2013-12-06 18:55:38]

I believe I don't have power to approve changes anyway.

On Fri, Dec 6, 2013 at 10:46 AM, William Chan (陈智昌)
<willchan@chromium.org> wrote:
> The two previous engineers you emailed no longer work on the project.
> Sending this to mmenke.
>
> On Dec 6, 2013 6:23 AM, <kirr@yandex-team.ru> wrote:
>>
>> ping :)
>>
>> https://codereview.chromium.org/104513002/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[mmenke, 2013-12-06 19:25:10]

Could you please file a bug describing the exact problem?  I'd love a simple
repro there, if you have one (If not, a description is fine).  I'm a bit
concerned that for this to work, we'd have to buffer the entire video into
memory to pass the the SimpleJob, which doesn't seem like something we should be
doing.

Assuming this is the right solution, I'm going to want unit tests for this. 
Doesn't look like we currently have any, unfortunately, but I don't think it
will be too hard to set up a test fixture.  I'm happy to give detailed
suggestions of how to go about this, to save you some time if you're not
familiar with writing network stack unit tests.

[kirr, 2013-12-15 09:17:27]

The problem is that BufferedDataSource could send a range request, but
URLRequestResourceBundleJob does not support it.
It happens when a video played in component extension and we get a cache miss. 
For example changing video position in such video will not work.
In previous versions (chromium 25) video was not started playing at all.

Here is the patch with sample extension.
https://drive.google.com/file/d/0B1i17CtHceIVZVRpNDVvbUlDRU0/edit?usp=sharing

I think I can write a unit test, but yes I'm unfamiliar with network stack test.
Should it just send a range request and check the result?

[kirr, 2013-12-15 09:17:58]

Sorry for the delay.

[mmenke, 2013-12-16 16:04:59]

On 2013/12/15 09:17:58, kirr wrote:
> Sorry for the delay.

Not a problem.  I'll try to get to this today, but there's a chance I won't get
to it until tomorrow.  Apologies in advance if that happens.

[mmenke, 2013-12-17 17:10:14]

For unit tests, I suggest we make something like url_request_unittest (Look at
the basic test fixture URLRequestTest):

We create a TestURLRequestContext with a normal job factory (Don't need a NetLog
or NetworkDelegate), and give it a custom ProtocolHandler that just returns a
subclass of URLRequestSimpleJob that has fixed data.

Then just make URLRequests and run them.  You'll need to pass them a
TestDelegate and call RunLoop().Run() after starting the URLRequest, and then
check the final status and returned data.  There are plenty of examples of this
in url_request_unittest.cc.

These tests should be very quick to write once you get the test fixture running,
so I think it's reasonable to have a bunch of them.  Please let me know if you
have any problems, I'm happy help out.

Suggested tests:

A basic one without a range (We should already have one that does this, but
looks like we don't).

One for the full range.

One with for a subrange (leave out characters at the front and end).

One with multiple ranges (Should return an error).

One with a 0-length range.

One with a range that goes backwards (Should fail?).


I'm on vacation for the next two weeks, but reviewing this should be fast enough
that I can do it while I'm off.

https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
net/url_request/url_request_simple_job.cc:63: if
(HttpUtil::ParseRangeHeader(range_header, &ranges)) {
Should we fail if we have a range header we can't parse?  I don't feel strongly
about it, but may be a little more consistent.

https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
net/url_request/url_request_simple_job.cc:68:
ERR_REQUEST_RANGE_NOT_SATISFIABLE));
Calling NotifyDone before Start() seems like a bad idea to me, particularly
since Start() is still being called, and you're relying on NotifyDone to post a
single async message that gets handled before StartAsync is called.

I suggest just copying the entire headers in this function, and parsing them in
StartAsync.  Alternatively, could parse the headers here, and just cache if
there's an error and handle it in StartAsync() to save a little memory, but I
don't think it's a big deal.

[kirr, 2013-12-18 08:03:30]

https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
net/url_request/url_request_simple_job.cc:68:
ERR_REQUEST_RANGE_NOT_SATISFIABLE));
It seems you are right.
But all URLRequestJobs with range request do the same thing.
See StreamURLRequestJob, FileSystemURLRequestJob etc.

Should we implement the different behaviour here?

On 2013/12/17 17:10:15, mmenke wrote:
> Calling NotifyDone before Start() seems like a bad idea to me, particularly
> since Start() is still being called, and you're relying on NotifyDone to post
a
> single async message that gets handled before StartAsync is called.
> 
> I suggest just copying the entire headers in this function, and parsing them
in
> StartAsync.  Alternatively, could parse the headers here, and just cache if
> there's an error and handle it in StartAsync() to save a little memory, but I
> don't think it's a big deal.

[mmenke, 2013-12-18 15:40:31]

https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
net/url_request/url_request_simple_job.cc:68:
ERR_REQUEST_RANGE_NOT_SATISFIABLE));
Thanks for pointing that out!

I think conceptually, it's a little odd calling being NotifyDone before you've
even started, and I'm a bit reluctant to add another contract.  However, it is
convenient in practice, and it looks like we already have 6 classes with this
code, or something much like it, copied and pasted, so I think the way to go is
to explicitly support this use case.

I'm not comfortable landing the CL until we have code preventing us from calling
Start when this happens.  Fortunately, that's a tiny CL, though it should have a
unit test.  I'll go ahead and put together the CL to do that myself, today or
tomorrow.  Once landed, I'll fine with this code.


On 2013/12/18 08:03:30, kirr wrote:
> It seems you are right.
> But all URLRequestJobs with range request do the same thing.
> See StreamURLRequestJob, FileSystemURLRequestJob etc.
> 
> Should we implement the different behaviour here?
> 
> On 2013/12/17 17:10:15, mmenke wrote:
> > Calling NotifyDone before Start() seems like a bad idea to me, particularly
> > since Start() is still being called, and you're relying on NotifyDone to
post
> a
> > single async message that gets handled before StartAsync is called.
> > 
> > I suggest just copying the entire headers in this function, and parsing them
> in
> > StartAsync.  Alternatively, could parse the headers here, and just cache if
> > there's an error and handle it in StartAsync() to save a little memory, but
I
> > don't think it's a big deal.
>

[mmenke, 2013-12-18 17:33:58]

Another engineer has pointed out to me just how weird this behavior really is,
and I think he's probably right.  So instead of supporting this behavior, the
right thing to do is to fix pre-existing classes that do it.  Since they all do
pretty much exactly the same thing, fixing them all shouldn't be too bad, though
we will want a test for the ERR_REQUEST_RANGE_NOT_SATISFIABLE case for
URLRequestFileJob, at least, and we don't currently seem to have any unit tests
that class.

I'm fine with you just fixing this CL, though if you have a burning desire to
fix them all, you're welcome to do so.  Since they're all near identical, I'm
thinking a parent class to parse the range request if present on
SetExtraRequestHeaders and stash it in a local, and then let Start() handle the
error logic, rather than having 7 copies of the new SetExtraRequestHeaders
function.


On 2013/12/18 15:40:31, mmenke wrote:
>
https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
> File net/url_request/url_request_simple_job.cc (right):
> 
>
https://codereview.chromium.org/104513002/diff/1/net/url_request/url_request_...
> net/url_request/url_request_simple_job.cc:68:
> ERR_REQUEST_RANGE_NOT_SATISFIABLE));
> Thanks for pointing that out!
> 
> I think conceptually, it's a little odd calling being NotifyDone before you've
> even started, and I'm a bit reluctant to add another contract.  However, it is
> convenient in practice, and it looks like we already have 6 classes with this
> code, or something much like it, copied and pasted, so I think the way to go
is
> to explicitly support this use case.
> 
> I'm not comfortable landing the CL until we have code preventing us from
calling
> Start when this happens.  Fortunately, that's a tiny CL, though it should have
a
> unit test.  I'll go ahead and put together the CL to do that myself, today or
> tomorrow.  Once landed, I'll fine with this code.
> 
> 
> On 2013/12/18 08:03:30, kirr wrote:
> > It seems you are right.
> > But all URLRequestJobs with range request do the same thing.
> > See StreamURLRequestJob, FileSystemURLRequestJob etc.
> > 
> > Should we implement the different behaviour here?
> > 
> > On 2013/12/17 17:10:15, mmenke wrote:
> > > Calling NotifyDone before Start() seems like a bad idea to me,
particularly
> > > since Start() is still being called, and you're relying on NotifyDone to
> post
> > a
> > > single async message that gets handled before StartAsync is called.
> > > 
> > > I suggest just copying the entire headers in this function, and parsing
them
> > in
> > > StartAsync.  Alternatively, could parse the headers here, and just cache
if
> > > there's an error and handle it in StartAsync() to save a little memory,
but
> I
> > > don't think it's a big deal.
> >

[mmenke, 2013-12-18 17:35:25]

On 2013/12/18 17:33:58, mmenke wrote:
> Another engineer has pointed out to me just how weird this behavior really is,
> and I think he's probably right.  So instead of supporting this behavior, the
> right thing to do is to fix pre-existing classes that do it.  Since they all
do
> pretty much exactly the same thing, fixing them all shouldn't be too bad,
though
> we will want a test for the ERR_REQUEST_RANGE_NOT_SATISFIABLE case for
> URLRequestFileJob, at least, and we don't currently seem to have any unit
tests
> that class.
> 
> I'm fine with you just fixing this CL, though if you have a burning desire to
> fix them all, you're welcome to do so.  Since they're all near identical, I'm
> thinking a parent class to parse the range request if present on
> SetExtraRequestHeaders and stash it in a local, and then let Start() handle
the
> error logic, rather than having 7 copies of the new SetExtraRequestHeaders
> function.

Stash it in a member variable, rather

[kirr, 2013-12-19 10:31:56]

Added helper class.
Please see, if I understand right.
If it's ok, I can add unit test for URLRequestSimpleJob and change other jobs
classes the same way.

[mmenke, 2013-12-19 17:05:33]

On 2013/12/19 10:31:56, kirr wrote:
> Added helper class.
> Please see, if I understand right.
> If it's ok, I can add unit test for URLRequestSimpleJob and change other jobs
> classes the same way.

Sorry I wasn't clearer, I was thinking of something more like this:

class URLRequestJobWithRangeSupport : public URLRequestJob {
  virtual void SetExtraRequestHeaders(
      const HttpRequestHeaders& headers) OVERRIDE {
    if (headers.GetHeader(HttpRequestHeaders::kRange, &range_header)) {
      if (!HttpUtil::ParseRangeHeader(range_header, &ranges_))
        ranges_.clear();
    }
  }

  const std::vector<HttpByteRange>& ranges{} { return ranges_; }

 private:
  std::vector<HttpByteRange> ranges_;
}

Then have the classes inherit from URLRequestJobWithRangeSupport, and then in
start:

Start() {
  if (ranges().size() > 1) {
    NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
               ERR_REQUEST_RANGE_NOT_SATISFIABLE));
    return;
  }
  // ....
}

We don't save a huge amount of code from the shared parent class, but given that
there are a bunch of slight variations in how the different classes handle
SetExtraRequestHeaders, I think it's best to have them share code, to reduce the
potential for bugs.


My suggestion is do two CLs:  In one, upgrade the old classes to use the new
code.  In the other, land your changes to URLRequestSimpleJob, with range
support (Along with unit tests).  This is just to try and keep each CL a little
more focused.

The CLs will both need to use the new class so one will depend on the other,
however, they can be landed in either order.  May be best to land the
URLRequestSimpleJob one first, so that we have some unit tests that cover the
new code.

[mmenke, 2013-12-19 17:11:12]

On 2013/12/19 17:05:33, mmenke wrote:
> On 2013/12/19 10:31:56, kirr wrote:
> > Added helper class.
> > Please see, if I understand right.
> > If it's ok, I can add unit test for URLRequestSimpleJob and change other
jobs
> > classes the same way.
> 
> Sorry I wasn't clearer, I was thinking of something more like this:
> 
> class URLRequestJobWithRangeSupport : public URLRequestJob {
>   virtual void SetExtraRequestHeaders(
>       const HttpRequestHeaders& headers) OVERRIDE {
>     if (headers.GetHeader(HttpRequestHeaders::kRange, &range_header)) {
>       if (!HttpUtil::ParseRangeHeader(range_header, &ranges_))
>         ranges_.clear();
>     }
>   }
> 
>   const std::vector<HttpByteRange>& ranges{} { return ranges_; }
> 
>  private:
>   std::vector<HttpByteRange> ranges_;
> }
> 
> Then have the classes inherit from URLRequestJobWithRangeSupport, and then in
> start:
> 
> Start() {
>   if (ranges().size() > 1) {
>     NotifyDone(URLRequestStatus(URLRequestStatus::FAILED,
>                ERR_REQUEST_RANGE_NOT_SATISFIABLE));
>     return;
>   }
>   // ....
> }
> 
> We don't save a huge amount of code from the shared parent class, but given
that
> there are a bunch of slight variations in how the different classes handle
> SetExtraRequestHeaders, I think it's best to have them share code, to reduce
the
> potential for bugs.
> 
> 
> My suggestion is do two CLs:  In one, upgrade the old classes to use the new
> code.  In the other, land your changes to URLRequestSimpleJob, with range
> support (Along with unit tests).  This is just to try and keep each CL a
little
> more focused.
> 
> The CLs will both need to use the new class so one will depend on the other,
> however, they can be landed in either order.  May be best to land the
> URLRequestSimpleJob one first, so that we have some unit tests that cover the
> new code.

Also, could you please file a bug for this issue, just for tracking purposes? 
Can just copy your CL description (And make it present tense), and you're done.

[kirr, 2013-12-23 13:35:50]

I've added URLRangeRequestJob. But to make this class consistent it should be
small.
It's just parsed headers and cache the result.

Also you asked to add check ranges in Start(), but I made it in StartAsync
because of this comment
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

[mmenke, 2014-01-06 15:21:05]

On 2013/12/23 13:35:50, kirr wrote:
> I've added URLRangeRequestJob. But to make this class consistent it should be
> small.
> It's just parsed headers and cache the result.
> 
> Also you asked to add check ranges in Start(), but I made it in StartAsync
> because of this comment
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur...

I apologize for not getting to this over my break.  I'll try to get to it today.
 If not, tomorrow.

[mmenke, 2014-01-07 18:51:38]

This is looking good!  Just nits.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
File net/url_request/url_range_request_job.cc (right):

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
nit:  New files shouldn't have the "(c)"

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.cc:15: : URLRequestJob(request, delegate),
range_parse_error_(false) {
range_parse_error_ should be initialized to "OK".

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
File net/url_request/url_range_request_job.h (right):

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.h:16: class NET_EXPORT URLRangeRequestJob
: public URLRequestJob {
Should have a comment here.

I suggest something like:

"URLRequestJob with support for parsing range requests.  It is up to subclasses
to handle the response, and pass any errors parsing the ranges after Start() has
been called."

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.h:25: int range_parse_error() const {
return range_parse_error_; }
I suggest range_parse_result_ instead, since "OK" isn't exactly an error.

Should also comment that this is "OK" if no range has been specified.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:23: class MockSimpleJob :
public net::URLRequestSimpleJob {
nit:  "net::" prefix not needed anywhere in this file.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:26: net::NetworkDelegate*
network_delegate)
nit:  4 less indent.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:45:
DISALLOW_COPY_AND_ASSIGN(MockSimpleJob);
nit:  Blank line before DISALLOW_COPY_AND_ASSIGN.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:51: public
net::URLRequestJobFactory::ProtocolHandler {
optional:  Suggest moving this class outside of the test fixture, just to make
the indentation easier to follow.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:63: kRangeLastPosition <
strlen(kTestData));
Instead of a CHECK, you could put things like the following at the top of the
file:

COMPILE_ASSERT(kRangeFirstPosition > 0, RangeStarTooLow);

(I believe you'd need to use something like ARRAYSIZE(kTestData) - 1 for the
last one).

I'm not sure these are really necessary, though.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:100: const std::string
data_part = std::string(kTestData + kRangeFirstPosition,
nit:  This should use kConstantNamingScheme.  I also think we could do a bit
better on the name.  Maybe kExpectedBody?

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:101: kTestData +
kRangeLastPosition + 1);
nit:  This should line up with the "kTestData" in the above line.  If there's
not enough space, you can move the "kTestData + kRangeFirstPosition" down to the
next line, and optionally move the "std::string(" down as well.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:139: EXPECT_EQ(kTestData,
delegate_.data_received());
Wonder if we should be returning an error or not in this case.  I don't feel
strongly about it, but it seems kinda weird that we're completely refusing to
handle multiple ranges, but happy to ignore bad ranges.

[kirr, 2014-01-09 07:33:21]

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
File net/url_request/url_range_request_job.cc (right):

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.cc:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
On 2014/01/07 18:51:39, mmenke wrote:
> nit:  New files shouldn't have the "(c)"

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.cc:15: : URLRequestJob(request, delegate),
range_parse_error_(false) {
On 2014/01/07 18:51:39, mmenke wrote:
> range_parse_error_ should be initialized to "OK".

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
File net/url_request/url_range_request_job.h (right):

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.h:16: class NET_EXPORT URLRangeRequestJob
: public URLRequestJob {
On 2014/01/07 18:51:39, mmenke wrote:
> Should have a comment here.
> 
> I suggest something like:
> 
> "URLRequestJob with support for parsing range requests.  It is up to
subclasses
> to handle the response, and pass any errors parsing the ranges after Start()
has
> been called."

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_rang...
net/url_request/url_range_request_job.h:25: int range_parse_error() const {
return range_parse_error_; }
On 2014/01/07 18:51:39, mmenke wrote:
> I suggest range_parse_result_ instead, since "OK" isn't exactly an error.
> 
> Should also comment that this is "OK" if no range has been specified.

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:23: class MockSimpleJob :
public net::URLRequestSimpleJob {
On 2014/01/07 18:51:39, mmenke wrote:
> nit:  "net::" prefix not needed anywhere in this file.

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:26: net::NetworkDelegate*
network_delegate)
On 2014/01/07 18:51:39, mmenke wrote:
> nit:  4 less indent.

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:45:
DISALLOW_COPY_AND_ASSIGN(MockSimpleJob);
On 2014/01/07 18:51:39, mmenke wrote:
> nit:  Blank line before DISALLOW_COPY_AND_ASSIGN.

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:51: public
net::URLRequestJobFactory::ProtocolHandler {
On 2014/01/07 18:51:39, mmenke wrote:
> optional:  Suggest moving this class outside of the test fixture, just to make
> the indentation easier to follow.

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:63: kRangeLastPosition <
strlen(kTestData));
On 2014/01/07 18:51:39, mmenke wrote:
> Instead of a CHECK, you could put things like the following at the top of the
> file:
> 
> COMPILE_ASSERT(kRangeFirstPosition > 0, RangeStarTooLow);
> 
> (I believe you'd need to use something like ARRAYSIZE(kTestData) - 1 for the
> last one).
> 
> I'm not sure these are really necessary, though.

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:100: const std::string
data_part = std::string(kTestData + kRangeFirstPosition,
On 2014/01/07 18:51:39, mmenke wrote:
> nit:  This should use kConstantNamingScheme.  I also think we could do a bit
> better on the name.  Maybe kExpectedBody?

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:101: kTestData +
kRangeLastPosition + 1);
On 2014/01/07 18:51:39, mmenke wrote:
> nit:  This should line up with the "kTestData" in the above line.  If there's
> not enough space, you can move the "kTestData + kRangeFirstPosition" down to
the
> next line, and optionally move the "std::string(" down as well.

Done.

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:139: EXPECT_EQ(kTestData,
delegate_.data_received());
As I understand we ignore invalid ranges just like http server does. So it's ok.
I return error in case of multiple ranges, because all job classes do the same.
I agree that it looks strange.

Maybe we should ignore multiple ranges (or support it where possible).
But I think it would be better to do in another cl, if it is needed.

On 2014/01/07 18:51:39, mmenke wrote:
> Wonder if we should be returning an error or not in this case.  I don't feel
> strongly about it, but it seems kinda weird that we're completely refusing to
> handle multiple ranges, but happy to ignore bad ranges.

[mmenke, 2014-01-09 15:49:23]

LGTM

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/104513002/diff/80001/net/url_request/url_requ...
net/url_request/url_request_simple_job_unittest.cc:139: EXPECT_EQ(kTestData,
delegate_.data_received());
It just seems a little weird to me that if we see request for "bytes=0-49,goat"
we just ignore the header, while if we see "bytes=0-49,70-79", then we return an
error.  But reading over the spec, you're right - it specifically says servers
must ignore range headers with invalid data.

On 2014/01/09 07:33:21, kirr wrote:
> As I understand we ignore invalid ranges just like http server does. So it's
ok.
> I return error in case of multiple ranges, because all job classes do the
same.
> I agree that it looks strange.
> 
> Maybe we should ignore multiple ranges (or support it where possible).
> But I think it would be better to do in another cl, if it is needed.
> 
> On 2014/01/07 18:51:39, mmenke wrote:
> > Wonder if we should be returning an error or not in this case.  I don't feel
> > strongly about it, but it seems kinda weird that we're completely refusing
to
> > handle multiple ranges, but happy to ignore bad ranges.
>

https://codereview.chromium.org/104513002/diff/160001/net/url_request/url_ran...
File net/url_request/url_range_request_job.h (right):

https://codereview.chromium.org/104513002/diff/160001/net/url_request/url_ran...
net/url_request/url_range_request_job.h:17: // It is up to subclasses to handle
the response,
nit:  Comma not needed.

https://codereview.chromium.org/104513002/diff/160001/net/url_request/url_ran...
net/url_request/url_range_request_job.h:18: // and pass any errors parsing the
ranges after Start() has been called.
nit:  This isn't clear.  Maybe "and deal with an errors parsing the range
request header.  This must be done after Start() has been called."

https://codereview.chromium.org/104513002/diff/160001/net/url_request/url_req...
File net/url_request/url_request_simple_job.cc (right):

https://codereview.chromium.org/104513002/diff/160001/net/url_request/url_req...
net/url_request/url_request_simple_job.cc:68: if (!ranges().empty() &&
range_parse_result() == net::OK)
nit:  "net::" not needed.

https://codereview.chromium.org/104513002/diff/160001/net/url_request/url_req...
File net/url_request/url_request_simple_job_unittest.cc (right):

https://codereview.chromium.org/104513002/diff/160001/net/url_request/url_req...
net/url_request/url_request_simple_job_unittest.cc:99: kTestData +
kRangeFirstPosition, kTestData + kRangeLastPosition + 1);
nit:  +2 indent.

[commit-bot: I haz the power, 2014-01-10 07:47:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kirr@yandex-team.ru/104513002/250001

[commit-bot: I haz the power, 2014-01-10 08:19:37]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[commit-bot: I haz the power, 2014-01-13 10:10:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kirr@yandex-team.ru/104513002/380001

[commit-bot: I haz the power, 2014-01-13 10:27:51]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[commit-bot: I haz the power, 2014-01-13 10:54:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kirr@yandex-team.ru/104513002/510003

[commit-bot: I haz the power, 2014-01-13 15:24:12]

Retried try job too often on win_rel for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-14 07:24:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kirr@yandex-team.ru/104513002/510003

[commit-bot: I haz the power, 2014-01-14 11:16:20]

Change committed as 244667