Fix Arc integration test.

chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc

Index: chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
diff --git a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc b/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
index 78512a0c0193e24f2dd0c8aa44b01fcd69cc693d..51c72104b79f6dbbd3d40abd59aea78d6a0384e4 100644
--- a/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
+++ b/chrome/browser/ui/app_list/arc/arc_app_list_prefs.cc
@@ -265,19 +265,23 @@ ArcAppListPrefs::~ArcAppListPrefs() {
 }
 
 void ArcAppListPrefs::StartPrefs() {
-  arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get();
-  CHECK(arc_session_manager);
-
-  if (arc_session_manager->profile()) {
-    // Note: If ArcSessionManager has profile, it should be as same as the one
-    // this instance has, because ArcAppListPrefsFactory creates an instance
-    // only if the given Profile meets ARC's requirement.
-    // Anyway, just in case, check it here.
-    DCHECK_EQ(profile_, arc_session_manager->profile());
-    OnArcPlayStoreEnabledChanged(
-        arc::IsArcPlayStoreEnabledForProfile(profile_));
+  // Don't tie ArcAppListPrefs created with sync test profie in sync integration
+  // test to ArcSessionManager.
+  if (!ArcAppListPrefsFactory::IsFactorySetForSyncTest()) {

[skym, 2017/03/03 19:44:58]

How come the other 3 places in this file that call arc::ArcSessionManager::Get()
don't need special handling?

[hidehiko, 2017/03/06 05:48:32]

Good catch. It should, IMHO.

+    arc::ArcSessionManager* arc_session_manager = arc::ArcSessionManager::Get();

[skym, 2017/03/03 19:44:58]

This seems like it would be more clean if you passed in a weak pointer to a pure
virtual interface into ArcAppListPrefs's constructor, WDYT?

[lgcheng, 2017/03/03 21:13:01]

This is because the life cycle for ArcAppListPrefs and ArcSessionManger is not
strictly the same. I tried to do what you mentioned, but lead to more problems
than it solves. 

Also, it seems there is no plan to make strict condition for life cycle for the
two object at this moment. 

[hidehiko, 2017/03/06 05:48:32]

As for life-time, there is a plan. crbug.com/672829.
Once it is solved, we can assume ASM is alive while this instance is alive.
WeakPtr is an option now, but considering the plan, just if check looks
acceptable for now? Or, we can clean up the WeakPtr later.

WDYT?

+    CHECK(arc_session_manager);

[skym, 2017/03/03 19:44:58]

Is CHECK really the appropriate macro here?
https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md...

[lgcheng, 2017/03/03 21:13:01]

Well I think hidehiko has his own reason for put the check here. But it seems
that's out of the scope of this cl. This cl just excludes this code snippet for
ArcAppListPrefs created with sync test profile. 

[hidehiko, 2017/03/06 05:48:32]

Just historical reason, IIUC. So, DCHECK is preferred, I think.

+
+    if (arc_session_manager->profile()) {
+      // Note: If ArcSessionManager has profile, it should be as same as the one
+      // this instance has, because ArcAppListPrefsFactory creates an instance
+      // only if the given Profile meets ARC's requirement.
+      // Anyway, just in case, check it here.
+      DCHECK_EQ(profile_, arc_session_manager->profile());
+      OnArcPlayStoreEnabledChanged(
+          arc::IsArcPlayStoreEnabledForProfile(profile_));
+    }
+    arc_session_manager->AddObserver(this);
   }
-  arc_session_manager->AddObserver(this);
 
   app_instance_holder_->AddObserver(this);
   if (!app_instance_holder_->has_instance())