Defer ARC file system operations while ARC is booting.

Defer ARC file system operations while ARC is booting.

An additional abstraction layer, ArcFileSystemOperationRunner, is inserted
above mojom::FileSystemInstance. Its production implementation is
ArcDeferredFileSystemOperationRunner, which defers file system operations
while ARC is booting.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:

- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready because ReadDirectory
  operations are deferred.
- When an Android content URL is opened soon after the user logs into
  the system (because the user opened the tab before they logged out for
  instance), the tab keeps loading until ARC boot finishes, instead of
  failing immediately.

This patch also changes following points for better media view UX:

- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=chromium:682577
TEST=unit_tests
TEST=Media views are mounted immediately, and a spinner is shown if it is opened.

Review-Url: https://codereview.chromium.org/2637163002
Cr-Commit-Position: refs/heads/master@{#445657}
Committed: https://chromium.googlesource.com/chromium/src/+/d280ab1a052245d5bcacf63a4478fc3c0f085899

[Shuhei Takahashi, 2017-01-17 17:28:07]

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

[commit-bot: I haz the power, 2017-01-17 17:28:17]

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

[Shuhei Takahashi, 2017-01-17 17:34:44]

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

[commit-bot: I haz the power, 2017-01-17 17:34:57]

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

[Shuhei Takahashi, 2017-01-17 17:40:36]

Description was changed from

==========
WIP

BUG=
==========

to

==========
Defer ARC file system operations while ARC is booting.

While ARC is booting, ARC file system operations are deferred until ARC boots
or ARC is disabled.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:
- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready.
- If an Android content URL is opened on login (because the user opened
  the tab before they logged out for instance), the tab waits until ARC
  boots, instead of failing immediately.

This patch also does following things for media view:
- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=TBD
TEST=Media views are mounted immediately, and a spinner is shown if it is
opened.
==========

[commit-bot: I haz the power, 2017-01-17 18:56:58]

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

[commit-bot: I haz the power, 2017-01-17 18:56:58]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shuhei Takahashi, 2017-01-19 06:52:33]

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

[commit-bot: I haz the power, 2017-01-19 06:52:53]

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

[Shuhei Takahashi, 2017-01-19 07:15:12]

Description was changed from

==========
Defer ARC file system operations while ARC is booting.

While ARC is booting, ARC file system operations are deferred until ARC boots
or ARC is disabled.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:
- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready.
- If an Android content URL is opened on login (because the user opened
  the tab before they logged out for instance), the tab waits until ARC
  boots, instead of failing immediately.

This patch also does following things for media view:
- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=TBD
TEST=Media views are mounted immediately, and a spinner is shown if it is
opened.
==========

to

==========
Defer ARC file system operations while ARC is booting.

An additional abstraction layer, ArcFileSystemOperationRunner, is inserted
above mojom::FileSystemInstance. Its typical implementation is
ArcDeferredFileSystemOperationRunner, which defers file system operations
while ARC is booting.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:

- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready because ReadDirectory
  operations are deferred.
- When an Android content URL is opened soon after the user logs into
  the system (because the user opened the tab before they logged out for
  instance), the tab keeps loading until ARC boot finishes, instead of
  failing immediately.

This patch also changes following points for better media view UX:

- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=chromium:682577
TEST=unit_tests
TEST=Media views are mounted immediately, and a spinner is shown if it is
opened.
==========

[Shuhei Takahashi, 2017-01-19 07:26:50]

Description was changed from

==========
Defer ARC file system operations while ARC is booting.

An additional abstraction layer, ArcFileSystemOperationRunner, is inserted
above mojom::FileSystemInstance. Its typical implementation is
ArcDeferredFileSystemOperationRunner, which defers file system operations
while ARC is booting.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:

- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready because ReadDirectory
  operations are deferred.
- When an Android content URL is opened soon after the user logs into
  the system (because the user opened the tab before they logged out for
  instance), the tab keeps loading until ARC boot finishes, instead of
  failing immediately.

This patch also changes following points for better media view UX:

- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=chromium:682577
TEST=unit_tests
TEST=Media views are mounted immediately, and a spinner is shown if it is
opened.
==========

to

==========
Defer ARC file system operations while ARC is booting.

An additional abstraction layer, ArcFileSystemOperationRunner, is inserted
above mojom::FileSystemInstance. Its production implementation is
ArcDeferredFileSystemOperationRunner, which defers file system operations
while ARC is booting.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:

- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready because ReadDirectory
  operations are deferred.
- When an Android content URL is opened soon after the user logs into
  the system (because the user opened the tab before they logged out for
  instance), the tab keeps loading until ARC boot finishes, instead of
  failing immediately.

This patch also changes following points for better media view UX:

- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=chromium:682577
TEST=unit_tests
TEST=Media views are mounted immediately, and a spinner is shown if it is
opened.
==========

[Shuhei Takahashi, 2017-01-19 07:27:51]

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

[commit-bot: I haz the power, 2017-01-19 07:28:00]

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

[Shuhei Takahashi, 2017-01-19 07:34:24]

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

[commit-bot: I haz the power, 2017-01-19 07:34:40]

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

[Shuhei Takahashi, 2017-01-19 07:41:22]

nya@chromium.org changed reviewers:
+ hashimoto@chromium.org, hidehiko@chromium.org, mtomasz@chromium.org

[Shuhei Takahashi, 2017-01-19 07:41:23]

hidehiko:
PTAL as an ARC owner.

mtomasz:
PTAL as an owner of c/b/c/file_manager.

hashimoto:
PTAL as a c/b/c/arc/fileapi expert.

[mtomasz, 2017-01-19 07:51:50]

c/b/c/file_manager lgtm

[commit-bot: I haz the power, 2017-01-19 08:41:23]

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

[commit-bot: I haz the power, 2017-01-19 08:41:23]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-01-20 08:01:49]

Nice CL! Almost l-gtm.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:23:
// Implements deferred ARC file system operations.
Wow, nice comment!

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:29:
// This file system oepration runner provides better UX when the user attempts
s/oepration/operation/

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:63:
// ArcSessionManager::Observer overrides:
Optional: L63 - L68 can be private?
These are for implementation purpose, than an interface this class wants to
provide?

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:72:
FRIEND_TEST_ALL_PREFIXES(ArcDeferredFileSystemOperationRunnerTest,
Through my recent refactoring work, I felt maintaining FRIEND_TEST_ALL_PREFIXES
is costly, even it looked a simpler case.
So, can we avoid FRIEND_TEST_ALL_PREFIXES, e.g., as follows?

friend class ArcDeferredFileSystemOperationRunnerTest;

class ArcDeferredFileSystemOperationRunnerTest {
  ...
  void CallOnStateChanged() {
    deferred_runner_->OnStateChanged();
  }
};

TEST_F(ArcDeferredFileSystemOperationRunnerTest, ...) {
  CallOnStateChanged();
}

or,

Define OnStateChangedForTesting() function in this class?

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:79:
ArcDeferredFileSystemOperationRunner(ArcBridgeService* bridge_service,
IIUC, this is for testing purpose (so that public ctor just redirects to this)?
Could you comment?

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:30:
class FileSystemInstanceForTest : public FakeFileSystemInstance {
Could you merge this implementation to FakeFileSystemInstance, instead of
inheriting and introducing it?

In general, "fake" is introduced to maintain in one place (the implementation
dir), rather than test code.
It is beneficial for the maintainers, because they just can edit one fake class,
rather than client test code
in distributed dirs one by one.

If just merging does not work in your case, addressing it in a following CL with
leaving TODO here is fine to me.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:73:
arc_service_manager_->arc_bridge_service()->file_system()->SetInstance(
For consistency, could you do this setup in SetUp() function, instead?
AFAIK, most ARC code does it there.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:55:
kAndroidDirectoryMimeType, -1, 0};
Clarification: any reasons this change (11 -> 0)?

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h:24:
void GetFileSizeOnIOThread(const std::string& url,
Clarification: Any reasons you change GURL to std::string, while this is still
representing URL?
AFAIK, in Chrome code, practically, GURL is the type to represent URL in most
cases.
If there is no big reason, GURL is preferred, I think.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_file_system_service.h (right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_file_system_service.h:21: // For
supporting ArcServiceManager::GetService<T>().
Style: while you're here, could you move this up?

https://google.github.io/styleguide/cppguide.html#Declaration_Order

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
File components/arc/file_system/arc_file_system_operation_runner.h (right):

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
components/arc/file_system/arc_file_system_operation_runner.h:18: // An
implementation of this interface is ArcDeferredFileSystemOperationRunner
In ArcServiceManager, ArcFileSystemOperationRunner instance must be unique,
otherwise named service getter won't work.
So, practically, ArcDeferredFileSystemOperationRunner should be the unique
subclass.
Could you explicitly document it? Also, maybe mentioning you introduced this
interface
for faking for testing will be valuable, I think.

(Optional) In the sense, how about renaming ArcDeferredFileSystemOperationRunner
to ArcFileSystemOperationRunnerImpl to imply it is the only (real)
implementation?

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
components/arc/file_system/arc_file_system_operation_runner.h:47: static const
char kArcServiceName[];
Style: Could you move up (maybe on top)?

https://google.github.io/styleguide/cppguide.html#Declaration_Order

Note: "using" there looks fine to me because these are close to virtual void
foo(...) = 0 declarations conceptually.

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
File components/arc/file_system/test/fake_arc_file_system_operation_runner.h
(right):

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
components/arc/file_system/test/fake_arc_file_system_operation_runner.h:13:
class FakeArcFileSystemOperationRunner : public ArcFileSystemOperationRunner {
Similar to my comment for FakeArcFileSystemInstance,
could you unify all this class's subclasses into here?

Considering the existing code, I'm fine to addressing this in a following CL
with leaving TODO here,
if it works you better.
Also, better to comment "how to faking" then (i.e., what's the difference from
the real implementation).

[Shuhei Takahashi, 2017-01-20 09:11:31]

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

[commit-bot: I haz the power, 2017-01-20 09:11:42]

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

[Shuhei Takahashi, 2017-01-20 09:13:04]

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:23:
// Implements deferred ARC file system operations.
On 2017/01/20 08:01:49, hidehiko wrote:
> Wow, nice comment!

:)

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:29:
// This file system oepration runner provides better UX when the user attempts
On 2017/01/20 08:01:49, hidehiko wrote:
> s/oepration/operation/

Good catch.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:63:
// ArcSessionManager::Observer overrides:
On 2017/01/20 08:01:49, hidehiko wrote:
> Optional: L63 - L68 can be private?
> These are for implementation purpose, than an interface this class wants to
> provide?

It is possible to make them private, but do we want to do so? I've never seen in
Chrome codebase interface implementations are hidden as private. Also it is
actually useless because anyone can call them by casting to interface classes.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:72:
FRIEND_TEST_ALL_PREFIXES(ArcDeferredFileSystemOperationRunnerTest,
On 2017/01/20 08:01:49, hidehiko wrote:
> Through my recent refactoring work, I felt maintaining
FRIEND_TEST_ALL_PREFIXES
> is costly, even it looked a simpler case.
> So, can we avoid FRIEND_TEST_ALL_PREFIXES, e.g., as follows?
> 
> friend class ArcDeferredFileSystemOperationRunnerTest;
> 
> class ArcDeferredFileSystemOperationRunnerTest {
>   ...
>   void CallOnStateChanged() {
>     deferred_runner_->OnStateChanged();
>   }
> };
> 
> TEST_F(ArcDeferredFileSystemOperationRunnerTest, ...) {
>   CallOnStateChanged();
> }
> 
> or,
> 
> Define OnStateChangedForTesting() function in this class?

Makes sense, done.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:79:
ArcDeferredFileSystemOperationRunner(ArcBridgeService* bridge_service,
On 2017/01/20 08:01:49, hidehiko wrote:
> IIUC, this is for testing purpose (so that public ctor just redirects to
this)?
> Could you comment?

Sure, done.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:30:
class FileSystemInstanceForTest : public FakeFileSystemInstance {
On 2017/01/20 08:01:49, hidehiko wrote:
> Could you merge this implementation to FakeFileSystemInstance, instead of
> inheriting and introducing it?
> 
> In general, "fake" is introduced to maintain in one place (the implementation
> dir), rather than test code.
> It is beneficial for the maintainers, because they just can edit one fake
class,
> rather than client test code
> in distributed dirs one by one.
> 
> If just merging does not work in your case, addressing it in a following CL
with
> leaving TODO here is fine to me.

Yup, in this case the fake implementation is fairly simple so I think we can
merge it to FakeFileSystemInstance. Done.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:73:
arc_service_manager_->arc_bridge_service()->file_system()->SetInstance(
On 2017/01/20 08:01:49, hidehiko wrote:
> For consistency, could you do this setup in SetUp() function, instead?
> AFAIK, most ARC code does it there.

Done.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:55:
kAndroidDirectoryMimeType, -1, 0};
On 2017/01/20 08:01:49, hidehiko wrote:
> Clarification: any reasons this change (11 -> 0)?

Yes. As I added commented above, the response to root documents are now
hard-coded and last_modified is always UNIX epoch.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.h:24:
void GetFileSizeOnIOThread(const std::string& url,
On 2017/01/20 08:01:49, hidehiko wrote:
> Clarification: Any reasons you change GURL to std::string, while this is still
> representing URL?
> AFAIK, in Chrome code, practically, GURL is the type to represent URL in most
> cases.
> If there is no big reason, GURL is preferred, I think.

OK, let's stick with GURL.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_file_system_service.h (right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_file_system_service.h:21: // For
supporting ArcServiceManager::GetService<T>().
On 2017/01/20 08:01:49, hidehiko wrote:
> Style: while you're here, could you move this up?
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order

Done.

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
File components/arc/file_system/arc_file_system_operation_runner.h (right):

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
components/arc/file_system/arc_file_system_operation_runner.h:18: // An
implementation of this interface is ArcDeferredFileSystemOperationRunner
On 2017/01/20 08:01:49, hidehiko wrote:
> In ArcServiceManager, ArcFileSystemOperationRunner instance must be unique,
> otherwise named service getter won't work.
> So, practically, ArcDeferredFileSystemOperationRunner should be the unique
> subclass.
> Could you explicitly document it? Also, maybe mentioning you introduced this
> interface
> for faking for testing will be valuable, I think.

Sure, updated the comments.


> (Optional) In the sense, how about renaming
ArcDeferredFileSystemOperationRunner
> to ArcFileSystemOperationRunnerImpl to imply it is the only (real)
> implementation?

Hmm, I'm wondering if which naming is good. It seems to me slightly better to
name this "deferred" because it more clearly conveys what it does.

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
components/arc/file_system/arc_file_system_operation_runner.h:47: static const
char kArcServiceName[];
On 2017/01/20 08:01:49, hidehiko wrote:
> Style: Could you move up (maybe on top)?
> 
> https://google.github.io/styleguide/cppguide.html#Declaration_Order
> 
> Note: "using" there looks fine to me because these are close to virtual void
> foo(...) = 0 declarations conceptually.

Done.

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
File components/arc/file_system/test/fake_arc_file_system_operation_runner.h
(right):

https://codereview.chromium.org/2637163002/diff/80001/components/arc/file_sys...
components/arc/file_system/test/fake_arc_file_system_operation_runner.h:13:
class FakeArcFileSystemOperationRunner : public ArcFileSystemOperationRunner {
On 2017/01/20 08:01:49, hidehiko wrote:
> Similar to my comment for FakeArcFileSystemInstance,
> could you unify all this class's subclasses into here?
> 
> Considering the existing code, I'm fine to addressing this in a following CL
> with leaving TODO here,
> if it works you better.
> Also, better to comment "how to faking" then (i.e., what's the difference from
> the real implementation).

Makes sense. Added a TODO here and other unit tests using fakes.

[hidehiko, 2017-01-20 10:06:26]

lgtm. defer to hashimoto@.

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc:55:
kAndroidDirectoryMimeType, -1, 0};
On 2017/01/20 09:13:04, Shuhei Takahashi wrote:
> On 2017/01/20 08:01:49, hidehiko wrote:
> > Clarification: any reasons this change (11 -> 0)?
> 
> Yes. As I added commented above, the response to root documents are now
> hard-coded and last_modified is always UNIX epoch.

Oh, I see. Thank you for explanation.

[commit-bot: I haz the power, 2017-01-20 10:12:18]

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

[commit-bot: I haz the power, 2017-01-20 10:12:19]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2017-01-23 08:28:43]

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.cc
(right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.cc:59:
callback.Run(-1);
Could you run the callback asynchronously here, using
base::ThreadTaskRunnerHandle::Get()->PostTask()? (the same goes for other
methods)
With this code, callback is run immediately (instead of run asynchronously) only
when file_system_instance is not available, and
this may result in subtle bugs which are difficult to reproduce.

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h
(right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:12:
#include "base/gtest_prod_util.h"
nit: is this needed?

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:144:

Please add a test case where file system instance is not available.

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:129:
base::Time::UnixEpoch();
Please add a brief comment to describe why you chose base::Time::UnixEpoch(),
instead of any other values (e.g. base::Time()).

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:762: arc_volumes_mounted_
= true;
nit: arc_volumes_mounted_ = enabled at the last of this function?

[Shuhei Takahashi, 2017-01-23 09:17:37]

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

[commit-bot: I haz the power, 2017-01-23 09:17:49]

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

[Shuhei Takahashi, 2017-01-23 09:17:52]

hashimoto: PTAL

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.cc
(right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.cc:59:
callback.Run(-1);
On 2017/01/23 08:28:42, hashimoto wrote:
> Could you run the callback asynchronously here, using
> base::ThreadTaskRunnerHandle::Get()->PostTask()? (the same goes for other
> methods)
> With this code, callback is run immediately (instead of run asynchronously)
only
> when file_system_instance is not available, and
> this may result in subtle bugs which are difficult to reproduce.

Done.

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h
(right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner.h:12:
#include "base/gtest_prod_util.h"
On 2017/01/23 08:28:42, hashimoto wrote:
> nit: is this needed?

No longer needed.

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:144:

On 2017/01/23 08:28:42, hashimoto wrote:
> Please add a test case where file system instance is not available.

Sounds great.

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:129:
base::Time::UnixEpoch();
On 2017/01/23 08:28:42, hashimoto wrote:
> Please add a brief comment to describe why you chose base::Time::UnixEpoch(),
> instead of any other values (e.g. base::Time()).

Any value should work, so I have nothing to comment. Or if you have any
suggestion to use any specific value here, please let me know.

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/file_manager/volume_manager.cc (right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/file_manager/volume_manager.cc:762: arc_volumes_mounted_
= true;
On 2017/01/23 08:28:43, hashimoto wrote:
> nit: arc_volumes_mounted_ = enabled at the last of this function?

Done.

[commit-bot: I haz the power, 2017-01-23 10:11:31]

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

[commit-bot: I haz the power, 2017-01-23 10:11:32]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Shuhei Takahashi, 2017-01-23 10:36:41]

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

[commit-bot: I haz the power, 2017-01-23 10:36:50]

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

[Shuhei Takahashi, 2017-01-23 10:37:16]

Oops, base::RunLoop has DCHECK() to forbid reuse. I uploaded a new patch to fix
buildbot failure.

[commit-bot: I haz the power, 2017-01-23 12:41:08]

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

[commit-bot: I haz the power, 2017-01-23 12:41:10]

Dry run: This issue passed the CQ dry run.

[hashimoto, 2017-01-24 04:05:45]

lgtm

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:129:
base::Time::UnixEpoch();
On 2017/01/23 09:17:51, Shuhei Takahashi wrote:
> On 2017/01/23 08:28:42, hashimoto wrote:
> > Please add a brief comment to describe why you chose
base::Time::UnixEpoch(),
> > instead of any other values (e.g. base::Time()).
> 
> Any value should work, so I have nothing to comment. Or if you have any
> suggestion to use any specific value here, please let me know.

Then it'd be nice to have a comment which says this value was arbitrarily chosen
and any other value should also work well.
That kind of comment would help future developers who may be going to replace
this code with new code.

https://codereview.chromium.org/2637163002/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:92:
run_loop.RunUntilIdle();
Can't you just base::RunLoop().RunUntilIdle()?

[Shuhei Takahashi, 2017-01-24 04:18:04]

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
File chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc (right):

https://codereview.chromium.org/2637163002/diff/100001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc:129:
base::Time::UnixEpoch();
On 2017/01/24 04:05:45, hashimoto wrote:
> On 2017/01/23 09:17:51, Shuhei Takahashi wrote:
> > On 2017/01/23 08:28:42, hashimoto wrote:
> > > Please add a brief comment to describe why you chose
> base::Time::UnixEpoch(),
> > > instead of any other values (e.g. base::Time()).
> > 
> > Any value should work, so I have nothing to comment. Or if you have any
> > suggestion to use any specific value here, please let me know.
> 
> Then it'd be nice to have a comment which says this value was arbitrarily
chosen
> and any other value should also work well.
> That kind of comment would help future developers who may be going to replace
> this code with new code.

Done.

https://codereview.chromium.org/2637163002/diff/140001/chrome/browser/chromeo...
File
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc
(right):

https://codereview.chromium.org/2637163002/diff/140001/chrome/browser/chromeo...
chrome/browser/chromeos/arc/fileapi/arc_deferred_file_system_operation_runner_unittest.cc:92:
run_loop.RunUntilIdle();
On 2017/01/24 04:05:45, hashimoto wrote:
> Can't you just base::RunLoop().RunUntilIdle()?

Thanks, it works. I misunderstood that we need to construct base::RunLoop before
tasks are posted...

[Shuhei Takahashi, 2017-01-24 04:18:19]

The CQ bit was checked by nya@chromium.org

[Shuhei Takahashi, 2017-01-24 04:18:19]

The patchset sent to the CQ was uploaded after l-g-t-m from
mtomasz@chromium.org, hidehiko@chromium.org, hashimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2637163002/#ps160001
(title: "Addressed hashimoto's comments.")

[commit-bot: I haz the power, 2017-01-24 04:18:39]

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, 2017-01-24 05:16:18]

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

[commit-bot: I haz the power, 2017-01-24 05:16:20]

Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Shuhei Takahashi, 2017-01-24 05:17:03]

The CQ bit was checked by nya@chromium.org

[commit-bot: I haz the power, 2017-01-24 05:17:13]

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, 2017-01-24 05:33:20]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1485235023346580,
"parent_rev": "2a90b3aac07b05d5639f5de565c33a66ecc84ff3", "commit_rev":
"d280ab1a052245d5bcacf63a4478fc3c0f085899"}

[commit-bot: I haz the power, 2017-01-24 05:34:28]

Description was changed from

==========
Defer ARC file system operations while ARC is booting.

An additional abstraction layer, ArcFileSystemOperationRunner, is inserted
above mojom::FileSystemInstance. Its production implementation is
ArcDeferredFileSystemOperationRunner, which defers file system operations
while ARC is booting.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:

- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready because ReadDirectory
  operations are deferred.
- When an Android content URL is opened soon after the user logs into
  the system (because the user opened the tab before they logged out for
  instance), the tab keeps loading until ARC boot finishes, instead of
  failing immediately.

This patch also changes following points for better media view UX:

- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=chromium:682577
TEST=unit_tests
TEST=Media views are mounted immediately, and a spinner is shown if it is
opened.
==========

to

==========
Defer ARC file system operations while ARC is booting.

An additional abstraction layer, ArcFileSystemOperationRunner, is inserted
above mojom::FileSystemInstance. Its production implementation is
ArcDeferredFileSystemOperationRunner, which defers file system operations
while ARC is booting.

This provides better UX when the user attempts to perform file operations
while ARC is booting. For example:

- Media views are mounted in Files app soon after the user logs into
  the system. If the user attempts to open media views before ARC boots,
  a spinner is shown until file system gets ready because ReadDirectory
  operations are deferred.
- When an Android content URL is opened soon after the user logs into
  the system (because the user opened the tab before they logged out for
  instance), the tab keeps loading until ARC boot finishes, instead of
  failing immediately.

This patch also changes following points for better media view UX:

- Mount/unmount media view volumes according to ARC opt-in status, not ARC
  file system instance availability.
- Add a short circuit in ArcDocumentsProviderRoot to return a metadata for
  root directories. Otherwise Files app fails to update the file system list
  (the left pane of UI) until deferred operations finish.

BUG=chromium:682577
TEST=unit_tests
TEST=Media views are mounted immediately, and a spinner is shown if it is
opened.

Review-Url: https://codereview.chromium.org/2637163002
Cr-Commit-Position: refs/heads/master@{#445657}
Committed:
https://chromium.googlesource.com/chromium/src/+/d280ab1a052245d5bcacf63a4478...
==========

[commit-bot: I haz the power, 2017-01-24 05:34:31]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/d280ab1a052245d5bcacf63a4478...