[fsp] Improve performance for reading small chunks of data.

[fsp] Improve performance for reading small chunks of data.

This patch adds a field to the BufferedFileStreamReader constructor, which says
how much bytes in total are going to be requested. This information is used for
an efficient buffering strategy.

TEST=unit_tests: *BufferingFileStreamReader*
BUG=398338
Committed: https://crrev.com/85aa9d79cb455b47fc383ce5a64bb7da3800ba11
Cr-Commit-Position: refs/heads/master@{#295177}

[mtomasz, 2014-08-22 05:01:28]

@hashimoto: PTAL. This is the approach we're discussing some time ago.

[hashimoto, 2014-08-22 06:53:42]

Seems to be the right direction, but please get reviewed first by File API
owners.

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/blob/blob...
webkit/browser/blob/blob_url_request_job.cc:580: item.length() -
additional_offset,
BlobData::Item is typedef-ed to DataElement and DataElement is using uint64 to
store offset and length.
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/data...

DataElement sets kuint64max to length if no value is provided when the data is
set.
I think you should replace int64 in FileStreamReader with uint64, or replace
uint64 in DataElement with int64 to make it work correctly.

The same goes for other parts.

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:250: // instance of the file stream
reader. If unknown, then equal to -1.
Why don't you use kuint64max (or kint64max) to be consistent with existing code?
This way, you don't have to handle -1 specially.

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_url_request_job.cc:222:
byte_range_.last_byte_position() - byte_range_.first_byte_position() + 1,
You can use remaining_bytes_.

[mtomasz, 2014-08-25 03:32:31]

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/blob/blob...
webkit/browser/blob/blob_url_request_job.cc:580: item.length() -
additional_offset,
On 2014/08/22 06:53:42, hashimoto wrote:
> BlobData::Item is typedef-ed to DataElement and DataElement is using uint64 to
> store offset and length.
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/data...
> 
> DataElement sets kuint64max to length if no value is provided when the data is
> set.
> I think you should replace int64 in FileStreamReader with uint64, or replace
> uint64 in DataElement with int64 to make it work correctly.
> 
> The same goes for other parts.

This is a good point. However, int64 are used all over fileapi code, so I think
this should be fixed separately. Filed crbug.com/406967.

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:250: // instance of the file stream
reader. If unknown, then equal to -1.
On 2014/08/22 06:53:42, hashimoto wrote:
> Why don't you use kuint64max (or kint64max) to be consistent with existing
code?
> This way, you don't have to handle -1 specially.

Seems that kuint64max and kint64max are deprecated. So instead of simple -1 we
would have the following code:

#include <limits>
(...)
CreateFileStreamReader(..., numeric_limits<uint64>::max(), ...)

This is just way longer than -1. Also IMHO less readable.

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_url_request_job.cc:222:
byte_range_.last_byte_position() - byte_range_.first_byte_position() + 1,
On 2014/08/22 06:53:42, hashimoto wrote:
> You can use remaining_bytes_.

Done.

[mtomasz, 2014-08-25 03:44:49]

PTAL.

@hashimoto:
  chrome/browser/chromeos/file_system_provider/*
  chrome/browser/chromeos/drive/fileapi/*

@tzik:
  chrome/browser/media_galleries/fileapi/*
  chrome/browser/sync_file_system/local/*
  content/browser/fileapi/upload_file_system_file_element_reader.cc
  webkit/browser/fileapi/*

@noamsml:
  chrome/browser/local_discovery/storage/*

@kinaba:
  chrome/browser/chromeos/fileapi/*

@darin:
  content/public/test/test_file_system_backend.*
  webkit/browser/blob/blob_url_request_job.cc

Thanks.

[tzik, 2014-08-25 08:00:41]

lgtm

[kinaba, 2014-08-25 08:07:54]

lgtm

https://codereview.chromium.org/470323003/diff/40001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/470323003/diff/40001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_backend.h:105: // ERR_UPLOAD_FILE_CHANGED
error.
Could you also add a comment on |length| here in addition to FileSystemContext?

[mtomasz, 2014-08-25 08:30:13]

https://codereview.chromium.org/470323003/diff/40001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/470323003/diff/40001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_backend.h:105: // ERR_UPLOAD_FILE_CHANGED
error.
On 2014/08/25 08:07:54, kinaba wrote:
> Could you also add a comment on |length| here in addition to
FileSystemContext?

Done.

[mtomasz, 2014-08-25 08:40:08]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-08-25 08:40:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/470323003/60001

[mtomasz, 2014-08-25 08:55:10]

The CQ bit was unchecked by mtomasz@chromium.org

[Noam Samuel, 2014-08-25 18:03:36]

On 2014/08/25 08:55:10, mtomasz wrote:
> The CQ bit was unchecked by mailto:mtomasz@chromium.org

local_discovery lgtm

[mtomasz, 2014-09-01 00:15:34]

On 2014/08/25 18:03:36, Noam Samuel wrote:
> On 2014/08/25 08:55:10, mtomasz wrote:
> > The CQ bit was unchecked by mailto:mtomasz@chromium.org
> 
> local_discovery lgtm

@darin: Ping.
@hashimoto: Ping.

[hashimoto, 2014-09-01 11:01:01]

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/blob/blob...
File webkit/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/blob/blob...
webkit/browser/blob/blob_url_request_job.cc:580: item.length() -
additional_offset,
On 2014/08/25 03:32:31, mtomasz wrote:
> On 2014/08/22 06:53:42, hashimoto wrote:
> > BlobData::Item is typedef-ed to DataElement and DataElement is using uint64
to
> > store offset and length.
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/webkit/common/data...
> > 
> > DataElement sets kuint64max to length if no value is provided when the data
is
> > set.
> > I think you should replace int64 in FileStreamReader with uint64, or replace
> > uint64 in DataElement with int64 to make it work correctly.
> > 
> > The same goes for other parts.
> 
> This is a good point. However, int64 are used all over fileapi code, so I
think
> this should be fixed separately. Filed crbug.com/406967.

Then could you fix crbug.com/406967 first, or implement some check to places
where uint64->int64 conversion happens?

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
File webkit/browser/fileapi/file_system_context.h (right):

https://codereview.chromium.org/470323003/diff/20001/webkit/browser/fileapi/f...
webkit/browser/fileapi/file_system_context.h:250: // instance of the file stream
reader. If unknown, then equal to -1.
On 2014/08/25 03:32:31, mtomasz wrote:
> On 2014/08/22 06:53:42, hashimoto wrote:
> > Why don't you use kuint64max (or kint64max) to be consistent with existing
> code?
> > This way, you don't have to handle -1 specially.
> 
> Seems that kuint64max and kint64max are deprecated. So instead of simple -1 we
> would have the following code:
> 
> #include <limits>
> (...)
> CreateFileStreamReader(..., numeric_limits<uint64>::max(), ...)
> 
> This is just way longer than -1. Also IMHO less readable.

I'm asking about consistency with the existing code, not about the length or the
readability of the code.
Also, using -1 is less intuitive than numeric_limits<uint64>::max(). (at least
-1 should be a constant.)

[mtomasz, 2014-09-02 05:13:27]

> > This is a good point. However, int64 are used all over fileapi code, so I
> think
> > this should be fixed separately. Filed crbug.com/406967.
> 
> Then could you fix crbug.com/406967 first, or implement some check to places
> where uint64->int64 conversion happens?

Added a checked_cast in places related to this patch.

> I'm asking about consistency with the existing code, not about the length or
the
> readability of the code.

You asked me why didn't I use it, and I answered why. The reason is length and
readability. What kind of answer do you expect from me?

As for consistency, -1 is already used in
webkit/browser/blob/blob_url_request_job.cc for unknown length, as well as in
webkit. It's a common paradigm.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> Also, using -1 is less intuitive than numeric_limits<uint64>::max(). (at least
> -1 should be a constant.)

We use -1 for unknown values all over Chromium code base. I added a constant as
you requested.

[hashimoto, 2014-09-03 11:15:54]

On 2014/09/02 05:13:27, mtomasz wrote:
> > > This is a good point. However, int64 are used all over fileapi code, so I
> > think
> > > this should be fixed separately. Filed crbug.com/406967.
> > 
> > Then could you fix crbug.com/406967 first, or implement some check to places
> > where uint64->int64 conversion happens?
> 
> Added a checked_cast in places related to this patch.
> 
> > I'm asking about consistency with the existing code, not about the length or
> the
> > readability of the code.
> 
> You asked me why didn't I use it, and I answered why. The reason is length and
> readability. What kind of answer do you expect from me?
I was expecting a more logical answer from you which describes why you are using
-1 which results in inconsistency with existing code (what is the reasoning to
prefer consistency with code under third_party/WebKit instead of code under
webkit/?) and a lot of length==-1 checks in the implementation.
> 
> As for consistency, -1 is already used in
> webkit/browser/blob/blob_url_request_job.cc for unknown length, as well as in
> webkit. It's a common paradigm.
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> 
> > Also, using -1 is less intuitive than numeric_limits<uint64>::max(). (at
least
> > -1 should be a constant.)
> 
> We use -1 for unknown values all over Chromium code base. I added a constant
as
> you requested.

[mtomasz, 2014-09-03 14:40:37]

On 2014/09/03 11:15:54, hashimoto wrote:
> On 2014/09/02 05:13:27, mtomasz wrote:
> > > > This is a good point. However, int64 are used all over fileapi code, so
I
> > > think
> > > > this should be fixed separately. Filed crbug.com/406967.
> > > 
> > > Then could you fix crbug.com/406967 first, or implement some check to
places
> > > where uint64->int64 conversion happens?
> > 
> > Added a checked_cast in places related to this patch.
> > 
> > > I'm asking about consistency with the existing code, not about the length
or
> > the
> > > readability of the code.
> > 
> > You asked me why didn't I use it, and I answered why. The reason is length
and
> > readability. What kind of answer do you expect from me?
> I was expecting a more logical answer from you which describes why you are
using
> -1 which results in inconsistency with existing code (what is the reasoning to
> prefer consistency with code under third_party/WebKit instead of code under
> webkit/?) and a lot of length==-1 checks in the implementation.

To be honest, I didn't notice that inconsistency. It seems that max values are
used in exactly *2* places in the entire /webkit/browser/fileapi/, all related
to quota. While we use -1 all over Chrome code base for unknown values.

https://code.google.com/p/chromium/codesearch#search/&q=numeric_limits%20pack...
0 results for numeric_limits in /webkit/browser/fileapi/.

https://code.google.com/p/chromium/codesearch#search/&q=kuint64max%20%20packa...
0 results for kuint64max in /webkit/browser/fileapi/.

https://code.google.com/p/chromium/codesearch#search/&q=kint64max%20%20packag...
2 results for kuint64max in /webkit/browser/fileapi/.

Since we agreed on using a suggested by you constant for -1, I'm not going to
discuss further on this issue.
If you have any other concerns about this CL, let me know. Otherwise, I'd like
to proceed.

> > 
> > As for consistency, -1 is already used in
> > webkit/browser/blob/blob_url_request_job.cc for unknown length, as well as
in
> > webkit. It's a common paradigm.
> > 
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > 
> > > Also, using -1 is less intuitive than numeric_limits<uint64>::max(). (at
> least
> > > -1 should be a constant.)
> > 
> > We use -1 for unknown values all over Chromium code base. I added a constant
> as
> > you requested.

[mtomasz, 2014-09-04 04:07:28]

On 2014/09/03 14:40:37, mtomasz wrote:
> On 2014/09/03 11:15:54, hashimoto wrote:
> > On 2014/09/02 05:13:27, mtomasz wrote:
> > > > > This is a good point. However, int64 are used all over fileapi code,
so
> I
> > > > think
> > > > > this should be fixed separately. Filed crbug.com/406967.
> > > > 
> > > > Then could you fix crbug.com/406967 first, or implement some check to
> places
> > > > where uint64->int64 conversion happens?
> > > 
> > > Added a checked_cast in places related to this patch.
> > > 
> > > > I'm asking about consistency with the existing code, not about the
length
> or
> > > the
> > > > readability of the code.
> > > 
> > > You asked me why didn't I use it, and I answered why. The reason is length
> and
> > > readability. What kind of answer do you expect from me?
> > I was expecting a more logical answer from you which describes why you are
> using
> > -1 which results in inconsistency with existing code (what is the reasoning
to
> > prefer consistency with code under third_party/WebKit instead of code under
> > webkit/?) and a lot of length==-1 checks in the implementation.
> 
> To be honest, I didn't notice that inconsistency. It seems that max values are
> used in exactly *2* places in the entire /webkit/browser/fileapi/, all related
> to quota. While we use -1 all over Chrome code base for unknown values.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=numeric_limits%20pack...
> 0 results for numeric_limits in /webkit/browser/fileapi/.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=kuint64max%20%20packa...
> 0 results for kuint64max in /webkit/browser/fileapi/.
> 
>
https://code.google.com/p/chromium/codesearch#search/&q=kint64max%20%20packag...
> 2 results for kuint64max in /webkit/browser/fileapi/.
> 
> Since we agreed on using a suggested by you constant for -1, I'm not going to
> discuss further on this issue.
> If you have any other concerns about this CL, let me know. Otherwise, I'd like
> to proceed.
> 
> > > 
> > > As for consistency, -1 is already used in
> > > webkit/browser/blob/blob_url_request_job.cc for unknown length, as well as
> in
> > > webkit. It's a common paradigm.
> > > 
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > >
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> > > 
> > > > Also, using -1 is less intuitive than numeric_limits<uint64>::max(). (at
> > least
> > > > -1 should be a constant.)
> > > 
> > > We use -1 for unknown values all over Chromium code base. I added a
constant
> > as
> > > you requested.

I discussed the issue with others, and I changed my mind. Let's use a max value
instead of -1. However, the meaning of the passed argument will change.
Currently the "length" argument is defined as "number of bytes to be downloaded
in total". If we use a max value, the semantics will change to "maximum number
of bytes which will be requested". So less bytes can be requested than the
passed value. Then the max value is logical.

[mtomasz, 2014-09-04 05:41:00]

@hashimoto: Replaced length -> limit and -1 to max. PTAL.

[hashimoto, 2014-09-04 11:21:25]

lgtm

Some parts are still using the word |length|.
Please check and fix all files to use the same word.

https://codereview.chromium.org/470323003/diff/140001/content/browser/fileapi...
File content/browser/fileapi/upload_file_system_file_element_reader.cc (right):

https://codereview.chromium.org/470323003/diff/140001/content/browser/fileapi...
content/browser/fileapi/upload_file_system_file_element_reader.cc:47:
range_length_,
Please add a check here.

https://codereview.chromium.org/470323003/diff/140001/webkit/browser/fileapi/...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/470323003/diff/140001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_backend.h:40: const int64 kMaximumLength =
std::numeric_limits<int64>::max();
Why is this constant needed?

https://codereview.chromium.org/470323003/diff/140001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_backend.h:120: int64 limit,
It's not unclear from the word |limit| if this value is measured from the
beginning of the file, or from the offset.
How about using something like "max_length", "max_bytes_to_read", or anything
which clearly says this value is the number of bytes measured from |offset|?

[mtomasz, 2014-09-05 01:04:40]

https://codereview.chromium.org/470323003/diff/140001/content/browser/fileapi...
File content/browser/fileapi/upload_file_system_file_element_reader.cc (right):

https://codereview.chromium.org/470323003/diff/140001/content/browser/fileapi...
content/browser/fileapi/upload_file_system_file_element_reader.cc:47:
range_length_,
On 2014/09/04 11:21:25, hashimoto wrote:
> Please add a check here.

Done.

https://codereview.chromium.org/470323003/diff/140001/webkit/browser/fileapi/...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/470323003/diff/140001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_backend.h:40: const int64 kMaximumLength =
std::numeric_limits<int64>::max();
On 2014/09/04 11:21:25, hashimoto wrote:
> Why is this constant needed?

Having a call:

new FileStreamReader(0, kMaximumLength) is saying more than
new FileStreamReader(0, std::numeric_limits<int64>::max()).

It's also shorter, since we don't need to include <limits> everywhere we want to
use a maximum length for a file stream reader.

Finally, if we change the type from int64 to uint64, then we can just do it in
one place.

https://codereview.chromium.org/470323003/diff/140001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_backend.h:120: int64 limit,
On 2014/09/04 11:21:25, hashimoto wrote:
> It's not unclear from the word |limit| if this value is measured from the
> beginning of the file, or from the offset.
> How about using something like "max_length", "max_bytes_to_read", or anything
> which clearly says this value is the number of bytes measured from |offset|?

This naming OFFSET and LIMIT is used widely in databases, and it means a limit
starting from the offset. max_length is not much better here since it may mean a
length of the file. max_bytes_to_read makes sense. Let's go with it.

[hashimoto, 2014-09-05 03:17:28]

Please fix the failing UploadFileSystemFileElementReaderTest.

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/blob/blo...
File webkit/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/blob/blo...
webkit/browser/blob/blob_url_request_job.cc:10: #include <vector>
nit: Are these include needed?

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/blob/blo...
webkit/browser/blob/blob_url_request_job.cc:584: item.length() -
additional_offset,
Shouldn't item_length_list_ be used here?

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/fileapi/...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_backend.h:38: // Maximum value of length
arguments used for
There is no longer |length| argument.

[mtomasz, 2014-09-05 05:31:42]

Done. I added two fixes, which basically convert max_uint64 to max_int64. This
is not nice, but should be removed with crbug.com/406967.

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/blob/blo...
File webkit/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/blob/blo...
webkit/browser/blob/blob_url_request_job.cc:10: #include <vector>
On 2014/09/05 03:17:27, hashimoto wrote:
> nit: Are these include needed?

algorithm for std::min.
string for std::string.
vector for std::vector.

All used in this file.

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/blob/blo...
webkit/browser/blob/blob_url_request_job.cc:584: item.length() -
additional_offset,
On 2014/09/05 03:17:27, hashimoto wrote:
> Shouldn't item_length_list_ be used here?

While filling out item_length_list_, file stream readers are lazily created, so
basically item_length_list_[index] may be undefined at this stage.

However, item.length() may be -1 (aka max int64) here, so I added an extra
check. BTW. In this file static_cast<uint64>(-1) is used, which I find weird and
tricky, so replaced with numeric_limits.

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/fileapi/...
File webkit/browser/fileapi/file_system_backend.h (right):

https://codereview.chromium.org/470323003/diff/160001/webkit/browser/fileapi/...
webkit/browser/fileapi/file_system_backend.h:38: // Maximum value of length
arguments used for
On 2014/09/05 03:17:27, hashimoto wrote:
> There is no longer |length| argument.

Done.

[hashimoto, 2014-09-09 04:51:31]

Sorry for being late to respond.
lgtm

[mtomasz, 2014-09-09 04:57:16]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-09 05:00:01]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/470323003/180001

[commit-bot: I haz the power, 2014-09-09 05:07:47]

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

[commit-bot: I haz the power, 2014-09-09 05:07:48]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/53226)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  win_chromium_x64_rel_swarming on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[mtomasz, 2014-09-09 06:51:30]

mtomasz@chromium.org changed reviewers:
+ jochen@chromium.org
- darin@chromium.org

[mtomasz, 2014-09-09 06:53:48]

@jochen: PTAL at:
  content/public/test/test_file_system_backend.*
  webkit/browser/blob/blob_url_request_job.cc

Thanks.

[jochen (gone - plz use gerrit), 2014-09-10 08:11:27]

my bits lgtm

[mtomasz, 2014-09-10 08:12:06]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-10 08:12:33]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/470323003/200001

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

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

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

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/53931)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  ios_rel_device_ninja on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[mtomasz, 2014-09-10 09:47:34]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-10 09:47:55]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mtomasz@chromium.org/470323003/220001

[commit-bot: I haz the power, 2014-09-10 09:57:36]

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

[commit-bot: I haz the power, 2014-09-10 09:57:39]

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

[mtomasz, 2014-09-11 03:24:01]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-11 03:25:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/470323003/220001

[commit-bot: I haz the power, 2014-09-11 03:47:55]

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

[commit-bot: I haz the power, 2014-09-11 03:47:56]

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

[mtomasz, 2014-09-11 05:11:25]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-11 05:12:01]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/470323003/240001

[commit-bot: I haz the power, 2014-09-11 05:22:48]

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

[commit-bot: I haz the power, 2014-09-11 05:22:50]

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

[mtomasz, 2014-09-11 07:32:15]

mtomasz@chromium.org changed reviewers:
+ jianli@chromium.org

[mtomasz, 2014-09-11 07:33:35]

On 2014/09/11 05:22:50, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   chromium_presubmit on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

@jianli: PTAL at webkit/browser/blob/blob_url_request_job.cc. Thanks.

[jianli, 2014-09-11 17:45:51]

https://codereview.chromium.org/470323003/diff/240001/storage/browser/blob/bl...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/240001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:586: : item.length() -
additional_offset,
Should this be item.length() - item.offset() - additional_offset?

[mtomasz, 2014-09-16 08:58:22]

https://codereview.chromium.org/470323003/diff/240001/storage/browser/blob/bl...
File storage/browser/blob/blob_url_request_job.cc (right):

https://codereview.chromium.org/470323003/diff/240001/storage/browser/blob/bl...
storage/browser/blob/blob_url_request_job.cc:586: : item.length() -
additional_offset,
On 2014/09/11 17:45:51, jianli wrote:
> Should this be item.length() - item.offset() - additional_offset?

I don't think so. item.offset() is the start position in bytes, and
item.length() length of the item from the offset.

[jianli, 2014-09-16 17:46:09]

blob_url_request_job.cc lgtm

[mtomasz, 2014-09-16 23:04:37]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-09-16 23:07:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/470323003/240001

[commit-bot: I haz the power, 2014-09-16 23:40:46]

Committed patchset #13 (id:240001) as b40d820bae76b5be054718551e23cae81752f5e2

[commit-bot: I haz the power, 2014-09-16 23:41:25]

Patchset 13 (id:??) landed as
https://crrev.com/85aa9d79cb455b47fc383ce5a64bb7da3800ba11
Cr-Commit-Position: refs/heads/master@{#295177}