Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
438493002:
Added ConsumerManagementService class to handle enroll state and device owner info in boot lockbox. (Closed)
|
Created:
6 years, 4 months ago by davidyu Modified:
6 years, 4 months ago CC:
chromium-reviews, dbeam+watch-options_chromium.org, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@signin Project:
chromium Visibility:
Public. |
DescriptionAdded ConsumerManagementService class to handle enroll state and device owner info in boot lockbox.
Also added the code to store owner info into boot lockbox during enrollment.
BUG=chromium:353050
TEST=unit_tests
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288451
Patch Set 1 #Patch Set 2 : Rename to ConsumerManagementService #Patch Set 3 : Added GetOwner() and SetOwner(). #Patch Set 4 : Added unit tests. #Patch Set 5 : Added the code to store owner email into boot lockbox. #Patch Set 6 : Call FlushAndSignBootAttributes() in SetOwner(). #
Total comments: 127
Patch Set 7 : #
Total comments: 24
Patch Set 8 : #Patch Set 9 : Only create ConsumerManagementService when --enable-consumer-management is specified. #
Total comments: 8
Patch Set 10 : #
Total comments: 6
Patch Set 11 : #Patch Set 12 : Fixed the broken test. #Messages
Total messages: 27 (0 generated)
This CL depends on https://codereview.chromium.org/426153003/
On 2014/07/31 04:37:39, davidyu wrote: > This CL depends on https://codereview.chromium.org/426153003/ More context about this state: the state will be set to NONE once the notification of SUCCESS, CANCELED, or *_FAILED has been sent to the owner.
On 2014/07/31 04:40:20, davidyu wrote: > On 2014/07/31 04:37:39, davidyu wrote: > > This CL depends on https://codereview.chromium.org/426153003/ > > More context about this state: the state will be set to NONE once the > notification of SUCCESS, CANCELED, or *_FAILED has been sent to the owner. BTW, should we make this a service singleton so that others can observe it and send notifications when the state is changed?
On 2014/08/01 07:13:42, davidyu wrote: > On 2014/07/31 04:40:20, davidyu wrote: > > On 2014/07/31 04:37:39, davidyu wrote: > > > This CL depends on https://codereview.chromium.org/426153003/ > > > > More context about this state: the state will be set to NONE once the > > notification of SUCCESS, CANCELED, or *_FAILED has been sent to the owner. > > BTW, should we make this a service singleton so that others can observe it and > send notifications when the state is changed? Since it becomes more complex, I'll add a unit test later.
On 2014/08/01 07:13:42, davidyu wrote: > On 2014/07/31 04:40:20, davidyu wrote: > > On 2014/07/31 04:37:39, davidyu wrote: > > > This CL depends on https://codereview.chromium.org/426153003/ > > > > More context about this state: the state will be set to NONE once the > > notification of SUCCESS, CANCELED, or *_FAILED has been sent to the owner. > > BTW, should we make this a service singleton so that others can observe it and > send notifications when the state is changed? Re adding a singleton: There is a very strong preference in the enteprise team to avoid singletons. They have problematic start-up and shutdown semantincs and shutting them down in a well-defined order is very difficult (it frequently goes wrong with the singletons we do have in Chrome). The BrowserPolicyConnector is the right home for additional services like this one.
Looks good to me overall, just some comments/nits. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:152: chromeos::CryptohomeClient* cryptohome_client = Nit: #include "chromeos/dbus/cryptohome_client.h" https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:113: ConsumerManagementService* GetConsumerManagementService() const { This is pretty confusing now. We have GetConsumerDeviceManagementService() and GetConsumerManagementService()(). You really have to squint your eyes to see the difference between the two. How about renaming the former to GetDeviceManagementServiceForConsumer() or at least adding brief comments explaining the difference between the two? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:33: ConsumerManagementService::GetEnrollState() { Nit: The style guide says that if you break between the return type and the method name, you should not indent. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:35: return static_cast<EnrollState>(prefs->GetInteger( What if the pref value is outside the valid range? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:42: prefs->CommitPendingWrite(); Why? Prefs get auto-committed for you. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:52: weak_ptr_factory_.GetWeakPtr(), callback)); Nit: Put each argument on a separate line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:79: weak_ptr_factory_.GetWeakPtr(), callback)); Nit: Put each argument on a separate line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:97: weak_ptr_factory_.GetWeakPtr(), callback)); Nit: Put each argument on a separate line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:10: #include "base/callback.h" Nit: #include "base/callback_forward.h" and move the heavy "base/callback.h" to the implementation file. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:27: // Consumer management service handles the enroll state, which is an integer 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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:29: // involved compoments, including settings page, signin page, and user Nit: What is the "signin page"? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:30: // notification. It also handles the owner email stored in boot lockbox. Nit: s/email/e-mail/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:31: class ConsumerManagementService { 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|? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:33: enum EnrollState { Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:34: ENROLL_NONE = 0, // Nothing. All notifications have been sent. 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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:45: explicit ConsumerManagementService(chromeos::CryptohomeClient* client); Nit: Make the constructor the first method. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:48: EnrollState GetEnrollState(); Nit 1: s/Enroll/Enrollment/g Nit 2: const. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:51: void SetEnrollState(EnrollState state); Nit: s/Enroll/Enrollment/g https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:54: void GetOwner(const base::Callback<void(const std::string&)>& callback); Nit: Add a typedef for this callback. Since you use the same type in several places, this will increase readability a lot. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:57: // invoked with a status when done. Nit: s/a status/an argzument indicating success or failure/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:58: void SetOwner(const std::string& email, +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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:59: const base::Callback<void(bool)>& callback); Nit: Add a typedef for this callback. Since you use the same type in several places, this will increase readability a lot. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:62: static const char* kDeviceOwner; Nit: Move this to an anonymous namespace in the implementation file. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:67: bool result, Nit: How about s/result/success/? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:72: bool result, Nit: How about s/result/success/? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:78: bool result, Nit: How about s/result/success/? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:7: #include <string> Nit: Already included by header. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:18: #include "chromeos/dbus/dbus_method_call_status.h" Nit: Already included by header. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:28: namespace { Nit: No need to wrap the entire test in an anonymous namespace. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:32: ConsumerManagementServiceTest() : Nit: Per style guide, move the : to the next line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:81: bool cryptohome_result_; Nit: Initialize this in the constructor to guarantee you will not be using an uninitialized variable. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:87: bool set_owner_status_; Nit: Initialize this in the constructor to guarantee you will not be using an uninitialized variable. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:90: TEST_F(ConsumerManagementServiceTest, CanGetEnrollState) { Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:95: EXPECT_EQ(ConsumerManagementService::ENROLL_ENROLLING, Nit: Check first that before the set call, GetEnrollState() returns something different. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:99: TEST_F(ConsumerManagementServiceTest, CanSetEnrollState) { Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:102: EXPECT_EQ(ConsumerManagementService::ENROLL_ENROLLING, Nit: Check first that before the set call, GetInteger() returns something different. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:111: ->set_value("test@chromium.org"); Nit 1: Move the -> operator to the previous line. Nit 2: Extract "test@chromium.org" to a constant at the top of the file. Nit 3: Use a domain ending in one of the reserved extensions, like ".example" or ".test," not a real domain. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:116: EXPECT_EQ("consumer_management.owner", get_boot_attribute_request_.name()); Nit: Extract "consumer_management.owner" to a constant at the top of the file. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:120: TEST_F(ConsumerManagementServiceTest, GetOwnerReturnsAnEmptyStringWhenFails) { Nit: s/When/WhenIt/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:124: ->set_value("test@chromium.org"); Nit: Move the -> operator to the previous line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:145: TEST_F(ConsumerManagementServiceTest, SetOwnerReturnsFalseWhenFails) { Nit: s/When/WhenIt/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:297: policy::BrowserPolicyConnectorChromeOS* connector = g_browser_process While I was working on another screen handler last week, Nikita asked me to factor out any new policy dependencies so that the screen handlers can easily move to a component for Athena. I think it may be best to pass in a policy::ConsumerManagementService* as a dependency in the constructor, and allow it to be NULL if not used (e.g. on Athena). There will still be a dependency on the consumer_management_service.h include but that is fine IMHO. The goal is to make it easy to untangle the policy dependency when the time comes, not to make it 100% nonexistent straight away. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:298: ->platform_part()->browser_policy_connector_chromeos(); Nit 1: Move the -> operator to the previous line. Nit 2: #include "chrome/browser/browser_process_platform_part.h" https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:301: base::Bind(&GaiaScreenHandler::OnSetOwnerDone, Nit: #include "base/bind.h" https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:303: typed_email, password, using_saml)); Nit: Add a return statement after this. That way, it becomes clear that the method ends at the end of this conditional. It will allow you to remove the final else in line 310. Or, even better, restructure as: if (is_enrolling_consumer_management_) { DoCompleteLogin(typed_email, password, using_saml); return; } if (typed_email == owner_email) { ... return; } ... https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:305: // Show Gaia signin page again since we only allow the owner to sign in. Nit: s/signin/sign-in/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:318: bool result) { Nit: |result| is unclear. Better call this |success|. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:320: LOG(ERROR) << "Failed to write owner email to boot lockbox."; Nit: s/email/e-mail/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:322: ->platform_part()->browser_policy_connector_chromeos(); Nit: Move the -> operator to the previous line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:88: I understand why you added the blank line here and put |OnSetOwnerDone| and |DoCompleteLogin| right after |HandleCompleteLogin| but I do not think this is the ideal methord order. IMHO, all |Handle*| methods should be listed here one after another, followed eventually by the helpers that they use. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:92: void OnSetOwnerDone(const std::string& typed_email, Nit: #include <string> https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:95: bool result); Nit: |result| is unclear. Better call this |success|. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:308: connector->GetConsumerManagementService()->GetEnrollState() == Nit: Indent two more spaces. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:309: policy::ConsumerManagementService::ENROLL_ENROLLING; Nit: Indent two more spaces. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc:86: policy::BrowserPolicyConnectorChromeOS* connector = See my comment in gaia_screen_handler.cc regarding adding new dependencies on policy code in screen handlers. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc:87: g_browser_process->platform_part()->browser_policy_connector_chromeos(); Nit 1: Indent two more spaces. Nit 2: #include "chrome/browser/browser_process_platform_part_ https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1904: // An integer pref of the current consumer management enroll state. Nit 1: s/enroll/enrollment/ Nit 2: Could you point out in the comment where the valid enum values for this pref are defined? https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1905: const char kConsumerManagementEnrollState[] = Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1906: "consumer_management.enroll_state"; Nit: s/enroll/enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.h:678: extern const char kConsumerManagementEnrollState[]; Nit: s/Enroll/Enrollment/
Looks good to me overall, just some comments/nits. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:152: chromeos::CryptohomeClient* cryptohome_client = Nit: #include "chromeos/dbus/cryptohome_client.h" https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:113: ConsumerManagementService* GetConsumerManagementService() const { This is pretty confusing now. We have GetConsumerDeviceManagementService() and GetConsumerManagementService()(). You really have to squint your eyes to see the difference between the two. How about renaming the former to GetDeviceManagementServiceForConsumer() or at least adding brief comments explaining the difference between the two? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:33: ConsumerManagementService::GetEnrollState() { Nit: The style guide says that if you break between the return type and the method name, you should not indent. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:35: return static_cast<EnrollState>(prefs->GetInteger( What if the pref value is outside the valid range? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:42: prefs->CommitPendingWrite(); Why? Prefs get auto-committed for you. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:52: weak_ptr_factory_.GetWeakPtr(), callback)); Nit: Put each argument on a separate line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:79: weak_ptr_factory_.GetWeakPtr(), callback)); Nit: Put each argument on a separate line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:97: weak_ptr_factory_.GetWeakPtr(), callback)); Nit: Put each argument on a separate line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:10: #include "base/callback.h" Nit: #include "base/callback_forward.h" and move the heavy "base/callback.h" to the implementation file. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:27: // Consumer management service handles the enroll state, which is an integer 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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:29: // involved compoments, including settings page, signin page, and user Nit: What is the "signin page"? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:30: // notification. It also handles the owner email stored in boot lockbox. Nit: s/email/e-mail/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:31: class ConsumerManagementService { 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|? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:33: enum EnrollState { Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:34: ENROLL_NONE = 0, // Nothing. All notifications have been sent. 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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:45: explicit ConsumerManagementService(chromeos::CryptohomeClient* client); Nit: Make the constructor the first method. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:48: EnrollState GetEnrollState(); Nit 1: s/Enroll/Enrollment/g Nit 2: const. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:51: void SetEnrollState(EnrollState state); Nit: s/Enroll/Enrollment/g https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:54: void GetOwner(const base::Callback<void(const std::string&)>& callback); Nit: Add a typedef for this callback. Since you use the same type in several places, this will increase readability a lot. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:57: // invoked with a status when done. Nit: s/a status/an argzument indicating success or failure/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:58: void SetOwner(const std::string& email, +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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:59: const base::Callback<void(bool)>& callback); Nit: Add a typedef for this callback. Since you use the same type in several places, this will increase readability a lot. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:62: static const char* kDeviceOwner; Nit: Move this to an anonymous namespace in the implementation file. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:67: bool result, Nit: How about s/result/success/? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:72: bool result, Nit: How about s/result/success/? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:78: bool result, Nit: How about s/result/success/? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:7: #include <string> Nit: Already included by header. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:18: #include "chromeos/dbus/dbus_method_call_status.h" Nit: Already included by header. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:28: namespace { Nit: No need to wrap the entire test in an anonymous namespace. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:32: ConsumerManagementServiceTest() : Nit: Per style guide, move the : to the next line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:81: bool cryptohome_result_; Nit: Initialize this in the constructor to guarantee you will not be using an uninitialized variable. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:87: bool set_owner_status_; Nit: Initialize this in the constructor to guarantee you will not be using an uninitialized variable. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:90: TEST_F(ConsumerManagementServiceTest, CanGetEnrollState) { Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:95: EXPECT_EQ(ConsumerManagementService::ENROLL_ENROLLING, Nit: Check first that before the set call, GetEnrollState() returns something different. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:99: TEST_F(ConsumerManagementServiceTest, CanSetEnrollState) { Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:102: EXPECT_EQ(ConsumerManagementService::ENROLL_ENROLLING, Nit: Check first that before the set call, GetInteger() returns something different. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:111: ->set_value("test@chromium.org"); Nit 1: Move the -> operator to the previous line. Nit 2: Extract "test@chromium.org" to a constant at the top of the file. Nit 3: Use a domain ending in one of the reserved extensions, like ".example" or ".test," not a real domain. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:116: EXPECT_EQ("consumer_management.owner", get_boot_attribute_request_.name()); Nit: Extract "consumer_management.owner" to a constant at the top of the file. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:120: TEST_F(ConsumerManagementServiceTest, GetOwnerReturnsAnEmptyStringWhenFails) { Nit: s/When/WhenIt/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:124: ->set_value("test@chromium.org"); Nit: Move the -> operator to the previous line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:145: TEST_F(ConsumerManagementServiceTest, SetOwnerReturnsFalseWhenFails) { Nit: s/When/WhenIt/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:297: policy::BrowserPolicyConnectorChromeOS* connector = g_browser_process While I was working on another screen handler last week, Nikita asked me to factor out any new policy dependencies so that the screen handlers can easily move to a component for Athena. I think it may be best to pass in a policy::ConsumerManagementService* as a dependency in the constructor, and allow it to be NULL if not used (e.g. on Athena). There will still be a dependency on the consumer_management_service.h include but that is fine IMHO. The goal is to make it easy to untangle the policy dependency when the time comes, not to make it 100% nonexistent straight away. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:298: ->platform_part()->browser_policy_connector_chromeos(); Nit 1: Move the -> operator to the previous line. Nit 2: #include "chrome/browser/browser_process_platform_part.h" https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:301: base::Bind(&GaiaScreenHandler::OnSetOwnerDone, Nit: #include "base/bind.h" https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:303: typed_email, password, using_saml)); Nit: Add a return statement after this. That way, it becomes clear that the method ends at the end of this conditional. It will allow you to remove the final else in line 310. Or, even better, restructure as: if (is_enrolling_consumer_management_) { DoCompleteLogin(typed_email, password, using_saml); return; } if (typed_email == owner_email) { ... return; } ... https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:305: // Show Gaia signin page again since we only allow the owner to sign in. Nit: s/signin/sign-in/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:318: bool result) { Nit: |result| is unclear. Better call this |success|. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:320: LOG(ERROR) << "Failed to write owner email to boot lockbox."; Nit: s/email/e-mail/ https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:322: ->platform_part()->browser_policy_connector_chromeos(); Nit: Move the -> operator to the previous line. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:88: I understand why you added the blank line here and put |OnSetOwnerDone| and |DoCompleteLogin| right after |HandleCompleteLogin| but I do not think this is the ideal methord order. IMHO, all |Handle*| methods should be listed here one after another, followed eventually by the helpers that they use. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:92: void OnSetOwnerDone(const std::string& typed_email, Nit: #include <string> https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:95: bool result); Nit: |result| is unclear. Better call this |success|. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:308: connector->GetConsumerManagementService()->GetEnrollState() == Nit: Indent two more spaces. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:309: policy::ConsumerManagementService::ENROLL_ENROLLING; Nit: Indent two more spaces. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc:86: policy::BrowserPolicyConnectorChromeOS* connector = See my comment in gaia_screen_handler.cc regarding adding new dependencies on policy code in screen handlers. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc:87: g_browser_process->platform_part()->browser_policy_connector_chromeos(); Nit 1: Indent two more spaces. Nit 2: #include "chrome/browser/browser_process_platform_part_ https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1904: // An integer pref of the current consumer management enroll state. Nit 1: s/enroll/enrollment/ Nit 2: Could you point out in the comment where the valid enum values for this pref are defined? https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1905: const char kConsumerManagementEnrollState[] = Nit: s/Enroll/Enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1906: "consumer_management.enroll_state"; Nit: s/enroll/enrollment/ https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.h:678: extern const char kConsumerManagementEnrollState[]; Nit: s/Enroll/Enrollment/
Wow! Thanks for so many comments! They are very appreciated. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.cc:152: chromeos::CryptohomeClient* cryptohome_client = On 2014/08/04 18:44:50, bartfab wrote: > Nit: #include "chromeos/dbus/cryptohome_client.h" Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/browser_policy_connector_chromeos.h:113: ConsumerManagementService* GetConsumerManagementService() const { On 2014/08/04 18:44:50, bartfab wrote: > This is pretty confusing now. We have GetConsumerDeviceManagementService() and > GetConsumerManagementService()(). You really have to squint your eyes to see the > difference between the two. How about renaming the former to > GetDeviceManagementServiceForConsumer() or at least adding brief comments > explaining the difference between the two? Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:33: ConsumerManagementService::GetEnrollState() { On 2014/08/04 18:44:51, bartfab wrote: > Nit: The style guide says that if you break between the return type and the > method name, you should not indent. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:35: return static_cast<EnrollState>(prefs->GetInteger( On 2014/08/04 18:44:51, bartfab wrote: > What if the pref value is outside the valid range? Added error handling code. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:42: prefs->CommitPendingWrite(); On 2014/08/04 18:44:51, bartfab wrote: > Why? Prefs get auto-committed for you. If we call PowerManagementClient::RequestRestart() immediately after this function, does pref service still have time to commit the pending writes? I saw in some place that we call CommitPendingWrite() before RequestRestart(), so I thought it might be necessary. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:52: weak_ptr_factory_.GetWeakPtr(), callback)); On 2014/08/04 18:44:51, bartfab wrote: > Nit: Put each argument on a separate line. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:79: weak_ptr_factory_.GetWeakPtr(), callback)); On 2014/08/04 18:44:51, bartfab wrote: > Nit: Put each argument on a separate line. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:97: weak_ptr_factory_.GetWeakPtr(), callback)); On 2014/08/04 18:44:51, bartfab wrote: > Nit: Put each argument on a separate line. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:10: #include "base/callback.h" On 2014/08/04 18:44:52, bartfab wrote: > Nit: #include "base/callback_forward.h" and move the heavy "base/callback.h" to > the implementation file. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:27: // Consumer management service handles the enroll state, which is an integer On 2014/08/04 18:44:52, bartfab wrote: > 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. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:29: // involved compoments, including settings page, signin page, and user On 2014/08/04 18:44:52, bartfab wrote: > Nit: What is the "signin page"? sign-in screen? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:30: // notification. It also handles the owner email stored in boot lockbox. On 2014/08/04 18:44:53, bartfab wrote: > Nit: s/email/e-mail/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:31: class ConsumerManagementService { On 2014/08/04 18:44:51, bartfab wrote: > 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|? 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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:33: enum EnrollState { On 2014/08/04 18:44:52, bartfab wrote: > Nit: s/Enroll/Enrollment/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:34: ENROLL_NONE = 0, // Nothing. All notifications have been sent. On 2014/08/04 18:44:53, bartfab wrote: > 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. Updated the comment. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:45: explicit ConsumerManagementService(chromeos::CryptohomeClient* client); On 2014/08/04 18:44:52, bartfab wrote: > Nit: Make the constructor the first method. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:48: EnrollState GetEnrollState(); On 2014/08/04 18:44:52, bartfab wrote: > Nit 1: s/Enroll/Enrollment/g > Nit 2: const. 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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:51: void SetEnrollState(EnrollState state); On 2014/08/04 18:44:53, bartfab wrote: > Nit: s/Enroll/Enrollment/g Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:54: void GetOwner(const base::Callback<void(const std::string&)>& callback); On 2014/08/04 18:44:52, bartfab wrote: > Nit: Add a typedef for this callback. Since you use the same type in several > places, this will increase readability a lot. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:57: // invoked with a status when done. On 2014/08/04 18:44:51, bartfab wrote: > Nit: s/a status/an argzument indicating success or failure/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:58: void SetOwner(const std::string& email, On 2014/08/04 18:44:51, bartfab wrote: > +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. 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. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:59: const base::Callback<void(bool)>& callback); On 2014/08/04 18:44:52, bartfab wrote: > Nit: Add a typedef for this callback. Since you use the same type in several > places, this will increase readability a lot. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:62: static const char* kDeviceOwner; On 2014/08/04 18:44:51, bartfab wrote: > Nit: Move this to an anonymous namespace in the implementation file. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:67: bool result, On 2014/08/04 18:44:52, bartfab wrote: > Nit: How about s/result/success/? Renamed to dbus_success, as it's just D-Bus calling status. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:72: bool result, On 2014/08/04 18:44:53, bartfab wrote: > Nit: How about s/result/success/? Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:78: bool result, On 2014/08/04 18:44:53, bartfab wrote: > Nit: How about s/result/success/? Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:7: #include <string> On 2014/08/04 18:44:53, bartfab wrote: > Nit: Already included by header. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:18: #include "chromeos/dbus/dbus_method_call_status.h" On 2014/08/04 18:44:53, bartfab wrote: > Nit: Already included by header. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:28: namespace { On 2014/08/04 18:44:55, bartfab wrote: > Nit: No need to wrap the entire test in an anonymous namespace. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:32: ConsumerManagementServiceTest() : On 2014/08/04 18:44:54, bartfab wrote: > Nit: Per style guide, move the : to the next line. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:81: bool cryptohome_result_; On 2014/08/04 18:44:54, bartfab wrote: > Nit: Initialize this in the constructor to guarantee you will not be using an > uninitialized variable. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:87: bool set_owner_status_; On 2014/08/04 18:44:54, bartfab wrote: > Nit: Initialize this in the constructor to guarantee you will not be using an > uninitialized variable. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:90: TEST_F(ConsumerManagementServiceTest, CanGetEnrollState) { On 2014/08/04 18:44:55, bartfab wrote: > Nit: s/Enroll/Enrollment/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:95: EXPECT_EQ(ConsumerManagementService::ENROLL_ENROLLING, On 2014/08/04 18:44:54, bartfab wrote: > Nit: Check first that before the set call, GetEnrollState() returns something > different. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:99: TEST_F(ConsumerManagementServiceTest, CanSetEnrollState) { On 2014/08/04 18:44:54, bartfab wrote: > Nit: s/Enroll/Enrollment/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:102: EXPECT_EQ(ConsumerManagementService::ENROLL_ENROLLING, On 2014/08/04 18:44:54, bartfab wrote: > Nit: Check first that before the set call, GetInteger() returns something > different. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:111: ->set_value("test@chromium.org"); On 2014/08/04 18:44:54, bartfab wrote: > Nit 1: Move the -> operator to the previous line. > Nit 2: Extract mailto:"test@chromium.org" to a constant at the top of the file. > Nit 3: Use a domain ending in one of the reserved extensions, like ".example" or > ".test," not a real domain. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:116: EXPECT_EQ("consumer_management.owner", get_boot_attribute_request_.name()); On 2014/08/04 18:44:54, bartfab wrote: > Nit: Extract "consumer_management.owner" to a constant at the top of the file. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:120: TEST_F(ConsumerManagementServiceTest, GetOwnerReturnsAnEmptyStringWhenFails) { On 2014/08/04 18:44:53, bartfab wrote: > Nit: s/When/WhenIt/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:124: ->set_value("test@chromium.org"); On 2014/08/04 18:44:55, bartfab wrote: > Nit: Move the -> operator to the previous line. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:145: TEST_F(ConsumerManagementServiceTest, SetOwnerReturnsFalseWhenFails) { On 2014/08/04 18:44:54, bartfab wrote: > Nit: s/When/WhenIt/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:297: policy::BrowserPolicyConnectorChromeOS* connector = g_browser_process On 2014/08/04 18:44:55, bartfab wrote: > While I was working on another screen handler last week, Nikita asked me to > factor out any new policy dependencies so that the screen handlers can easily > move to a component for Athena. I think it may be best to pass in a > policy::ConsumerManagementService* as a dependency in the constructor, and allow > it to be NULL if not used (e.g. on Athena). There will still be a dependency on > the consumer_management_service.h include but that is fine IMHO. The goal is to > make it easy to untangle the policy dependency when the time comes, not to make > it 100% nonexistent straight away. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:298: ->platform_part()->browser_policy_connector_chromeos(); On 2014/08/04 18:44:55, bartfab wrote: > Nit 1: Move the -> operator to the previous line. > Nit 2: #include "chrome/browser/browser_process_platform_part.h" Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:301: base::Bind(&GaiaScreenHandler::OnSetOwnerDone, On 2014/08/04 18:44:56, bartfab wrote: > Nit: #include "base/bind.h" Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:303: typed_email, password, using_saml)); On 2014/08/04 18:44:55, bartfab wrote: > Nit: Add a return statement after this. That way, it becomes clear that the > method ends at the end of this conditional. It will allow you to remove the > final else in line 310. > > Or, even better, restructure as: > > if (is_enrolling_consumer_management_) { > DoCompleteLogin(typed_email, password, using_saml); > return; > } > > if (typed_email == owner_email) { > ... > return; > } > > ... Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:305: // Show Gaia signin page again since we only allow the owner to sign in. On 2014/08/04 18:44:56, bartfab wrote: > Nit: s/signin/sign-in/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:318: bool result) { On 2014/08/04 18:44:55, bartfab wrote: > Nit: |result| is unclear. Better call this |success|. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:320: LOG(ERROR) << "Failed to write owner email to boot lockbox."; On 2014/08/04 18:44:56, bartfab wrote: > Nit: s/email/e-mail/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:322: ->platform_part()->browser_policy_connector_chromeos(); On 2014/08/04 18:44:55, bartfab wrote: > Nit: Move the -> operator to the previous line. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:88: On 2014/08/04 18:44:56, bartfab wrote: > I understand why you added the blank line here and put |OnSetOwnerDone| and > |DoCompleteLogin| right after |HandleCompleteLogin| but I do not think this is > the ideal methord order. IMHO, all |Handle*| methods should be listed here one > after another, followed eventually by the helpers that they use. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:92: void OnSetOwnerDone(const std::string& typed_email, On 2014/08/04 18:44:56, bartfab wrote: > Nit: #include <string> Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:95: bool result); On 2014/08/04 18:44:56, bartfab wrote: > Nit: |result| is unclear. Better call this |success|. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:308: connector->GetConsumerManagementService()->GetEnrollState() == On 2014/08/04 18:44:56, bartfab wrote: > Nit: Indent two more spaces. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:309: policy::ConsumerManagementService::ENROLL_ENROLLING; On 2014/08/04 18:44:56, bartfab wrote: > Nit: Indent two more spaces. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc:86: policy::BrowserPolicyConnectorChromeOS* connector = On 2014/08/04 18:44:57, bartfab wrote: > See my comment in gaia_screen_handler.cc regarding adding new dependencies on > policy code in screen handlers. Done. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc:87: g_browser_process->platform_part()->browser_policy_connector_chromeos(); On 2014/08/04 18:44:56, bartfab wrote: > Nit 1: Indent two more spaces. > Nit 2: #include "chrome/browser/browser_process_platform_part_ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1904: // An integer pref of the current consumer management enroll state. On 2014/08/04 18:44:57, bartfab wrote: > Nit 1: s/enroll/enrollment/ > Nit 2: Could you point out in the comment where the valid enum values for this > pref are defined? Done. https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1905: const char kConsumerManagementEnrollState[] = On 2014/08/04 18:44:57, bartfab wrote: > Nit: s/Enroll/Enrollment/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.cc:1906: "consumer_management.enroll_state"; On 2014/08/04 18:44:57, bartfab wrote: > Nit: s/enroll/enrollment/ Done. https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_names.h File chrome/common/pref_names.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/common/pref_name... chrome/common/pref_names.h:678: extern const char kConsumerManagementEnrollState[]; On 2014/08/04 18:44:57, bartfab wrote: > Nit: s/Enroll/Enrollment/ Done.
Hi Nikita, Can you help review chrome/browser/resources/chromeos/login/* and chrome/browser/ui/webui/* Hi Gabriel, Can you help review chrome/browser/prefs/browser_prefs.cc Thanks!
chrome/browser/prefs/browser_prefs.cc lgtm stamp
Roger, could you take a look at the use of the owner e-mail address in the boot lockbox (see ConsumerManagementService::SetOwner()) and give us your thoughts on whether this is OK in light of the eventual switch to GAIA IDs? https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:42: prefs->CommitPendingWrite(); On 2014/08/05 07:26:58, davidyu wrote: > On 2014/08/04 18:44:51, bartfab wrote: > > Why? Prefs get auto-committed for you. > > If we call PowerManagementClient::RequestRestart() immediately after this > function, does pref service still have time to commit the pending writes? I saw > in some place that we call CommitPendingWrite() before RequestRestart(), so I > thought it might be necessary. During a clean browser shutdown, pending writes are committed. CommitPendingWrite() should only be necessary if you anticipate a crash. I am not sure whether RequestRestart() performs a clean shutdown or forefully reboots without giving Chrome a chance to clean up. Best check the code. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:29: // involved compoments, including settings page, signin page, and user On 2014/08/05 07:26:58, davidyu wrote: > On 2014/08/04 18:44:52, bartfab wrote: > > Nit: What is the "signin page"? > > sign-in screen? Yes, that is the canonical term :). https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:31: class ConsumerManagementService { On 2014/08/05 07:26:59, davidyu wrote: > On 2014/08/04 18:44:51, bartfab wrote: > > 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|? > > 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. Thanks for clarifying. The name makes sense then. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:48: EnrollState GetEnrollState(); On 2014/08/05 07:26:58, davidyu wrote: > On 2014/08/04 18:44:52, bartfab wrote: > > Nit 1: s/Enroll/Enrollment/g > > Nit 2: const. > > 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. Yeah, I would say we prefer non-const to mutable in that case. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:58: void SetOwner(const std::string& email, On 2014/08/05 07:26:58, davidyu wrote: > On 2014/08/04 18:44:51, bartfab wrote: > > +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. > > 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. 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 :). https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:19: const char* kAttrOwnerId = "consumer_management.owner_id"; Nit: Avoid abbreviations like "Attr." https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:29: // between compoments, including settings page, sign-in screen, and user Nit: s/between/and between/ https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:30: // notification. It also handles the owner user ID stored in boot lockbox. Nit: s/boot/the boot/ https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:36: ENROLLMENT_SUCCESS, // Success. How does "Success" differ from "enrollment is completed"? https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:6: Nit: Since the header only includes "base/callback_forward.h" now, the unit test file must include "base/callback.h". https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:25: const char* kAttrOwnerId = "consumer_management.owner_id"; Nit: Avoid abbreviations like "Attr." https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:172: // Consumer management service for checking if the enrollment is in progress. Nit: s/the // https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:173: policy::ConsumerManagementService* consumer_management_; Nit: As in consumer_management_handler.h, I think s/consumer_management/management_service/ (here and in the constructor argument) would be better. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:265: GetConsumerManagementService(); Nit: Indent four more spaces. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/consumer_management_handler.h (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.h:37: policy::ConsumerManagementService* consumer_management_; Nit: Here and in the constructor argument: ConsumerManagementHandler (this class) and ConsumerManagementService (the class passed in as a dependency) have the "consumer management" part of their name in common. Calling one of them |consumer_management| is not particularly descriptive in that context. The last part of their names is where they differ. How about s/consumer_management/management_service/? https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:334: GetConsumerManagementService(); Nit: Indent four more spaces. https://codereview.chromium.org/438493002/diff/120001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/common/pref_name... chrome/common/pref_names.cc:1905: // meaning of the value is defined in enum EnrollmentState in: Nit: s/in enum/in the enum/
https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:42: prefs->CommitPendingWrite(); On 2014/08/05 18:07:13, bartfab wrote: > On 2014/08/05 07:26:58, davidyu wrote: > > On 2014/08/04 18:44:51, bartfab wrote: > > > Why? Prefs get auto-committed for you. > > > > If we call PowerManagementClient::RequestRestart() immediately after this > > function, does pref service still have time to commit the pending writes? I > saw > > in some place that we call CommitPendingWrite() before RequestRestart(), so I > > thought it might be necessary. > > During a clean browser shutdown, pending writes are committed. > CommitPendingWrite() should only be necessary if you anticipate a crash. I am > not sure whether RequestRestart() performs a clean shutdown or forefully reboots > without giving Chrome a chance to clean up. Best check the code. RequestRestart() eventually runs "shutdown -r now" (in powerd_setuid_helper.cc), so I think it will give Chrome a chance to clean up. https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/438493002/diff/100001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:58: void SetOwner(const std::string& email, On 2014/08/05 18:07:13, bartfab wrote: > On 2014/08/05 07:26:58, davidyu wrote: > > On 2014/08/04 18:44:51, bartfab wrote: > > > +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. > > > > 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. > > 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 :). 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. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.cc:19: const char* kAttrOwnerId = "consumer_management.owner_id"; On 2014/08/05 18:07:13, bartfab wrote: > Nit: Avoid abbreviations like "Attr." Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service.h (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:29: // between compoments, including settings page, sign-in screen, and user On 2014/08/05 18:07:13, bartfab wrote: > Nit: s/between/and between/ Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:30: // notification. It also handles the owner user ID stored in boot lockbox. On 2014/08/05 18:07:13, bartfab wrote: > Nit: s/boot/the boot/ Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service.h:36: ENROLLMENT_SUCCESS, // Success. On 2014/08/05 18:07:13, bartfab wrote: > How does "Success" differ from "enrollment is completed"? Update the comment. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:6: On 2014/08/05 18:07:13, bartfab wrote: > Nit: Since the header only includes "base/callback_forward.h" now, the unit test > file must include "base/callback.h". Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/chromeos... chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:25: const char* kAttrOwnerId = "consumer_management.owner_id"; On 2014/08/05 18:07:13, bartfab wrote: > Nit: Avoid abbreviations like "Attr." Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:172: // Consumer management service for checking if the enrollment is in progress. On 2014/08/05 18:07:13, bartfab wrote: > Nit: s/the // Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:173: policy::ConsumerManagementService* consumer_management_; On 2014/08/05 18:07:13, bartfab wrote: > Nit: As in consumer_management_handler.h, I think > s/consumer_management/management_service/ (here and in the constructor argument) > would be better. Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:265: GetConsumerManagementService(); On 2014/08/05 18:07:14, bartfab wrote: > Nit: Indent four more spaces. Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/chromeos/consumer_management_handler.h (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/chromeos/consumer_management_handler.h:37: policy::ConsumerManagementService* consumer_management_; On 2014/08/05 18:07:14, bartfab wrote: > Nit: Here and in the constructor argument: ConsumerManagementHandler (this > class) and ConsumerManagementService (the class passed in as a dependency) have > the "consumer management" part of their name in common. Calling one of them > |consumer_management| is not particularly descriptive in that context. The last > part of their names is where they differ. How about > s/consumer_management/management_service/? Done. https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:334: GetConsumerManagementService(); On 2014/08/05 18:07:14, bartfab wrote: > Nit: Indent four more spaces. Done. https://codereview.chromium.org/438493002/diff/120001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/438493002/diff/120001/chrome/common/pref_name... chrome/common/pref_names.cc:1905: // meaning of the value is defined in enum EnrollmentState in: On 2014/08/05 18:07:14, bartfab wrote: > Nit: s/in enum/in the enum/ Done.
Thanks Bartosz for pointing this CL to me. For the moment, since cros uses email as |user_id|, this CL is consistent with the rest of the code. To make migration to gaiaid easier, I would suggest new code should not assume that |user_id| is an email. This CL looks good from this point of view. Eventually, I would guess that HandleCompleteLogin() will be passed the typed email and gaiaid so it can use the latter to call SetOwner(). And we'll need a "migration" step to move all existing data keyed off email and move it to gaiaid. Does that make sense?
On 2014/08/07 00:58:05, Roger Tawa (build sheriff) wrote: > Thanks Bartosz for pointing this CL to me. > > For the moment, since cros uses email as |user_id|, this CL is consistent with > the rest of the code. To make migration to gaiaid easier, I would suggest new > code should not assume that |user_id| is an email. This CL looks good from this > point of view. > > Eventually, I would guess that HandleCompleteLogin() will be passed the typed > email and gaiaid so it can use the latter to call SetOwner(). And we'll need a > "migration" step to move all existing data keyed off email and move it to > gaiaid. Does that make sense? SGTM
https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:65: policy::ConsumerManagementService* management_service); Nit: I suggested renaming |consumer_management| to |management_service| inside the |ConsumerManagementHandler| class, which inherently deals with consumer management already and where consumer managemen" is implied. This class here does not generally deal with consumer management, so the meaning of |management_service| os rather ambiguous. I think |consumer_management| would be the better name here. https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:263: policy::ConsumerManagementService* management_service = Nit: I suggested renaming |consumer_management| to |management_service| inside the |ConsumerManagementHandler| class, which inherently deals with consumer management already and where consumer managemen" is implied. This class here does not generally deal with consumer management, so the meaning of |management_service| os rather ambiguous. I think |consumer_management| would be the better name here. https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:301: policy::ConsumerManagementService* management_service = Nit: I suggested renaming |consumer_management| to |management_service| inside the |ConsumerManagementHandler| class, which inherently deals with consumer management already and where consumer managemen" is implied. This class here does not generally deal with consumer management, so the meaning of |management_service| os rather ambiguous. I think |consumer_management| would be the better name here. https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:332: policy::ConsumerManagementService* management_service = Nit: I suggested renaming |consumer_management| to |management_service| inside the |ConsumerManagementHandler| class, which inherently deals with consumer management already and where consumer managemen" is implied. This class here does not generally deal with consumer management, so the meaning of |management_service| os rather ambiguous. I think |consumer_management| would be the better name here.
https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:65: policy::ConsumerManagementService* management_service); On 2014/08/07 14:32:35, bartfab wrote: > Nit: I suggested renaming |consumer_management| to |management_service| inside > the |ConsumerManagementHandler| class, which inherently deals with consumer > management already and where consumer managemen" is implied. This class here > does not generally deal with consumer management, so the meaning of > |management_service| os rather ambiguous. I think |consumer_management| would be > the better name here. Done. https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/oobe_ui.cc (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/oobe_ui.cc:263: policy::ConsumerManagementService* management_service = On 2014/08/07 14:32:35, bartfab wrote: > Nit: I suggested renaming |consumer_management| to |management_service| inside > the |ConsumerManagementHandler| class, which inherently deals with consumer > management already and where consumer managemen" is implied. This class here > does not generally deal with consumer management, so the meaning of > |management_service| os rather ambiguous. I think |consumer_management| would be > the better name here. Done. https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc:301: policy::ConsumerManagementService* management_service = On 2014/08/07 14:32:35, bartfab wrote: > Nit: I suggested renaming |consumer_management| to |management_service| inside > the |ConsumerManagementHandler| class, which inherently deals with consumer > management already and where consumer managemen" is implied. This class here > does not generally deal with consumer management, so the meaning of > |management_service| os rather ambiguous. I think |consumer_management| would be > the better name here. Done. https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... File chrome/browser/ui/webui/options/options_ui.cc (right): https://codereview.chromium.org/438493002/diff/180001/chrome/browser/ui/webui... chrome/browser/ui/webui/options/options_ui.cc:332: policy::ConsumerManagementService* management_service = On 2014/08/07 14:32:35, bartfab wrote: > Nit: I suggested renaming |consumer_management| to |management_service| inside > the |ConsumerManagementHandler| class, which inherently deals with consumer > management already and where consumer managemen" is implied. This class here > does not generally deal with consumer management, so the meaning of > |management_service| os rather ambiguous. I think |consumer_management| would be > the better name here. Done.
lgtm
lgtm with couple UX questions. https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:308: // Show Gaia sign-in screen again, since we only allow the owner to sign How would you inform user that previous email is not an owner? https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:385: consumer_management_->SetEnrollmentState( In case of error you seem to proceed with login, should you cancel it instead? https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:105: void OnSetOwnerDone(const std::string& typed_email, nit: Please add comments for these 2 methods.
https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc (right): https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:308: // Show Gaia sign-in screen again, since we only allow the owner to sign On 2014/08/08 08:48:45, Nikita Kostylev wrote: > How would you inform user that previous email is not an owner? This should not happen unless the user explicitly changes the pre-filled email, so the current implementation does not do anything because we already have the "owner must sign in" message on the right hand side. I'm still discussing with our UI designer on this. If she thinks we need to show a message to a user, I'll do that in another CL. https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc:385: consumer_management_->SetEnrollmentState( On 2014/08/08 08:48:45, Nikita Kostylev wrote: > In case of error you seem to proceed with login, should you cancel it instead? There's not much we can do here. So we should continue logging in the user and show them an error message there. I added a comment to make the code more readable. https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... File chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h (right): https://codereview.chromium.org/438493002/diff/200001/chrome/browser/ui/webui... chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.h:105: void OnSetOwnerDone(const std::string& typed_email, On 2014/08/08 08:48:45, Nikita Kostylev wrote: > nit: Please add comments for these 2 methods. Done.
The CQ bit was checked by davidyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidyu@chromium.org/438493002/220001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by davidyu@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/davidyu@chromium.org/438493002/240001
Message was sent while issue was closed.
Change committed as 288451 |
