mediaview: Skeleton for ARC Documents Provider FS.

mediaview: Skeleton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot

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

[Shuhei Takahashi, 2016-12-12 08:55:47]

Description was changed from

==========
mediaview: Skelton for ARC Documents Provider FS.

BUG=chromium:671511
TEST=trybot
==========

to

==========
mediaview: Skelton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider.
It almost contains boilderplates only, and implementations are coming
soon.

BUG=chromium:671511
TEST=trybot
==========

[Shuhei Takahashi, 2016-12-12 08:55:53]

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

[commit-bot: I haz the power, 2016-12-12 08:56:08]

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-12-12 10:35:42]

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

[commit-bot: I haz the power, 2016-12-12 10:35:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Shuhei Takahashi, 2016-12-12 16:04:38]

Description was changed from

==========
mediaview: Skelton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider.
It almost contains boilderplates only, and implementations are coming
soon.

BUG=chromium:671511
TEST=trybot
==========

to

==========
mediaview: Skelton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot
==========

[Shuhei Takahashi, 2016-12-12 16:05:12]

nya@chromium.org changed reviewers:
+ hashimoto@chromium.org, jam@chromium.org, mtomasz@chromium.org,
tzik@chromium.org, yusukes@chromium.org

[Shuhei Takahashi, 2016-12-12 16:05:12]

hashimoto: PTAL all
yusukes: PTAL chrome/browser/chromeos/arc/...
mtomasz: PTAL chrome/browser/chromeos/fileapi/...
tzik: PTAL storage/...
jam: PTAL chrome/browser/platform_util_unittest.cc

[jam, 2016-12-12 17:58:37]

lgtm

[Shuhei Takahashi, 2016-12-13 01:18:19]

nya@chromium.org changed reviewers:
+ lhchavez@google.com

[Shuhei Takahashi, 2016-12-13 01:18:20]

lhchavez: PTAL for chrome/browser/chromeos/arc
yusukes: FYI

[tzik, 2016-12-13 04:53:03]

lgtm

[mtomasz, 2016-12-13 07:50:04]

chrome/browser/chromeos/fileapi/ lgtm

[hashimoto, 2016-12-13 07:52:29]

chrome/browser/chromeos/arc/fileapi/ lgtm

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

https://codereview.chromium.org/2568033002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h:93:
base::WeakPtrFactory<ArcDocumentsProviderAsyncFileUtil> weak_ptr_factory_;
How about adding this when it's actually needed?

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

https://codereview.chromium.org/2568033002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:51:
ArcDocumentsProviderAsyncFileUtil async_file_util_;
How about adding this when it's actually needed?

[Shuhei Takahashi, 2016-12-13 10:27:38]

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

[Shuhei Takahashi, 2016-12-13 10:27:47]

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

https://codereview.chromium.org/2568033002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.h:93:
base::WeakPtrFactory<ArcDocumentsProviderAsyncFileUtil> weak_ptr_factory_;
On 2016/12/13 07:52:29, hashimoto wrote:
> How about adding this when it's actually needed?

Done.

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

https://codereview.chromium.org/2568033002/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:51:
ArcDocumentsProviderAsyncFileUtil async_file_util_;
On 2016/12/13 07:52:29, hashimoto wrote:
> How about adding this when it's actually needed?

Done. I assume you meant a line below :)

[commit-bot: I haz the power, 2016-12-13 10:27:50]

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-12-13 11:35:25]

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

[commit-bot: I haz the power, 2016-12-13 11:35:26]

Dry run: This issue passed the CQ dry run.

[Yusuke Sato, 2016-12-13 18:58:56]

lgtm w/ style comments

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc
(right):

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:7:
#include "base/bind.h"
nit: unused?

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:12:
#include "base/memory/ptr_util.h"
nit: unused?

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:15:
#include "storage/browser/fileapi/file_system_context.h"
nit: same (line 15-18)

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc
(right):

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:7:
#include <memory>
nit: remove as the header already has it.

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
"However, any includes present in the related header do not need to be included
again in the related cc (i.e., foo.cc can rely on foo.h's includes)."

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:11:
#include "base/memory/ptr_util.h"
unused?

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:13:
#include "storage/browser/fileapi/file_stream_reader.h"
unused? (L13-14)

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h
(right):

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:14:
namespace storage {
nit: space between L14/15 if you keep this?

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:15:
class AsyncFileUtil;
nit: why are these forward declarations necessary? Since
chromeos::FileSystemBackendDelegate (the parent class) is visible, and the class
only overrides some of the parent's methods, I think including
chrome/browser/chromeos/fileapi/file_system_backend_delegate.h does the trick,
no?

Actually you're not having base::Time here (for L38) but the code compiles.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:20:
}  // namespace storage
same

[Luis Héctor Chávez, 2016-12-13 19:10:50]

lhchavez@chromium.org changed reviewers:
+ lhchavez@chromium.org
- lhchavez@google.com

[Luis Héctor Chávez, 2016-12-13 19:10:51]

Adding the correct lhchavez.

lgtm with a nit, but can you also please s/Skelton/Skeleton/ (or stub) in the
commit subject + description?

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc
(right):

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:36:
NOTIMPLEMENTED();
Can you also add the TODO here and in L167 for consistency?

[Shuhei Takahashi, 2016-12-14 02:47:53]

Description was changed from

==========
mediaview: Skelton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot
==========

to

==========
mediaview: Skeleton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot
==========

[Shuhei Takahashi, 2016-12-14 03:24:55]

Thanks for reviewing! Updated the description s/skelton/skeleton/g.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc
(right):

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:7:
#include "base/bind.h"
On 2016/12/13 18:58:56, Yusuke Sato (ooo Dec 16 to 31) wrote:
> nit: unused?

Done.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:12:
#include "base/memory/ptr_util.h"
On 2016/12/13 18:58:55, Yusuke Sato (ooo Dec 16 to 31) wrote:
> nit: unused?

Done.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:15:
#include "storage/browser/fileapi/file_system_context.h"
On 2016/12/13 18:58:55, Yusuke Sato (ooo Dec 16 to 31) wrote:
> nit: same (line 15-18)

Done.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_async_file_util.cc:36:
NOTIMPLEMENTED();
On 2016/12/13 19:10:51, Luis Héctor Chávez wrote:
> Can you also add the TODO here and in L167 for consistency?

Done.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc
(right):

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:7:
#include <memory>
On 2016/12/13 18:58:56, Yusuke Sato (ooo Dec 16 to 31) wrote:
> nit: remove as the header already has it.
> 
> https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes
> "However, any includes present in the related header do not need to be
included
> again in the related cc (i.e., foo.cc can rely on foo.h's includes)."

Done.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:11:
#include "base/memory/ptr_util.h"
On 2016/12/13 18:58:56, Yusuke Sato (ooo Dec 16 to 31) wrote:
> unused?

Done.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.cc:13:
#include "storage/browser/fileapi/file_stream_reader.h"
On 2016/12/13 18:58:56, Yusuke Sato (ooo Dec 16 to 31) wrote:
> unused? (L13-14)

Actually they are used: std::unique_ptr<FileStreamReader>(nullptr_t).

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h
(right):

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:14:
namespace storage {
On 2016/12/13 18:58:56, Yusuke Sato (ooo Dec 16 to 31) wrote:
> nit: space between L14/15 if you keep this?

Done.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:15:
class AsyncFileUtil;
On 2016/12/13 18:58:56, Yusuke Sato (ooo Dec 16 to 31) wrote:
> nit: why are these forward declarations necessary? Since
> chromeos::FileSystemBackendDelegate (the parent class) is visible, and the
class
> only overrides some of the parent's methods, I think including
> chrome/browser/chromeos/fileapi/file_system_backend_delegate.h does the trick,
> no?
> 
> Actually you're not having base::Time here (for L38) but the code compiles.

Thanks, I agree we don't need them.

https://codereview.chromium.org/2568033002/diff/20001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_backend_delegate.h:20:
}  // namespace storage
On 2016/12/13 18:58:56, Yusuke Sato (ooo Dec 16 to 31) wrote:
> same

Done.

[Shuhei Takahashi, 2016-12-14 03:25:11]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2016-12-14 03:25:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
hashimoto@chromium.org, tzik@chromium.org, jam@chromium.org,
mtomasz@chromium.org, lhchavez@chromium.org, yusukes@chromium.org
Link to the patchset: https://codereview.chromium.org/2568033002/#ps40001
(title: "IWYU and comments.")

[commit-bot: I haz the power, 2016-12-14 03:25:33]

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-12-14 04:36:24]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1481685911438390,
"parent_rev": "256a99b09bc4cd8cca49421aefb569cbc96bfb0f", "commit_rev":
"0b0e6a9cc7b4d7af17d84fce975d5183fa7602ef"}

[commit-bot: I haz the power, 2016-12-14 04:37:06]

Description was changed from

==========
mediaview: Skeleton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot
==========

to

==========
mediaview: Skeleton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot

Review-Url: https://codereview.chromium.org/2568033002
==========

[commit-bot: I haz the power, 2016-12-14 04:37:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-12-14 04:39:33]

Description was changed from

==========
mediaview: Skeleton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot

Review-Url: https://codereview.chromium.org/2568033002
==========

to

==========
mediaview: Skeleton of ARC Documents Provider FS.

This patch will add a skelton of ARC Documents Provider File System
which will allow Chrome to access Android Documents Provider in ARC.
It almost contains boilerplates only; implementations are coming in
later patches.

BUG=chromium:671511
TEST=trybot

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

[commit-bot: I haz the power, 2016-12-14 04:39:34]

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