arc: Add utility functions to implement ARC content file system

arc: Add utility functions to implement ARC content file system

Add intent_helper_util to use IntentHelperInstance from IO thread.
Add FileSystemUrlToArcUrl and a test for it.

BUG=654684
TEST=unit_tests

Committed: https://crrev.com/e1c72209b2babf55ccd41247330e2ec18caa85df
Cr-Commit-Position: refs/heads/master@{#431425}

[hashimoto, 2016-11-02 08:14:27]

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:14:49]

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:48:16]

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

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

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 <- THIS
3. https://codereview.chromium.org/2469243002 Implement
ArcContentFileSystemAsyncFileUtil::GetFileInfo
4. https://codereview.chromium.org/2470843004 Implement
ArcContentFileSystemFileStreamReader::Read/GetLength
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 09:33:38]

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

[commit-bot: I haz the power, 2016-11-02 09:33:39]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-11-04 17:19:50]

Minor comments only.

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

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.cc:42: GURL
FileSystemUrlToArcUrl(const storage::FileSystemURL& url) {
Could you share the "path_after_root" and later code, so that you do not need to
play with path conversion a lot?

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

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:12: 
Unnecessary empty line?

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:15: // Utility to call
IntentHelperInstance::GetFileSize on the IO thread.
The comment looks ambiguous.
1) This function is called on IO thread.
2) GetFileSize is called on IO thread.
IIUC, former, right? Could you disambiguate? Ditto for below.

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:26: 
Ditto.

[hashimoto, 2016-11-07 05:34:16]

PTAL

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

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.cc:42: GURL
FileSystemUrlToArcUrl(const storage::FileSystemURL& url) {
On 2016/11/04 17:19:49, hidehiko wrote:
> Could you share the "path_after_root" and later code, so that you do not need
to
> play with path conversion a lot?

You mean sharing the code with ExternalFileUrlToArcUrl()?

What ExternalFileUrlToArcUrl() does is reverse of ArcUrlToExternalFileUrl().
Virtual path is "arc-content/<escaped ARC URL>".

OTOH FileSystemUrlToArcUrl() deals with FileSystemURL's path which is
"/special/arc-content/<escaped ARC URL>".
(File system API is hard!)

Added a comment to clarify what's going on here.

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

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:12: 
On 2016/11/04 17:19:49, hidehiko wrote:
> Unnecessary empty line?

Done.

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:15: // Utility to call
IntentHelperInstance::GetFileSize on the IO thread.
On 2016/11/04 17:19:49, hidehiko wrote:
> The comment looks ambiguous.
> 1) This function is called on IO thread.
> 2) GetFileSize is called on IO thread.
> IIUC, former, right? Could you disambiguate? Ditto for below.

Done.

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:26: 
On 2016/11/04 17:19:49, hidehiko wrote:
> Ditto.

Done.

[hashimoto, 2016-11-07 05:34:24]

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

[commit-bot: I haz the power, 2016-11-07 05:34:36]

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-07 06:32:55]

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

[commit-bot: I haz the power, 2016-11-07 06:32:56]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2016-11-07 18:07:54]

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

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.cc:42: GURL
FileSystemUrlToArcUrl(const storage::FileSystemURL& url) {
On 2016/11/07 05:34:15, hashimoto wrote:
> On 2016/11/04 17:19:49, hidehiko wrote:
> > Could you share the "path_after_root" and later code, so that you do not
need
> to
> > play with path conversion a lot?
> 
> You mean sharing the code with ExternalFileUrlToArcUrl()?
> 
> What ExternalFileUrlToArcUrl() does is reverse of ArcUrlToExternalFileUrl().
> Virtual path is "arc-content/<escaped ARC URL>".
> 
> OTOH FileSystemUrlToArcUrl() deals with FileSystemURL's path which is
> "/special/arc-content/<escaped ARC URL>".
> (File system API is hard!)
> 
> Added a comment to clarify what's going on here.

My question was;

Why FileSystemUrlToArcUrl passes through virtual path?

With, e.g., ArcEscapedPathToArcUrl(),

In FileSystemUrlToArcUrl():
/special/arc-content/<ARC escaped path> -> <ARC escaped path> -> ARC URL.
IN ExternalFileUrlToArcUrl():
/arc-content/<ARC escaped path> -> <ARC escaped path> -> ARC URL.

where <ARC escaped path> = |path_after_root| in the current implementation.
So, latter code can be shared in those cases, I think?

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

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:15: // Utility to call
IntentHelperInstance::GetFileSize on the IO thread.
On 2016/11/07 05:34:16, hashimoto wrote:
> On 2016/11/04 17:19:49, hidehiko wrote:
> > The comment looks ambiguous.
> > 1) This function is called on IO thread.
> > 2) GetFileSize is called on IO thread.
> > IIUC, former, right? Could you disambiguate? Ditto for below.
> 
> Done.

Much clearer to me!

[hashimoto, 2016-11-08 01:34:16]

PTAL

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

https://codereview.chromium.org/2464333003/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_content_file_system_url_util.cc:42: GURL
FileSystemUrlToArcUrl(const storage::FileSystemURL& url) {
On 2016/11/07 18:07:54, hidehiko wrote:
> On 2016/11/07 05:34:15, hashimoto wrote:
> > On 2016/11/04 17:19:49, hidehiko wrote:
> > > Could you share the "path_after_root" and later code, so that you do not
> need
> > to
> > > play with path conversion a lot?
> > 
> > You mean sharing the code with ExternalFileUrlToArcUrl()?
> > 
> > What ExternalFileUrlToArcUrl() does is reverse of ArcUrlToExternalFileUrl().
> > Virtual path is "arc-content/<escaped ARC URL>".
> > 
> > OTOH FileSystemUrlToArcUrl() deals with FileSystemURL's path which is
> > "/special/arc-content/<escaped ARC URL>".
> > (File system API is hard!)
> > 
> > Added a comment to clarify what's going on here.
> 
> My question was;
> 
> Why FileSystemUrlToArcUrl passes through virtual path?
> 
> With, e.g., ArcEscapedPathToArcUrl(),
> 
> In FileSystemUrlToArcUrl():
> /special/arc-content/<ARC escaped path> -> <ARC escaped path> -> ARC URL.
> IN ExternalFileUrlToArcUrl():
> /arc-content/<ARC escaped path> -> <ARC escaped path> -> ARC URL.
> 
> where <ARC escaped path> = |path_after_root| in the current implementation.
> So, latter code can be shared in those cases, I think?

That makes sense.
Done.

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

lgtm

https://codereview.chromium.org/2464333003/diff/40001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/intent_helper_util.h (right):

https://codereview.chromium.org/2464333003/diff/40001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/intent_helper_util.h:14: // Utility which
posts a task to run IntentHelperInstance::GetFileSize.
Nice. This gets totally clear to me.

[hashimoto, 2016-11-10 22:26:30]

The CQ bit was checked by hashimoto@chromium.org

[commit-bot: I haz the power, 2016-11-10 22:27:04]

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 00:46:22]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-11-11 00:48:51]

Description was changed from

==========
arc: Add utility functions to implement ARC content file system

Add intent_helper_util to use IntentHelperInstance from IO thread.
Add FileSystemUrlToArcUrl and a test for it.

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

to

==========
arc: Add utility functions to implement ARC content file system

Add intent_helper_util to use IntentHelperInstance from IO thread.
Add FileSystemUrlToArcUrl and a test for it.

BUG=654684
TEST=unit_tests

Committed: https://crrev.com/e1c72209b2babf55ccd41247330e2ec18caa85df
Cr-Commit-Position: refs/heads/master@{#431425}
==========

[commit-bot: I haz the power, 2016-11-11 00:48:51]

Patchset 3 (id:??) landed as
https://crrev.com/e1c72209b2babf55ccd41247330e2ec18caa85df
Cr-Commit-Position: refs/heads/master@{#431425}