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..ef23a1bb2ae67daa20b4f9ccf99746fb26cfb6f8 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
@@ -269,11 +269,6 @@ void ArcAppListPrefs::StartPrefs() {
   CHECK(arc_session_manager);
 

[lgcheng, 2017/02/24 01:31:20]

For sync test profiles, I think they are safe and will not affect
ArcSessionManger now.

Although we still make ArcSessionManager observe ArcAppListPrefs created with
synctest profiles, but no notifications will be sent to ArcSessionManager from
those ArcAppListPrefs now. 

We can add more checks if desired. WDYT? 

[hidehiko, 2017/02/24 18:22:26]

I think we should not subscribe if profile_ is not the one ASM has.
Otherwise OnArcPlayStoreEnaledChanged() can be called with unexpected value,
when the Google Play Store enabled preference for profile which ASM has is
updated.

Indeed, the case may not practically happen *now*, just because the current test
does not have such a code path. However, if someone in future hits it, it'd be
very difficult to understand what's going on, so I'd like to avoid it.

So, how about;

Take ArcSessionManager pointer as ArcAppListPrefs's ctor.
In is_sync_test_ == true case (in the factory), pass nullptr to the pointer.
Otherwise (i.e., in real use) pass the real instance ptr.

Then, you can get rid of ArcSessionManager::Get() and use the passed ptr.
If it is nullptr, do nothing. If it is real ptr, subscribe, and call
OnArcPlayStoreEnabledChanged().

Bonus is, in that way, you do not need to modify IsArcAllowedForProfile(), too,
because SyncTest only needs ArcAppListPrefs instances.

Note: this is just one idea, and there should be other alternatives.
My focus is not to tie ASM, if the corresponding profile_ is different from the
one ASM has.
If it is very difficult to realize, could you add comment the reason, in
details?

WDYT?

[lgcheng, 2017/02/25 01:40:38]

Sync test need ArcPackageSyncDataTypeController which relies on
IsArcAllowedForProfile(). So modification of IsArcAllowedForProfile() is still
needed. 

Passing ptr of ASM to ArcAppListPrefs and get rid of ArcSessionManger::Get()
seems not safe neither. It seems there is no guarantee of life circle. (See
~ArcAppListPrefs).

I will try some alternatives.

[hidehiko, 2017/03/02 17:45:38]

Good to know. Then, could you fake ReadyForStart(), instead? (IIUC, that is the
problematic point, right?).

I still am worried about allowing ARC to non-Primary user profile, because it is
global state.
If it is possible to fake in smaller point, that sounds safer.

WDYT?
Oh, miscommunication. I was waiting for your alternatives, actually... Thank you
for ping.

[lgcheng, 2017/03/02 23:45:10]

I think this is the right way to go. PTAL at the update.

   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_));
   }