Run RemoveArcData after a user has opted out

components/arc/user_data/arc_user_data_service.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 "components/arc/user_data/arc_user_data_service.h"
+
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/session_manager_client.h"
+#include "components/arc/arc_service_manager.h"
+#include "components/signin/core/account_id/account_id.h"
+#include "components/user_manager/user_manager.h"
+
+namespace arc {
+
+ArcUserDataService::ArcUserDataService(ArcServiceManager* arc_service_manager) {
+  ArcBridgeService::Get()->AddObserver(this);
+  arc_service_manager_ = arc_service_manager;

[Daniel Erat, 2016/05/20 17:41:06]

assign this in an initialization list instead

[dspaid, 2016/05/23 01:22:37]

Done.

+}
+
+ArcUserDataService::~ArcUserDataService() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  ArcBridgeService::Get()->RemoveObserver(this);
+}
+
+void ArcUserDataService::OnBridgeStopped() {

[Daniel Erat, 2016/05/20 17:41:06]

method order in .cc should match .h

[dspaid, 2016/05/23 01:22:37]

Done.

+  DCHECK(thread_checker_.CalledOnValidThread());
+  const AccountId& account_id =
+      user_manager::UserManager::Get()->GetPrimaryUser()->GetAccountId();
+  if (account_id != user_prefs_account_id_) {

[Daniel Erat, 2016/05/20 17:41:05]

how does this happen? would it be cleaner to check it in ArcServiceManager?

[dspaid, 2016/05/23 01:22:37]

As far as I know it shouldn't happen.  But if for some reason it did it would be
very bad (data loss) so I'd rather check for it.
I don't think it would be any cleaner to check from ArcServiceManager, since
this method is called directly from ArcBridgeService and any logic in
ArcServiceManager would probably have to be as some kind of state return in
IsArcEnabled.

[Daniel Erat, 2016/05/24 14:51:22]

mind making this a LOG(ERROR), then?

[dspaid, 2016/05/25 00:06:48]

Done.

+    LOG(WARNING) << "User preferences not loaded for primary user";

[Daniel Erat, 2016/05/20 17:41:06]

nit: include both ids in the warning message to make it easier to track down
what went wrong?

[dspaid, 2016/05/23 01:22:37]

Done.

+    user_prefs_account_id_ = EmptyAccountId();
+    return;
+  }
+  ClearIfDisabled(account_id);
+}
+
+void ArcUserDataService::ClearIfDisabled(const AccountId& account_id) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  user_prefs_account_id_ = account_id;

[Daniel Erat, 2016/05/20 17:41:06]

i'm confused about how this works. it looks like this member is only assigned
here, but it's checked in OnBridgeStopped(), which calls this method. do you
always expect ClearIfDisabled() to be called at least once before
OnBridgeStopped() is called?

[dspaid, 2016/05/23 01:22:37]

Yes, ClearIfDisabled is called explicitly from OnPrimaryProfilePrepared, which
should always be called before the bridge is started.  Its unfortunately very
indirect, but its the best I could come up with given the difficulty in
accessing preferences from components/arc.

+  if (ArcBridgeService::Get()->state() != ArcBridgeService::State::STOPPED) {
+    LOG(ERROR) << "ARC instance not stopped, user data can't be cleared";
+    return;
+  }
+  if (arc_service_manager_->IsArcEnabled()) {
+    return;
+  }
+  chromeos::SessionManagerClient* session_manager_client =
+      chromeos::DBusThreadManager::Get()->GetSessionManagerClient();
+  session_manager_client->RemoveArcData();
+}
+
+}  // namespace arc