arc: Fix Android App resurrected by sync.

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

 // Copyright 2016 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/ui/app_list/arc/arc_package_sync_data_type_controller.h"
 
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/browser/ui/app_list/arc/arc_app_list_prefs.h"
 #include "chrome/common/pref_names.h"
-#include "components/arc/common/app.mojom.h"
-#include "components/arc/instance_holder.h"
+#include "components/arc/arc_bridge_service.h"
 #include "components/prefs/pref_service.h"
+#include "components/sync/driver/pref_names.h"
 #include "components/sync/driver/sync_client.h"
 #include "components/sync/driver/sync_prefs.h"
 #include "components/sync/driver/sync_service.h"
 
 // ArcPackage sync service is controlled by apps checkbox in sync settings. Arc
 // apps and regular Chrome apps have same user control.
+namespace  {
+
+bool ValidateEnableArcPackageSyncPref(Profile* profile) {
+  PrefService* pref_service = profile->GetPrefs();
+  // If device is set to sync everything, Arc package should be synced.
+  if (pref_service->GetBoolean(sync_driver::prefs::kSyncKeepEverythingSynced))
+    return true;
+
+  bool apps_sync_enable = pref_service->GetBoolean(

[khmel, 2016/09/28 17:01:56]

nit: const bool apps_...

[lgcheng, 2016/09/28 17:22:41]

Done.

+      sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::APPS));
+  // ArcPackage sync service is controlled by apps checkbox in sync settings.
+  // Update ArcPackage sync setting pref if it is different from apps sync
+  // setting pref.
+  const char* arc_sync_path =
+      sync_driver::SyncPrefs::GetPrefNameForDataType(syncer::ARC_PACKAGE);
+  if (apps_sync_enable != pref_service->GetBoolean(arc_sync_path)) {
+    pref_service->SetBoolean(arc_sync_path, apps_sync_enable);
+  }
+
+  return apps_sync_enable;
+}
+
+// Indicates whether ARC is enabled on this machine.
+bool IsArcEnabled(Profile* profile) {
+  return profile->GetPrefs()->GetBoolean(prefs::kArcEnabled);

[khmel, 2016/09/28 17:01:56]

This could be dangerous. ArcEnabled may be set in profile but profile itself may
be denied.
You need to use ArcAuthService::IsEnabledForProfile() && your condition here.

[lgcheng, 2016/09/28 17:22:41]

Thanks for pointing out this logic!
Done. But did you mean ArcAuthService::IsAllowedForProfile()?

+}
+
+}  // namespace
+
 ArcPackageSyncDataTypeController::ArcPackageSyncDataTypeController(
     syncer::ModelType type,
     const base::Closure& dump_stack,
     sync_driver::SyncClient* sync_client,
     Profile* profile)
     : sync_driver::UIDataTypeController(type, dump_stack, sync_client),
+      model_normal_start_(true),

[xiyuan, 2016/09/28 16:31:14]

nit: move this initializer to header

[lgcheng, 2016/09/28 16:58:10]

Done.

       profile_(profile),
       sync_client_(sync_client) {
   pref_registrar_.Init(profile_->GetPrefs());
   pref_registrar_.Add(
       sync_driver::SyncPrefs::GetPrefNameForDataType(type),
       base::Bind(&ArcPackageSyncDataTypeController::OnArcAppsSyncPrefChanged,
                  base::Unretained(this)));
   pref_registrar_.Add(
       prefs::kArcEnabled,
       base::Bind(&ArcPackageSyncDataTypeController::OnArcEnabledPrefChanged,
                  base::Unretained(this)));
+
+  arc::ArcBridgeService* arc_bridge_service = arc::ArcBridgeService::Get();
+  if (arc_bridge_service)
+    arc_bridge_service->app()->AddObserver(this);
 }
 
-ArcPackageSyncDataTypeController::~ArcPackageSyncDataTypeController() {}
+ArcPackageSyncDataTypeController::~ArcPackageSyncDataTypeController() {
+  arc::ArcBridgeService* arc_bridge_service = arc::ArcBridgeService::Get();
+  if (arc_bridge_service)
+    arc_bridge_service->app()->RemoveObserver(this);
+}
 
 bool ArcPackageSyncDataTypeController::ReadyForStart() const {
   DCHECK(CalledOnValidThread());
+  return ValidateEnableArcPackageSyncPref(profile_) && IsArcEnabled(profile_);

[xiyuan, 2016/09/28 16:31:14]

nit: swap the two conditions to check IsArcEnabled first ?

[lgcheng, 2016/09/28 16:58:10]

Thanks for pointing out that swap the order normally result in early return.
Done.

+}
+
+void ArcPackageSyncDataTypeController::OnInstanceReady() {
+  // Normally, ArcPackageSyncDataTypeController::StartModels() gets called
+  // before Arc is ready. But in integration test, the order can be either way.
+  // If OnInstanceReady comes before ArcPackageSyncDataTypeController
+  // ::StartModels(), this function is no-op and waits for StartModels() to be
+  // called.

[xiyuan, 2016/09/28 16:31:14]

nit: mention that |model_normal_start_| is initialized as true

[lgcheng, 2016/09/28 16:58:10]

Done.

+  if (model_normal_start_)
+    return;
+
+  model_normal_start_ = true;
+  OnModelLoaded();
+}
+
+bool ArcPackageSyncDataTypeController::StartModels() {
+  DCHECK_EQ(state(), MODEL_STARTING);
   ArcAppListPrefs* prefs = ArcAppListPrefs::Get(profile_);
-  return profile_->GetPrefs()->GetBoolean(
-             sync_driver::SyncPrefs::GetPrefNameForDataType(type())) &&
-         prefs && prefs->app_instance_holder()->has_instance();
+  model_normal_start_ = prefs && prefs->app_instance_holder()->has_instance();
+  return model_normal_start_;
 }
 
 void ArcPackageSyncDataTypeController::OnArcAppsSyncPrefChanged() {
   DCHECK(CalledOnValidThread());
 
   if (!ReadyForStart()) {
     // If apps sync in advanced sync settings is turned off then generate an
     // unrecoverable error.
     if (state() != NOT_RUNNING && state() != STOPPING) {
       syncer::SyncError error(
           FROM_HERE, syncer::SyncError::DATATYPE_POLICY_ERROR,
           "Arc package sync is now disabled by user.", type());
       CreateErrorHandler()->OnUnrecoverableError(error);
     }
     return;
   }
-  sync_driver::SyncService* sync_service = sync_client_->GetSyncService();
-  DCHECK(sync_service);
-  sync_service->ReenableDatatype(type());
+  EnableDataType();
 }
 
 void ArcPackageSyncDataTypeController::OnArcEnabledPrefChanged() {
   DCHECK(CalledOnValidThread());
-  if (!profile_->GetPrefs()->GetBoolean(prefs::kArcEnabled)) {
+
+  if (!ReadyForStart()) {
     // If enable Arc in settings is turned off then generate an unrecoverable
     // error.
     if (state() != NOT_RUNNING && state() != STOPPING) {
       syncer::SyncError error(
           FROM_HERE, syncer::SyncError::DATATYPE_POLICY_ERROR,
           "Arc package sync is now disabled because user disables Arc.",
           type());
       CreateErrorHandler()->OnUnrecoverableError(error);
     }
+    return;
   }
+  EnableDataType();
 }
+
+void ArcPackageSyncDataTypeController::EnableDataType() {
+  sync_driver::SyncService* sync_service = sync_client_->GetSyncService();
+  DCHECK(sync_service);
+  sync_service->ReenableDatatype(type());
+}