mediaview: Fix ARC file system operation deferring.

mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the
refactoring.

This patch moves the construction of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST

Review-Url: https://codereview.chromium.org/2704123007
Cr-Commit-Position: refs/heads/master@{#452441}
Committed: https://chromium.googlesource.com/chromium/src/+/8514e79471d9dfa4a9376317b86f45b57e11eea6

[Shuhei Takahashi, 2017-02-23 08:17:56]

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

[commit-bot: I haz the power, 2017-02-23 08:18:11]

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-02-23 08:19:03]

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

[Shuhei Takahashi, 2017-02-23 08:19:04]

hidehiko: PTAL

[Shuhei Takahashi, 2017-02-23 08:19:57]

Description was changed from

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login. However it is no longer the case after the
refactoring.

This patch moves the construct of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST
==========

to

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the refactoring.

This patch moves the construct of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST
==========

[Shuhei Takahashi, 2017-02-23 08:20:12]

Description was changed from

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the refactoring.

This patch moves the construct of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST
==========

to

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the
refactoring.

This patch moves the construct of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST
==========

[Shuhei Takahashi, 2017-02-23 08:20:35]

Description was changed from

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the
refactoring.

This patch moves the construct of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST
==========

to

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the
refactoring.

This patch moves the construction of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST
==========

[hidehiko, 2017-02-23 08:36:04]

Sorry for the breakage and thank you very much for quick fix!

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

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:45: bool
observe_events)
Optional: Just a random idea.
Now, because |profile| is null to indicate testing instance, and
SetShouldDefer() is always called with false, we may be able to drop
observe_events_?

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

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:105: //
Profile this runner is associated with.
Could you note that this can be null for testing?

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:106:
const Profile* profile_;
const Profile* const profile_;

In ARC, it is encouraged to make this "const" if it is not changed in the class.
You can find it in ARC style guide.

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

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc:39:
callback.Run(-1);
(Not due to this change but) Looks a bug (missing return)?
If you'd like, I'm ok to leave this as is in this CL, and to address it in
another CL.

[commit-bot: I haz the power, 2017-02-23 08:45:02]

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

[commit-bot: I haz the power, 2017-02-23 08:45:03]

Dry run: This issue passed the CQ dry run.

[Shuhei Takahashi, 2017-02-23 08:49:30]

PTAL

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

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.cc:45: bool
observe_events)
On 2017/02/23 08:36:04, hidehiko wrote:
> Optional: Just a random idea.
> Now, because |profile| is null to indicate testing instance, and
> SetShouldDefer() is always called with false, we may be able to drop
> observe_events_?

Indeed |profile_| is nullptr only in testing, but I want to keep
|observe_events_| because checking |profile_ == nullptr| to decide if it should
call observe events (e.g. InstanceHolder) is a bit confusing. Anyway keeping a
field just adds 4byte to Chrome memory consumption since it's essentially a
singleton instance :)

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

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:105: //
Profile this runner is associated with.
On 2017/02/23 08:36:04, hidehiko wrote:
> Could you note that this can be null for testing?

Done.

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner.h:106:
const Profile* profile_;
On 2017/02/23 08:36:04, hidehiko wrote:
> const Profile* const profile_;
> 
> In ARC, it is encouraged to make this "const" if it is not changed in the
class.
> You can find it in ARC style guide.

I agree we want more const :)
Also changed observe_events_.

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

https://codereview.chromium.org/2704123007/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/fileapi/arc_file_system_operation_runner_util.cc:39:
callback.Run(-1);
On 2017/02/23 08:36:04, hidehiko wrote:
> (Not due to this change but) Looks a bug (missing return)?
> If you'd like, I'm ok to leave this as is in this CL, and to address it in
> another CL.

Great catch! Let's fix it now.

[hidehiko, 2017-02-23 08:52:58]

LGTM!

[Shuhei Takahashi, 2017-02-23 08:53:42]

The CQ bit was checked by nya@chromium.org

[commit-bot: I haz the power, 2017-02-23 08:54:02]

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-02-23 09:30:00]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1487840022176910,
"parent_rev": "85fe2a8ce1f31d30a8aa7417472aa9bd42c99f47", "commit_rev":
"8514e79471d9dfa4a9376317b86f45b57e11eea6"}

[commit-bot: I haz the power, 2017-02-23 09:30:38]

Description was changed from

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the
refactoring.

This patch moves the construction of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST
==========

to

==========
mediaview: Fix ARC file system operation deferring.

After recent changes of ARC startup refactoring (crbug.com/657687), ARC
file system operation deferring of media views was broken.

Formerly ArcFileSystemOperationRunner was constructed in
ArcServiceLauncher::Initialize(), which happens before a primary profile
is created. Thus it was wrong to check ARC opt-in status in its
constructor. Nevertheless it seemed to work because
ArcFileSystemOperationRunner::OnArcPlayStoreEnabledChanged() was always
called soon after login, which is no longer the case after the
refactoring.

This patch moves the construction of ArcFileSystemOperationRunner to
ArcServiceLauncher::OnPrimaryUserProfilePrepared() and passes Profile to
ArcFileSystemOperationRunner explicitly.

Due to this change, ARC file operations issued before a primary profile
gets ready are dropped, but we do not expect such operations. But just
in case, we emit error logs in such cases.

BUG=chromium:695358
BUG=chromium:657687
TEST=unit_tests
TEST=No "operations are dropped" lines in /var/log/ui/ui.LATEST

Review-Url: https://codereview.chromium.org/2704123007
Cr-Commit-Position: refs/heads/master@{#452441}
Committed:
https://chromium.googlesource.com/chromium/src/+/8514e79471d9dfa4a9376317b86f...
==========

[commit-bot: I haz the power, 2017-02-23 09:30:39]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/8514e79471d9dfa4a9376317b86f...