Added ConsumerManagementService class to handle enroll state and device owner info in boot lockbox.

chrome/browser/chromeos/policy/consumer_management_service.h

+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_CHROMEOS_POLICY_CONSUMER_MANAGEMENT_SERVICE_H_
+#define CHROME_BROWSER_CHROMEOS_POLICY_CONSUMER_MANAGEMENT_SERVICE_H_
+
+#include <string>
+
+#include "base/callback.h"

[bartfab (slow), 2014/08/04 18:44:52]

Nit: #include "base/callback_forward.h" and move the heavy "base/callback.h" to
the implementation file.

[davidyu, 2014/08/05 07:26:59]

Done.

+#include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "chromeos/dbus/dbus_method_call_status.h"
+
+class PrefRegistrySimple;
+
+namespace chromeos {
+class CryptohomeClient;
+}
+
+namespace cryptohome {
+class BaseReply;
+}
+
+namespace policy {
+
+// Consumer management service handles the enroll state, which is an integer

[bartfab (slow), 2014/08/04 18:44:52]

Nit 1: s/Consumer/The consumer/
Nit 2: s/enroll/enrollment/
Nit 3: The fact that it is an integer is an implementation detail IMHO. I would
reword the comment to say that it is an enum value passed around between various
components and across reboots via user pref.

[davidyu, 2014/08/05 07:26:59]

Done.

+// variable stored in local state to pass the enrollment status between all
+// involved compoments, including settings page, signin page, and user

[bartfab (slow), 2014/08/04 18:44:52]

Nit: What is the "signin page"?

[davidyu, 2014/08/05 07:26:58]

sign-in screen?

[bartfab (slow), 2014/08/05 18:07:13]

Yes, that is the canonical term :).

+// notification. It also handles the owner email stored in boot lockbox.

[bartfab (slow), 2014/08/04 18:44:53]

Nit: s/email/e-mail/

[davidyu, 2014/08/05 07:26:59]

Done.

+class ConsumerManagementService {

[bartfab (slow), 2014/08/04 18:44:51]

I am not sure this name is very well chosen. We have |*ManagementService|
classes in policy code already and they do something very different. How about
something like |ConsumerEnrollmentStatusHelper| or simply
|ConsumerEnrollmentStatus|?

[davidyu, 2014/08/05 07:26:59]

I'm planning to add other consumer management related stuffs in this class as
well, like owner info. So it handles stuffs more than just the enrollment
status.

[bartfab (slow), 2014/08/05 18:07:13]

Thanks for clarifying. The name makes sense then.

+ public:
+  enum EnrollState {

[bartfab (slow), 2014/08/04 18:44:52]

Nit: s/Enroll/Enrollment/

[davidyu, 2014/08/05 07:26:58]

Done.

+    ENROLL_NONE = 0,             // Nothing. All notifications have been sent.

[bartfab (slow), 2014/08/04 18:44:53]

Nit 1: Here and below: s/ENROLL/ENROLLMENT/
Nit 2: What if enrollment was never started to begin with? What state would we
have then? The documentation of NONE sounds like it should be used at the end of
enrollment only, but not before it.

[davidyu, 2014/08/05 07:26:59]

Updated the comment.

+    ENROLL_ENROLLING,            // Enrollment is in progress.
+    ENROLL_SUCCESS,              // Success.
+    ENROLL_CANCELED,             // Canceled by the user.
+    ENROLL_BOOT_LOCKBOX_FAILED,  // Failed to write to the boot lockbox.
+    ENROLL_DM_SERVER_FAILED,     // Failed to register the device.
+  };
+
+  // Registers prefs.
+  static void RegisterPrefs(PrefRegistrySimple* registry);
+
+  explicit ConsumerManagementService(chromeos::CryptohomeClient* client);

[bartfab (slow), 2014/08/04 18:44:52]

Nit: Make the constructor the first method.

[davidyu, 2014/08/05 07:26:58]

Done.

+
+  // Returns the enroll state.
+  EnrollState GetEnrollState();

[bartfab (slow), 2014/08/04 18:44:52]

Nit 1: s/Enroll/Enrollment/g
Nit 2: const.

[davidyu, 2014/08/05 07:26:58]

Done.

Actually, GetOwner() should also be const as well. However, it calls
GetWeakPtr(), which makes it non-constable. I can make weak_ptr_factory_
mutable, but it looks like the "mutable" keyword is not widely used in Chrome. 

[bartfab (slow), 2014/08/05 18:07:13]

Yeah, I would say we prefer non-const to mutable in that case.

+
+  // Sets the enroll state.
+  void SetEnrollState(EnrollState state);

[bartfab (slow), 2014/08/04 18:44:53]

Nit: s/Enroll/Enrollment/g

[davidyu, 2014/08/05 07:26:59]

Done.

+
+  // Returns the device owner stored in the boot lockbox via |callback|.
+  void GetOwner(const base::Callback<void(const std::string&)>& callback);

[bartfab (slow), 2014/08/04 18:44:52]

Nit: Add a typedef for this callback. Since you use the same type in several
places, this will increase readability a lot.

[davidyu, 2014/08/05 07:26:59]

Done.

+
+  // Stores the device owner into the boot lockbox and signs it. |callback| is
+  // invoked with a status when done.

[bartfab (slow), 2014/08/04 18:44:51]

Nit: s/a status/an argzument indicating success or failure/

[davidyu, 2014/08/05 07:26:59]

Done.

+  void SetOwner(const std::string& email,

[bartfab (slow), 2014/08/04 18:44:51]

+rogertawa@

All of Chrome OS needs to move from e-mail addresses to GAIA IDs. I wonder
whether adding one more place where we use an e-mail address instead of a GAIA
ID as we should at this point is a good idea.

Also, nit: Even where we use e-mail addresses for now, we already call the
arguments |user_id| to indicate that we will be switching away from e-mails
really, really soon now.

[davidyu, 2014/08/05 07:26:58]

Changed the argument name to user_id. When we migrate to Gaia ID, we will also
need to have Gaia ID in PolicyData. The current plan is to compare this field
with PolicyData.username. We will also need to get user ID in Gaia sign-in
screen, as it is the source of this field.

[bartfab (slow), 2014/08/05 18:07:13]

Yes, it will be a long and difficult journey. What you say makes sense - but I
would still like to get Roger's opinion before we land this CL. I will try not
to forget to add him this time :).

[davidyu, 2014/08/06 03:04:31]

BTW, we may still need to keep the owner email around during the migration. I
need it to pre-fill the email field of Gaia sign-in page during the enrollment.

+                const base::Callback<void(bool)>& callback);

[bartfab (slow), 2014/08/04 18:44:52]

Nit: Add a typedef for this callback. Since you use the same type in several
places, this will increase readability a lot.

[davidyu, 2014/08/05 07:26:59]

Done.

+
+ private:
+  static const char* kDeviceOwner;

[bartfab (slow), 2014/08/04 18:44:51]

Nit: Move this to an anonymous namespace in the implementation file.

[davidyu, 2014/08/05 07:26:59]

Done.

+
+  void OnGetBootAttributeDone(
+      const base::Callback<void(const std::string&)>& callback,
+      chromeos::DBusMethodCallStatus call_status,
+      bool result,

[bartfab (slow), 2014/08/04 18:44:52]

Nit: How about s/result/success/?

[davidyu, 2014/08/05 07:26:58]

Renamed to dbus_success, as it's just D-Bus calling status.

+      const cryptohome::BaseReply& reply);
+
+  void OnSetBootAttributeDone(const base::Callback<void(bool)>& callback,
+                              chromeos::DBusMethodCallStatus call_status,
+                              bool result,

[bartfab (slow), 2014/08/04 18:44:53]

Nit: How about s/result/success/?

[davidyu, 2014/08/05 07:26:59]

Done.

+                              const cryptohome::BaseReply& reply);
+
+  void OnFlushAndSignBootAttributesDone(
+      const base::Callback<void(bool)>& callback,
+      chromeos::DBusMethodCallStatus call_status,
+      bool result,

[bartfab (slow), 2014/08/04 18:44:53]

Nit: How about s/result/success/?

[davidyu, 2014/08/05 07:26:58]

Done.

+      const cryptohome::BaseReply& reply);
+
+  chromeos::CryptohomeClient* client_;
+  base::WeakPtrFactory<ConsumerManagementService> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(ConsumerManagementService);
+};
+
+}  // namespace policy
+
+#endif  // CHROME_BROWSER_CHROMEOS_POLICY_CONSUMER_MANAGEMENT_SERVICE_H_