Start ARC and sign in after Chrome OS login

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 017ecaa23967ecdede358b9d5744db36615be9a9..335f3cadcb1c50eb4c7a11323af6867fa87fc35a 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
@@ -23,6 +23,7 @@
 #include "chrome/common/pref_names.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/arc/arc_service_manager.h"
+#include "components/arc/arc_util.h"
 #include "components/crx_file/id_util.h"
 #include "components/pref_registry/pref_registry_syncable.h"
 #include "components/prefs/scoped_user_pref_update.h"
@@ -142,6 +143,8 @@ void DeleteAppFolderFromFileThread(const base::FilePath& path) {
 }
 
 bool IsArcEnabled() {
+  if (arc::ShouldAlwaysStartArc())
+    return true;
   const auto* arc_session_manager = arc::ArcSessionManager::Get();
   return arc_session_manager && arc_session_manager->IsArcPlayStoreEnabled();
 }
@@ -294,6 +297,14 @@ base::FilePath ArcAppListPrefs::GetIconPath(
 
 void ArcAppListPrefs::RequestIcon(const std::string& app_id,
                                   ui::ScaleFactor scale_factor) {
+#if defined(UNIT_TEST)
+  // FIXME: what's the best way/place to detect and cancel this?

[victorhsieh, 2017/02/25 00:21:33]

Please advise.

[hidehiko, 2017/02/28 19:26:06]

Could you share which test is problematic and its stacktrace?

In real use case, the ASM is destroyed after UI message loop is stopped,
then ArcAppListPrefs is destroyed. Between them we do not expect this is called.

FYI: crbug.com/672829
Slightly related off-topic.
For historical reason, shutdown operation is not in the reverse order of the
construction. To make the situation simpler, it's nice to fix it, IMO.
Considering the current plan, I think early next milestone could be a good
timing for this fix, at the moment.

[victorhsieh, 2017/02/28 21:47:06]

ChromeLauncherControllerImplWithArcTest.ArcAppPin/1 is an example.
https://paste.googleplex.com/5898263773315072
It does look like in this failing case, ASM is destroyed (in TearDown) before
message loop.  So perhaps option 1) keep this check or something similar/better,
option 2) reorder ChromeLauncherControllerImplTest::TearDown (not sure if it's
safe), or option 3) stub out some code to prevent the test going that deep if
possible.  WDYT?

BTW, this problem only surfaces now because without the new flag, this function
returns early at L310.  That is, the callback still runs, but didn't access the
deleted object.

[hidehiko, 2017/03/02 15:57:51]

Great to know. Thank you for explanation.

My two cents, 2) looks better to adapt the real usage.
However, I'm sure existing ArcSessionManager's usage in tests is unexpected,
unfortunately, and this is not the regression. So, as long as TODO is there, I
think it is ok in either way for now.

[victorhsieh, 2017/03/02 18:49:52]

Acknowledged.

+  // ArcSessionManager can be terminated during test tear down, before callback
+  // into this function.
+  if (arc::ArcSessionManager::Get() == nullptr)
+    return;
+#endif
+
   if (!IsRegistered(app_id)) {
     VLOG(2) << "Request to load icon for non-registered app: " << app_id << ".";
     return;
@@ -417,7 +428,7 @@ std::unique_ptr<ArcAppListPrefs::PackageInfo> ArcAppListPrefs::GetPackage(
 }
 
 std::vector<std::string> ArcAppListPrefs::GetAppIds() const {
-  if (!IsArcEnabled()) {
+  if (!arc::ShouldAlwaysStartArc() && !IsArcEnabled()) {
     // Default ARC apps available before OptIn.
     std::vector<std::string> ids;
     for (const auto& default_app : default_apps_.app_map()) {
@@ -431,7 +442,6 @@ std::vector<std::string> ArcAppListPrefs::GetAppIds() const {
 
 std::vector<std::string> ArcAppListPrefs::GetAppIdsNoArcEnabledCheck() const {
   std::vector<std::string> ids;
-
   const base::DictionaryValue* apps = prefs_->GetDictionary(prefs::kArcApps);
   DCHECK(apps);
 
@@ -595,7 +605,9 @@ void ArcAppListPrefs::RemoveAllApps() {
 void ArcAppListPrefs::OnArcPlayStoreEnabledChanged(bool enabled) {
   UpdateDefaultAppsHiddenState();
 
-  if (enabled)
+  // TODO(victorhsieh): refactor the logic.  For Persistent ARC, we need to
+  // collect apps whether or not the user is opted in to Play.
+  if (enabled || arc::ShouldAlwaysStartArc())

[khmel, 2017/02/28 00:10:10]

Top level comment and thoughts.

I think that is might be better to put control logic to Android side only and
here only rely on events from Android.

For example if user Opts out from using Play Store. IIUC we should automatically
delete all non-system packages on Android, right? If so we can try to handle
Package added/removed/updated events and drop logic here to analyze if ARC is
enabled or not. RemoveAllApps here is to handle event ARC stopped as opt-out,
which, it seems no longer the case for pARC. This potentially may seriously
simplify impl here.

Yet another point.
I am not sure if you aware or not about secondary user profile case. At this
moment we don't create ArcAppListPrefs for such profile. We probably need to
revise ArcAppListPrefsFactory. I am not forcing to do in this CL, just keep in
mind.

WDYT?

[victorhsieh, 2017/02/28 18:57:33]

Yes, that's also my plan, too.  This CL should pretty much make this happen
after login, excluding unimplemented opt-in mode.  In fact, this line of change
should not be part of this CL, but the future CL that implements opt-in.  So
reverting.
Secondary profile is very unlikely to be supported in pARC.  In fact, there is a
discussion to rethink the UX.
Yes

[khmel, 2017/02/28 21:58:44]

Even system apps?

[victorhsieh, 2017/02/28 23:43:16]

Yes.  But the plan is still TBD at the moment.

     NotifyRegisteredApps();
   else
     RemoveAllApps();