Split ArcSessionManager::OnPrimaryUserProfilePrepared().

Split ArcSessionManager::OnPrimaryUserProfilePrepared().

The operations in the method can be categorlized into two groups:
1) Set Profile instance and instantiate several objects
  which depends on the Profile instnace.
2) Start observing arc.enabled preference, and sets initial state
  based on its initial value.

This CL splits the method into them. 1) is SetProfile() and 2) is
StartPreferenceHandler().
For that perspective, IsArcAllowedForProfile() check is moved
into ArcServiceLauncher::OnPrimaryUserProfilePrepared(), which is
the only caller of the original
ArcSessionManager::OnPrimaryUserProfilePrepared().

Along with the change, tests, specifically ArcSessionManagerTests
are rewritten.

BUG=657687
BUG=b/31079732
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2708923013
Cr-Commit-Position: refs/heads/master@{#453640}
Committed: https://chromium.googlesource.com/chromium/src/+/2ad9932506a578cd937848e2c7b2904ee4d67268

[hidehiko, 2017-02-24 17:45:40]

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

[commit-bot: I haz the power, 2017-02-24 17:46:05]

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, 2017-02-24 18:18:39]

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

[commit-bot: I haz the power, 2017-02-24 18:18:40]

Dry run: This issue passed the CQ dry run.

[hidehiko, 2017-02-24 18:29:12]

hidehiko@chromium.org changed reviewers:
+ benwells@chromium.org, lhchavez@chromium.org, skuhne@chromium.org,
skym@chromium.org

[hidehiko, 2017-02-24 18:29:13]

PTAL.

lhchavez@: Main reviewer. c/b/c/arc OWNER.
skym@: c/b/sync OWNER
benwells@: c/b/ui/app_list/ OWNER (usually I'm asking xiyuan@, but he's OOO. So
could you help?). c/b/ui/views/apps/ OWNER.
skuhne@: c/b/ui/ash/launcher OWNER.

Thank you for review in advance.
- hidehiko

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (left):

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:238: void FetchAuthCode();
This is declared but not defined (unused). So slightly offtopic, but removed
here.

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.h (right):

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.h:142: void
StartPreferenceHandler();
Note: this is named after the new class planning to be extracted, which is
ArcPlayStoreEnabledPreferenceHandler, since we already have PreferenceHandler
for the class name which observes preference changes.

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (left):

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:277:
ASSERT_EQ(ArcSessionManager::State::NOT_INITIALIZED,
Note: this is unexpected use case, so may be not a good testing.
The re-login case is tested above.

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:369:
arc_session_manager()->Shutdown();
Similarly, split into two tests.

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:391:
TEST_F(ArcSessionManagerTest, DisabledForDeviceLocalAccount) {
Removed. This is testing for public account, which is already tested in
ChromeArcUtilTest.

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:423:
TEST_F(ArcSessionManagerTest, DisabledForNonPrimaryProfile) {
Removed. Similary, this is also tested in ChromeArcUtilTest.

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/ui/ash/launc...
File chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc
(left):

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/ui/ash/launc...
chrome/browser/ui/ash/launcher/chrome_launcher_controller_impl_unittest.cc:2142:
arc::ArcSessionManager::Get()->Shutdown();
This is unexpected usage. No need to test.

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/ui/views/app...
File
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc
(left):

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/ui/views/app...
chrome/browser/ui/views/apps/app_info_dialog/app_info_dialog_ash_unittest.cc:37:
arc::ArcSessionManager::DisableUIForTesting();
Unused. Just removed.

[skym, 2017-02-24 18:41:25]

c/b/sync lgtm

[Mr4D (OOO till 08-26), 2017-02-24 22:36:01]

c/b/ui/ash/launcher lgtm

[Yusuke Sato, 2017-02-25 01:11:02]

yusukes@chromium.org changed reviewers:
+ yusukes@chromium.org

[Yusuke Sato, 2017-02-25 01:11:03]

Luis is ooo today. lgtm

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:500: VLOG(1) << "ARC is
already enabled.";
nit: VLOG messages seem outdated by now. Some (or many) of "ARC" should now be
"Play Store". Mind updating them, maybe in a separate CL?

[hidehiko, 2017-02-27 22:39:53]

Ben, friendly ping?

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
File chrome/browser/chromeos/arc/arc_session_manager.cc (right):

https://codereview.chromium.org/2708923013/diff/1/chrome/browser/chromeos/arc...
chrome/browser/chromeos/arc/arc_session_manager.cc:500: VLOG(1) << "ARC is
already enabled.";
On 2017/02/25 01:11:03, Yusuke Sato wrote:
> nit: VLOG messages seem outdated by now. Some (or many) of "ARC" should now be
> "Play Store". Mind updating them, maybe in a separate CL?

Sure, will do in another CL.

[benwells, 2017-02-28 02:27:44]

Sorry for the delay, somehow missed this.

app_list and app_info_dialog lgtm

[hidehiko, 2017-02-28 16:19:55]

The CQ bit was checked by hidehiko@chromium.org

[commit-bot: I haz the power, 2017-02-28 16:20:24]

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-28 16:22:29]

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

[commit-bot: I haz the power, 2017-02-28 16:22:30]

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

[hidehiko, 2017-02-28 17:37:08]

Patchset #2 (id:20001) has been deleted

[hidehiko, 2017-02-28 17:37:28]

Rebased and re-submitting.

[hidehiko, 2017-02-28 17:38:10]

The CQ bit was checked by hidehiko@chromium.org

[hidehiko, 2017-02-28 17:38:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
benwells@chromium.org, skym@chromium.org, yusukes@chromium.org,
skuhne@chromium.org
Link to the patchset: https://codereview.chromium.org/2708923013/#ps40001
(title: "rebase")

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

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-28 18:07:44]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1488303490835760,
"parent_rev": "1c6f48bce7e97d90037adb101dcd1fb12592787d", "commit_rev":
"2ad9932506a578cd937848e2c7b2904ee4d67268"}

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

Description was changed from

==========
Split ArcSessionManager::OnPrimaryUserProfilePrepared().

The operations in the method can be categorlized into two groups:
1) Set Profile instance and instantiate several objects
  which depends on the Profile instnace.
2) Start observing arc.enabled preference, and sets initial state
  based on its initial value.

This CL splits the method into them. 1) is SetProfile() and 2) is
StartPreferenceHandler().
For that perspective, IsArcAllowedForProfile() check is moved
into ArcServiceLauncher::OnPrimaryUserProfilePrepared(), which is
the only caller of the original
ArcSessionManager::OnPrimaryUserProfilePrepared().

Along with the change, tests, specifically ArcSessionManagerTests
are rewritten.

BUG=657687
BUG=b/31079732
TEST=Ran trybots.
==========

to

==========
Split ArcSessionManager::OnPrimaryUserProfilePrepared().

The operations in the method can be categorlized into two groups:
1) Set Profile instance and instantiate several objects
  which depends on the Profile instnace.
2) Start observing arc.enabled preference, and sets initial state
  based on its initial value.

This CL splits the method into them. 1) is SetProfile() and 2) is
StartPreferenceHandler().
For that perspective, IsArcAllowedForProfile() check is moved
into ArcServiceLauncher::OnPrimaryUserProfilePrepared(), which is
the only caller of the original
ArcSessionManager::OnPrimaryUserProfilePrepared().

Along with the change, tests, specifically ArcSessionManagerTests
are rewritten.

BUG=657687
BUG=b/31079732
TEST=Ran trybots.

Review-Url: https://codereview.chromium.org/2708923013
Cr-Commit-Position: refs/heads/master@{#453640}
Committed:
https://chromium.googlesource.com/chromium/src/+/2ad9932506a578cd937848e2c7b2...
==========

[commit-bot: I haz the power, 2017-02-28 18:08:29]

Committed patchset #2 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2ad9932506a578cd937848e2c7b2...