arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

Non-zero offset handling will be implemented later.

BUG=654684
TEST=unit_tests

Committed: https://crrev.com/913e7e3d63caa7302c32e9e3bfd61d79a70446a8
Cr-Commit-Position: refs/heads/master@{#431992}

[hashimoto, 2016-11-02 08:22:17]

Description was changed from

==========
arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

BUG=654684
TEST=unit_tests
==========

to

==========
arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

Non-zero offset handling will be implemented later.

BUG=654684
TEST=unit_tests
==========

[hashimoto, 2016-11-02 08:47:13]

The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-02 08:47:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[hashimoto, 2016-11-02 08:49:07]

hashimoto@chromium.org changed reviewers:
+ hidehiko@chromium.org

[hashimoto, 2016-11-02 08:49:07]

Sorry for spamming you with CLs.
Please review these CLs at your convenience.

This is part of a series of CLs:
1. https://codereview.chromium.org/2469963003 Add methods to
IntentHelperInstance to read ARC contents
2. https://codereview.chromium.org/2464333003 Add utility functions to implement
ARC content file system
3. https://codereview.chromium.org/2469243002 Implement
ArcContentFileSystemAsyncFileUtil::GetFileInfo
4. https://codereview.chromium.org/2470843004 Implement
ArcContentFileSystemFileStreamReader::Read/GetLength <- THIS
5. https://codereview.chromium.org/2464333004 Convert content: and file: URLs
from ARC to externalfile: URLs

[commit-bot: I haz the power, 2016-11-02 10:21:21]

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

[commit-bot: I haz the power, 2016-11-02 10:21:22]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-11-10 07:07:06]

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_backend_delegate.cc
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_backend_delegate.cc:40:
return base::MakeUnique<ArcContentFileSystemFileStreamReader>(arc_url,
max_bytes_to_read gets ignored now.

According to:
https://cs.chromium.org/chromium/src/storage/browser/fileapi/file_system_back...

it needs to be supported?

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.h
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.h:23:
class ArcContentFileSystemFileStreamReader : public storage::FileStreamReader {
Now it has concrete implementation. It's time to add (even shorter) document for
what this is, and what this is for.

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:54:
mojo::ScopedHandle scoped_handle{mojo::Handle(wrapped_handle)};
Can braces be replaced by parens?

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:111:
EXPECT_EQ(std::string(kData, kData + arraysize(kData)),
Optional: How about comparing by StringPiece?

[hashimoto, 2016-11-11 22:30:17]

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_backend_delegate.cc
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_backend_delegate.cc:40:
return base::MakeUnique<ArcContentFileSystemFileStreamReader>(arc_url,
On 2016/11/10 07:07:06, hidehiko wrote:
> max_bytes_to_read gets ignored now.
> 
> According to:
>
https://cs.chromium.org/chromium/src/storage/browser/fileapi/file_system_back...
> 
> it needs to be supported?

IIUC implementations MAY use max_bytes_to_read to avoid downloading unneeded
contents.
In many parts, existing code is ignoring this value.
https://cs.chromium.org/chromium/src/storage/browser/fileapi/isolated_file_sy...

Because AFAIK there is no way to benefit from this argument with Android's
ContentResolver, I'm just ignoring it here.

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.h
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.h:23:
class ArcContentFileSystemFileStreamReader : public storage::FileStreamReader {
On 2016/11/10 07:07:06, hidehiko wrote:
> Now it has concrete implementation. It's time to add (even shorter) document
for
> what this is, and what this is for.

Done.

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:54:
mojo::ScopedHandle scoped_handle{mojo::Handle(wrapped_handle)};
On 2016/11/10 07:07:06, hidehiko wrote:
> Can braces be replaced by parens?
Parens would make this into a form of "A a(B(c));", and we'd be bitten by most
vexing parse.

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:111:
EXPECT_EQ(std::string(kData, kData + arraysize(kData)),
On 2016/11/10 07:07:06, hidehiko wrote:
> Optional: How about comparing by StringPiece?

Done.

[hashimoto, 2016-11-11 22:33:21]

The CQ bit was checked by hashimoto@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-11 22:34:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-11 23:29:48]

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

[commit-bot: I haz the power, 2016-11-11 23:29:49]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-11-14 17:46:34]

lgtm with one minor comment.

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_backend_delegate.cc
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_backend_delegate.cc:40:
return base::MakeUnique<ArcContentFileSystemFileStreamReader>(arc_url,
On 2016/11/11 22:30:17, hashimoto wrote:
> On 2016/11/10 07:07:06, hidehiko wrote:
> > max_bytes_to_read gets ignored now.
> > 
> > According to:
> >
>
https://cs.chromium.org/chromium/src/storage/browser/fileapi/file_system_back...
> > 
> > it needs to be supported?
> 
> IIUC implementations MAY use max_bytes_to_read to avoid downloading unneeded
> contents.
> In many parts, existing code is ignoring this value.
>
https://cs.chromium.org/chromium/src/storage/browser/fileapi/isolated_file_sy...
> 
> Because AFAIK there is no way to benefit from this argument with Android's
> ContentResolver, I'm just ignoring it here.

Good to know. Thanks.

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc
(right):

https://codereview.chromium.org/2470843004/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader_unittest.cc:54:
mojo::ScopedHandle scoped_handle{mojo::Handle(wrapped_handle)};
On 2016/11/11 22:30:17, hashimoto wrote:
> On 2016/11/10 07:07:06, hidehiko wrote:
> > Can braces be replaced by parens?
> Parens would make this into a form of "A a(B(c));", and we'd be bitten by most
> vexing parse.

Oops... I understood. Thank you for explanation.

https://codereview.chromium.org/2470843004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc
(right):

https://codereview.chromium.org/2470843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:38:
if (file_stream_) {
nit: omit brace in this case?

[hashimoto, 2016-11-14 21:58:27]

The CQ bit was checked by hashimoto@chromium.org

[hashimoto, 2016-11-14 21:58:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
hidehiko@chromium.org
Link to the patchset: https://codereview.chromium.org/2470843004/#ps60001
(title: "Remove braces")

[hashimoto, 2016-11-14 21:58:33]

https://codereview.chromium.org/2470843004/diff/40001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc
(right):

https://codereview.chromium.org/2470843004/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_file_stream_reader.cc:38:
if (file_stream_) {
On 2016/11/14 17:46:34, hidehiko wrote:
> nit: omit brace in this case?

Done.

[commit-bot: I haz the power, 2016-11-14 22:00:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-14 23:56:06]

Description was changed from

==========
arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

Non-zero offset handling will be implemented later.

BUG=654684
TEST=unit_tests
==========

to

==========
arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

Non-zero offset handling will be implemented later.

BUG=654684
TEST=unit_tests
==========

[commit-bot: I haz the power, 2016-11-14 23:56:07]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-11-15 00:02:15]

Description was changed from

==========
arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

Non-zero offset handling will be implemented later.

BUG=654684
TEST=unit_tests
==========

to

==========
arc: Implement ArcContentFileSystemFileStreamReader::Read/GetLength

Non-zero offset handling will be implemented later.

BUG=654684
TEST=unit_tests

Committed: https://crrev.com/913e7e3d63caa7302c32e9e3bfd61d79a70446a8
Cr-Commit-Position: refs/heads/master@{#431992}
==========

[commit-bot: I haz the power, 2016-11-15 00:02:16]

Patchset 4 (id:??) landed as
https://crrev.com/913e7e3d63caa7302c32e9e3bfd61d79a70446a8
Cr-Commit-Position: refs/heads/master@{#431992}