[fsp] Add FileStreamReader for the reading operation.

[fsp] Add FileStreamReader for the reading operation.

This patch adds a FileStreamReader implementation for provided file systeme.
Note, that this is an initial version, which doesn't validate modification time.
That will be added in a separate patch.

TEST=browser_tests: *FileSystemProvider*ReadFile*,
     unit_tests: *FileSystemProvider*FileStreamReader*
BUG=248427
R=benwells@chromium.org, hirono@chromium.org, kinaba@chromium.org

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

[mtomasz, 2014-05-16 03:31:35]

@kinaba: PTAL at C++.
@hirono: PTAL at JS.

This CL depends on: 287673004.

Thanks.

[hirono, 2014-05-16 04:06:47]

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:88:
onSuccess([TESTING_BROKEN_TIRAMISU_FILE], false /* has_next */);
The comment is wrong?

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:197:
var textToSendInChunks = textToSend.split('(?= )');
/(?= )/
This one?

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:254:
var onTestSuccess = chrome.test.callbackPass(function() {});
I just noticed we don't need "function() {}".
chrome.test.callbackPass() returns a function that we want.

[mtomasz, 2014-05-16 06:19:08]

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:88:
onSuccess([TESTING_BROKEN_TIRAMISU_FILE], false /* has_next */);
On 2014/05/16 04:06:47, hirono wrote:
> The comment is wrong?

Which comment? has_next in the comment is a name of the argument.

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:197:
var textToSendInChunks = textToSend.split('(?= )');
On 2014/05/16 04:06:47, hirono wrote:
> /(?= )/
> This one?

Very good catch. It revealed a bug in the code. Done. Fixed.

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:254:
var onTestSuccess = chrome.test.callbackPass(function() {});
On 2014/05/16 04:06:47, hirono wrote:
> I just noticed we don't need "function() {}".
> chrome.test.callbackPass() returns a function that we want.

Done.

[hirono, 2014-05-16 06:23:41]

lgtm! Thank you!

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/288113004/diff/10002/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:88:
onSuccess([TESTING_BROKEN_TIRAMISU_FILE], false /* has_next */);
On 2014/05/16 06:19:08, mtomasz wrote:
> On 2014/05/16 04:06:47, hirono wrote:
> > The comment is wrong?
> 
> Which comment? has_next in the comment is a name of the argument.

Ah, I misunderstood as it is the meaning of value.

[hirono, 2014-05-16 06:24:48]

https://codereview.chromium.org/288113004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/288113004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:283: 
nit: The line is added unintentionally?

[mtomasz, 2014-05-16 06:31:11]

https://codereview.chromium.org/288113004/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/288113004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:283: 
On 2014/05/16 06:24:48, hirono wrote:
> nit: The line is added unintentionally?

Done.

[hirono, 2014-05-16 06:36:01]

On 2014/05/16 06:31:11, mtomasz wrote:
>
https://codereview.chromium.org/288113004/diff/40001/chrome/browser/chromeos/...
> File
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
> (right):
> 
>
https://codereview.chromium.org/288113004/diff/40001/chrome/browser/chromeos/...
>
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:283: 
> On 2014/05/16 06:24:48, hirono wrote:
> > nit: The line is added unintentionally?
> 
> Done.

still LGTM.

[kinaba, 2014-05-16 07:58:00]

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:45:
base::File::FILE_ERROR_SECURITY));
I think you can directly do callback.Run() because OnOpenFileCompleteOnUIThread
does it after all.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:95:
callback, 0, false /* has_next */, base::File::FILE_ERROR_ABORT));
ditto: no need for posting.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:115:
// TODO(mtomasz): Comment.
Comment.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:126:
return;
ditto: no need for posting.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:199:
if (!file_system_.get()) {
Dereferencing a weak pointer in different threads (IO here) than its born place
(UI) is not thread safe (see the comments in weak_ptr.h). |file_handle_| should
be used here. Same for other checks on the IO thread.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:215:
// Lazily initialize with the first call to Read().
nit: Read() => GetLength()

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:293:
callback.Run(net::FileErrorToNetError(result));
return;

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h
(right):

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h:86:
fileapi::FileSystemContext* context_;
Will this be used in future patches?

[mtomasz, 2014-05-16 09:18:19]

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:45:
base::File::FILE_ERROR_SECURITY));
On 2014/05/16 07:58:00, kinaba wrote:
> I think you can directly do callback.Run() because
OnOpenFileCompleteOnUIThread
> does it after all.

Done. I also added some thread checks.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:95:
callback, 0, false /* has_next */, base::File::FILE_ERROR_ABORT));
On 2014/05/16 07:58:00, kinaba wrote:
> ditto: no need for posting.

Done.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:115:
// TODO(mtomasz): Comment.
On 2014/05/16 07:58:00, kinaba wrote:
> Comment.

Done.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:126:
return;
On 2014/05/16 07:58:00, kinaba wrote:
> ditto: no need for posting.

Done.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:199:
if (!file_system_.get()) {
On 2014/05/16 07:58:00, kinaba wrote:
> Dereferencing a weak pointer in different threads (IO here) than its born
place
> (UI) is not thread safe (see the comments in weak_ptr.h). |file_handle_|
should
> be used here. Same for other checks on the IO thread.

Oops. It is also incorrect here, since we don't want to reinitialize once the FS
is gone. Done.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:215:
// Lazily initialize with the first call to Read().
On 2014/05/16 07:58:00, kinaba wrote:
> nit: Read() => GetLength()

Done.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:293:
callback.Run(net::FileErrorToNetError(result));
On 2014/05/16 07:58:00, kinaba wrote:
> return;

Done.

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h
(right):

https://codereview.chromium.org/288113004/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h:86:
fileapi::FileSystemContext* context_;
On 2014/05/16 07:58:00, kinaba wrote:
> Will this be used in future patches?

Nope. Removed.

[kinaba, 2014-05-19 02:22:00]

lgtm

[mtomasz, 2014-05-19 03:19:05]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-05-19 03:19:10]

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

[commit-bot: I haz the power, 2014-05-19 06:19:54]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-19 07:20:51]

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

[commit-bot: I haz the power, 2014-05-19 07:20:51]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[mtomasz, 2014-05-19 08:39:43]

On 2014/05/19 07:20:51, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_chromeos_rel on tryserver.chromium
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

I've fixed tests + reading + changed string to ArrayBuffer, since string wasn't
binary safe.

[mtomasz, 2014-05-19 08:41:07]

On 2014/05/19 08:39:43, mtomasz wrote:
> On 2014/05/19 07:20:51, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
> 
> I've fixed tests + reading + changed string to ArrayBuffer, since string
wasn't
> binary safe.

@hirono, @kinaba: PTAL one more time. Thanks.

[kinaba, 2014-05-19 08:42:38]

On 2014/05/19 08:39:43, mtomasz wrote:
> On 2014/05/19 07:20:51, I haz the power (commit-bot) wrote:
> > Try jobs failed on following builders:
> >   linux_chromium_chromeos_rel on tryserver.chromium
> >
>
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
> 
> I've fixed tests + reading + changed string to ArrayBuffer, since string
wasn't
> binary safe.

*.cc lgtm still

[mtomasz, 2014-05-19 14:59:57]

@benwells: PTAL. I changed DOMString to ArrayBuffer in the IDL. DOMString wasn't
binary safe.

[benwells, 2014-05-19 23:43:06]

On 2014/05/19 14:59:57, mtomasz wrote:
> @benwells: PTAL. I changed DOMString to ArrayBuffer in the IDL. DOMString
wasn't
> binary safe.

Can you explain how it isn't binary safe? I'm interested.

[mtomasz, 2014-05-20 00:24:37]

On 2014/05/19 23:43:06, benwells wrote:
> On 2014/05/19 14:59:57, mtomasz wrote:
> > @benwells: PTAL. I changed DOMString to ArrayBuffer in the IDL. DOMString
> wasn't
> > binary safe.
> 
> Can you explain how it isn't binary safe? I'm interested.

@benwells: I didn't investigate it in details, but length of the string on the
JS side != length of the string on the C++ side. I think that character encoding
messed up sth. here.

[benwells, 2014-05-20 03:06:24]

lgtm.

[hirono, 2014-05-20 04:03:06]

lgtm with a nit.

https://codereview.chromium.org/288113004/diff/200001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/288113004/diff/200001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:158:
* @param {function(string, boolean)} onSuccess Success callback with a chunk
function(string, boolean) -> function(ArrayBuffer, boolean) ?

[mtomasz, 2014-05-20 04:39:35]

https://codereview.chromium.org/288113004/diff/200001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js
(right):

https://codereview.chromium.org/288113004/diff/200001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/file_system_provider/read_file/test.js:158:
* @param {function(string, boolean)} onSuccess Success callback with a chunk
On 2014/05/20 04:03:07, hirono wrote:
> function(string, boolean) -> function(ArrayBuffer, boolean) ?

Done.

[mtomasz, 2014-05-20 04:39:49]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-05-20 04:40:10]

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

[commit-bot: I haz the power, 2014-05-20 09:09:06]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[commit-bot: I haz the power, 2014-05-20 10:35:44]

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

[commit-bot: I haz the power, 2014-05-20 10:35:45]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[mtomasz, 2014-05-21 01:18:36]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-05-21 01:19:16]

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

[mtomasz, 2014-05-21 06:43:37]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-05-21 06:45:50]

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

[mtomasz, 2014-05-21 14:40:55]

The CQ bit was unchecked by mtomasz@chromium.org

[mtomasz, 2014-05-21 14:41:05]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2014-05-21 19:57:45]

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

[mtomasz, 2014-05-22 01:26:41]

Committed patchset #14 manually as r272041 (presubmit successful).