Add an enrolling state for consumer management section in settings page.

Add an enrolling state for consumer management section in settings page.

When the enrollment is in progress, the button shows "Enrolling..." and is
disabled.

Also made BrowserOptionsHandler an observer of ConsumerManagementService, so
that the settings page is updated when the enrollment state is changed.

BUG=chromium:353050
TEST=unit_tests

Committed: https://crrev.com/51ffb80c42001d78b3728e9b13cac6fd57b1a6fc
Cr-Commit-Position: refs/heads/master@{#294323}

[bartfab (slow), 2014-08-20 14:40:03]

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5717: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_ENROLL_BUTTON" desc="The text of the
consumer management button in the settings page for enrollment.">
What is "the settings page for enrollment"? How about something like this:

"The text of the consumer management button in the settings page when the device
is not enrolled."

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5720: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_ENROLLING_BUTTON" desc="The text of the
consumer management button in the settings page when the enrollment is in
progress.">
Nit: s/the enrollment/enrollment/

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5723: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_UNENROLL_BUTTON" desc="The text of the
consumer management button in the settings page for unenrollment.">
How about something like this:

"The text of the consumer management button in the settings page when the device
is enrolled."

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5726: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_UNENROLLING_BUTTON" desc="The text of the
consumer management button in the settings page when the unenrollment is in
progress.">
Nit: s/the unenrollment/unenrollment/

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_service.cc (right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.cc:124:
NotifyEnrollmentStateChanged();
Nit: Since this is the only caller of NotifyEnrollmentStateChanged(), you might
as well fold the two into one method.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.cc:200: return
policy_data && policy_data->management_mode() ==
What if policy data has not been retrieved yet? The device may still be
enrolled. It looks like this method should take a callback that it invokes once
it knows for sure whether the device is enrolled or not.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.cc:210: //
TODO(davidyu): Implement this.
Nit: Can you add a bug number?

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:82: // Called when
the enrollment state is changed.
Nit: s/is changed/changes/

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:84:
ConsumerManagementService* service) = 0;
Why not just pass the state here instead of the entire |service|?

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:122: // Returns
true if the enrollment is in progress.
Nit: s/the/consumer/

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:125: // Returns
true if the unenrollment is in progress.
Nit: s/the/consumer/

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc
(right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:245:
device_policy_.policy_data().set_management_mode(mode);
Nit: #include "chrome/browser/chromeos/policy/device_policy_builder.h"

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:252:
scoped_ptr<ConsumerManagementService> service_;
Nit: #include "base/memory/scoped_ptr.h"

https://codereview.chromium.org/493613002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:48: if
(state == 'enrolled') {
Nit: You could simplify this entire method to:
var enrolled = state == 'enrolled';
$('enroll-content').hidden = !enrolled;
$('unenroll-content').hidden = enrolled;

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:110: #include
"chrome/browser/chromeos/policy/consumer_management_service.h"
Nit: Already included by the header file.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:773:
g_browser_process->platform_part()->browser_policy_connector_chromeos()->
Nit: #include "chrome/browser/browser_process_platform_part.h"

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1491: if
(service->IsEnrolled()) {
Nit: No need for curly braces.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1492: state =
service->IsUnenrolling() ? "unenrolling" : "enrolled";
I would not be surprised if this nested conditional turned up elsewhere as well,
because other methods also want to distinguish the four states
unenrolled/enrolled/unenrolling/enrolling. How about making an enum for these
and having OnConsumerManagementEnrollmentStateChanged() return this enum instead
of IsEnrolled(), IsUnenrolling() and IsEnrolling()?

[davidyu, 2014-08-21 09:40:30]

I renamed the old enum to EnrollmentStage and called the new one Status.

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5717: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_ENROLL_BUTTON" desc="The text of the
consumer management button in the settings page for enrollment.">
On 2014/08/20 14:40:02, bartfab wrote:
> What is "the settings page for enrollment"? How about something like this:
> 
> "The text of the consumer management button in the settings page when the
device
> is not enrolled."

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5720: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_ENROLLING_BUTTON" desc="The text of the
consumer management button in the settings page when the enrollment is in
progress.">
On 2014/08/20 14:40:01, bartfab wrote:
> Nit: s/the enrollment/enrollment/

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5723: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_UNENROLL_BUTTON" desc="The text of the
consumer management button in the settings page for unenrollment.">
On 2014/08/20 14:40:02, bartfab wrote:
> How about something like this:
> 
> "The text of the consumer management button in the settings page when the
device
> is enrolled."

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/app/chromeos_strings....
chrome/app/chromeos_strings.grdp:5726: <message
name="IDS_OPTIONS_CONSUMER_MANAGEMENT_UNENROLLING_BUTTON" desc="The text of the
consumer management button in the settings page when the unenrollment is in
progress.">
On 2014/08/20 14:40:01, bartfab wrote:
> Nit: s/the unenrollment/unenrollment/

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_service.cc (right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.cc:124:
NotifyEnrollmentStateChanged();
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: Since this is the only caller of NotifyEnrollmentStateChanged(), you
might
> as well fold the two into one method.

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.cc:200: return
policy_data && policy_data->management_mode() ==
On 2014/08/20 14:40:02, bartfab wrote:
> What if policy data has not been retrieved yet? The device may still be
> enrolled. It looks like this method should take a callback that it invokes
once
> it knows for sure whether the device is enrolled or not.

Added a unknown state which is returned when policy data is not ready. Observers
will be notified when policy data is ready.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.cc:210: //
TODO(davidyu): Implement this.
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: Can you add a bug number?

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:82: // Called when
the enrollment state is changed.
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: s/is changed/changes/

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:84:
ConsumerManagementService* service) = 0;
On 2014/08/20 14:40:02, bartfab wrote:
> Why not just pass the state here instead of the entire |service|?

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:122: // Returns
true if the enrollment is in progress.
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: s/the/consumer/

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service.h:125: // Returns
true if the unenrollment is in progress.
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: s/the/consumer/

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc
(right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:245:
device_policy_.policy_data().set_management_mode(mode);
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: #include "chrome/browser/chromeos/policy/device_policy_builder.h"

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:252:
scoped_ptr<ConsumerManagementService> service_;
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: #include "base/memory/scoped_ptr.h"

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/resources/opt...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/resources/opt...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:48: if
(state == 'enrolled') {
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: You could simplify this entire method to:
> var enrolled = state == 'enrolled';
> $('enroll-content').hidden = !enrolled;
> $('unenroll-content').hidden = enrolled;

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:110: #include
"chrome/browser/chromeos/policy/consumer_management_service.h"
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: Already included by the header file.

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:773:
g_browser_process->platform_part()->browser_policy_connector_chromeos()->
On 2014/08/20 14:40:02, bartfab wrote:
> Nit: #include "chrome/browser/browser_process_platform_part.h"

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1491: if
(service->IsEnrolled()) {
On 2014/08/20 14:40:03, bartfab wrote:
> Nit: No need for curly braces.

Done.

https://codereview.chromium.org/493613002/diff/1/chrome/browser/ui/webui/opti...
chrome/browser/ui/webui/options/browser_options_handler.cc:1492: state =
service->IsUnenrolling() ? "unenrolling" : "enrolled";
On 2014/08/20 14:40:02, bartfab wrote:
> I would not be surprised if this nested conditional turned up elsewhere as
well,
> because other methods also want to distinguish the four states
> unenrolled/enrolled/unenrolling/enrolling. How about making an enum for these
> and having OnConsumerManagementEnrollmentStateChanged() return this enum
instead
> of IsEnrolled(), IsUnenrolling() and IsEnrolling()?

Done.

[bartfab (slow), 2014-08-21 11:39:36]

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.cc (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:11: #include
"base/macros.h"
Nit: Already included by the header file.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:65: // The
relative path to the settings page URL for opening the consumer
How about:

The path to the consumer management enrollment/unenrollment confirmation
overlay, relative to the settings page URL.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:99: // A NULL
value may be passed in the test.
Nit: s/the test/tests/

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:144: } else {
Nit: The style guide forbids else after a return in a conditional. Just remove
the else and unindent the rest of the method.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:148: else
Nit: The style guide forbids else after a return in a conditional.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:26: class
ProfileOAuth2TokenService;
Nit: No longer used.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:62: // Indicating
if the device is enrolled, or if enrollment or unenrollment is
Nit: Make this an actual sentence. As it stands, it has no subject.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:238:
EnrollmentStage stage, Profile* profile);
Nit: One argument per line in declarations and definitions.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:246: // Returns the
sign-in authenticated account ID of the profile.
Nit 1: "sign-in authenticated account ID" is really hard to parse. How about
something like: Returns the account ID signed in to |profile|.

Nit 2: This could be a helper method in an anonymous namespace.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc
(right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:10:
#include "base/memory/scoped_ptr.h"
Actually, I missed it that the header already includes this. :(

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1980: * @param {string}
status consumer management service status string.
Nit: s/consumer/Consumer/

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:44: *
@param {string} status consumer management service status string
Nit 1: s/consumer/Consumer/
Nit 2: Add full stop.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1471:
base::StringValue(policy::ConsumerManagementService::GetStatusString(
Nit: How about we remove the argument from GetStatusString(), making it return
the current status as a string? You could then remove the |status| argument from
OnConsumerManagementStatusChanged() and have this method call
g_browser_process->platform_part()->browser_policy_connector_chromeos()->GetStatusString()
instead.

https://codereview.chromium.org/493613002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1943: // meaning of the value is defined in the enum
ConsumerEnrollmentStage in:
Nit: s/ConsumerEnrollmentStage/EnrollmentStage/

[bartfab (slow), 2014-08-21 11:54:27]

Sorry, I reviewed the CLs in the wrong order today and put some comments here
that would have belonged to 468873002.

[davidyu, 2014-08-22 05:14:13]

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.cc (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:11: #include
"base/macros.h"
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: Already included by the header file.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:65: // The
relative path to the settings page URL for opening the consumer
On 2014/08/21 11:39:35, bartfab wrote:
> How about:
> 
> The path to the consumer management enrollment/unenrollment confirmation
> overlay, relative to the settings page URL.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:99: // A NULL
value may be passed in the test.
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: s/the test/tests/

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:144: } else {
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: The style guide forbids else after a return in a conditional. Just remove
> the else and unindent the rest of the method.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:148: else
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: The style guide forbids else after a return in a conditional.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:26: class
ProfileOAuth2TokenService;
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: No longer used.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:62: // Indicating
if the device is enrolled, or if enrollment or unenrollment is
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: Make this an actual sentence. As it stands, it has no subject.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:238:
EnrollmentStage stage, Profile* profile);
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: One argument per line in declarations and definitions.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:246: // Returns the
sign-in authenticated account ID of the profile.
On 2014/08/21 11:39:35, bartfab wrote:
> Nit 1: "sign-in authenticated account ID" is really hard to parse. How about
> something like: Returns the account ID signed in to |profile|.
> 
> Nit 2: This could be a helper method in an anonymous namespace.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service_unittest.cc
(right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service_unittest.cc:10:
#include "base/memory/scoped_ptr.h"
On 2014/08/21 11:39:35, bartfab wrote:
> Actually, I missed it that the header already includes this. :(

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1980: * @param {string}
status consumer management service status string.
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: s/consumer/Consumer/

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/resources...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:44: *
@param {string} status consumer management service status string
On 2014/08/21 11:39:35, bartfab wrote:
> Nit 1: s/consumer/Consumer/
> Nit 2: Add full stop.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/options/browser_options_handler.cc (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/options/browser_options_handler.cc:1471:
base::StringValue(policy::ConsumerManagementService::GetStatusString(
On 2014/08/21 11:39:35, bartfab wrote:
> Nit: How about we remove the argument from GetStatusString(), making it return
> the current status as a string? You could then remove the |status| argument
from
> OnConsumerManagementStatusChanged() and have this method call
>
g_browser_process->platform_part()->browser_policy_connector_chromeos()->GetStatusString()
> instead.

Done.

https://codereview.chromium.org/493613002/diff/20001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/493613002/diff/20001/chrome/common/pref_names...
chrome/common/pref_names.cc:1943: // meaning of the value is defined in the enum
ConsumerEnrollmentStage in:
On 2014/08/21 11:39:36, bartfab wrote:
> Nit: s/ConsumerEnrollmentStage/EnrollmentStage/

Done.

[bartfab (slow), 2014-08-25 13:00:46]

lgtm

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.cc (right):

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:80: : id_(id),
button_click_callback_(button_click_callback) {
As discussed in another code review: The style guide forbids inlining virtual
methods and the policy code follows this religiously. Please make the
constructor, destructor and virtual methods non-inline.

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:124: 
Nit: Remove extraneous blank line.

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:18: #include
"chrome/browser/notifications/notification_delegate.h"
Nit: This is now only used in the implementation file, not in the header.

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:61: // The status
Indicates if the device is enrolled, or if enrollment or
Nit: s/Indicates/indicates/

[davidyu, 2014-08-28 09:21:15]

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.cc (right):

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:80: : id_(id),
button_click_callback_(button_click_callback) {
On 2014/08/25 13:00:45, bartfab wrote:
> As discussed in another code review: The style guide forbids inlining virtual
> methods and the policy code follows this religiously. Please make the
> constructor, destructor and virtual methods non-inline.

Done.

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.cc:124: 
On 2014/08/25 13:00:45, bartfab wrote:
> Nit: Remove extraneous blank line.

Done.

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:18: #include
"chrome/browser/notifications/notification_delegate.h"
On 2014/08/25 13:00:45, bartfab wrote:
> Nit: This is now only used in the implementation file, not in the header.

Done.

https://codereview.chromium.org/493613002/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:61: // The status
Indicates if the device is enrolled, or if enrollment or
On 2014/08/25 13:00:46, bartfab wrote:
> Nit: s/Indicates/indicates/

Done.

[davidyu, 2014-08-28 13:01:55]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-08-28 13:03:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidyu@chromium.org/493613002/60001

[commit-bot: I haz the power, 2014-08-28 13:57:28]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[commit-bot: I haz the power, 2014-08-28 14:00:49]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-28 14:00:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[davidyu, 2014-08-29 02:04:02]

davidyu@chromium.org changed reviewers:
+ antrim@chromium.org, stevenjb@chromium.org

[davidyu, 2014-08-29 02:04:02]

Hi Steven,

Would you please review the following files?

    chrome/browser/resources/options/browser_options.html
    chrome/browser/resources/options/browser_options.js
    chrome/browser/ui/webui/options/browser_options_handler.cc
    chrome/browser/ui/webui/options/browser_options_handler.h
    chrome/browser/ui/webui/options/chromeos/consumer_management_handler.cc
    chrome/browser/resources/options/chromeos/consumer_management_overlay.html
    chrome/browser/resources/options/chromeos/consumer_management_overlay.js

Hi Denis,

Would you please review the following files?

    chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc
    chrome/browser/ui/webui/chromeos/login/signin_screen_handler.cc

Thanks!

[Denis Kuznetsov (DE-MUC), 2014-09-02 14:00:44]

chrome/browser/ui/webui/chromeos/* 
lgtm

[Denis Kuznetsov (DE-MUC), 2014-09-02 14:00:44]

chrome/browser/ui/webui/chromeos/* 
lgtm

[stevenjb, 2014-09-02 17:06:39]

stevenjb@chromium.org changed reviewers:
+ dbeam@chromium.org

[stevenjb, 2014-09-02 17:06:40]

+dbeam@ - Please take a quick look at browser_options.js

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:116:
chromeos::DeviceSettingsService* device_settings_service);
nit: We should at least comment that device_settings_service (and client for
that matter) must be guaranteed to outlive this service.

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1982: if (status ==
'STATUS_UNKNOWN') {
I think we may normally use CamelCase for states like this. +dbeam@

[davidyu, 2014-09-03 06:29:18]

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/policy/consumer_management_service.h (right):

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/policy/consumer_management_service.h:116:
chromeos::DeviceSettingsService* device_settings_service);
On 2014/09/02 17:06:39, stevenjb wrote:
> nit: We should at least comment that device_settings_service (and client for
> that matter) must be guaranteed to outlive this service.

Done.

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/resources...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/60001/chrome/browser/resources...
chrome/browser/resources/options/browser_options.js:1982: if (status ==
'STATUS_UNKNOWN') {
On 2014/09/02 17:06:40, stevenjb wrote:
> I think we may normally use CamelCase for states like this. +dbeam@

Done.

[Dan Beam, 2014-09-04 23:12:37]

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1978: * @param {string}
status Consumer management service status string.
can this be an @enum {string} instead?

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1987: button.hidden = false;
var strId;

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1990:
loadTimeData.getString('consumerManagementEnrollButton');
strId = 'consumerManagementEnrollButton';

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:2006: }
button.textContent = loadTimeData.getString(strId);

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:44: *
@param {string} status Consumer management service status string.
can this be an enum?

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:47: //
Status should only be enrolled, or unenrolled.
s/,//

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:47: //
Status should only be enrolled, or unenrolled.
assert(status == 'StatusEnrolled' || status == 'StatusUnenrolled');

[davidyu, 2014-09-05 06:14:11]

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1978: * @param {string}
status Consumer management service status string.
On 2014/09/04 23:12:37, Dan Beam wrote:
> can this be an @enum {string} instead?

Done.

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1987: button.hidden = false;
On 2014/09/04 23:12:37, Dan Beam wrote:
> var strId;

Done.

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1990:
loadTimeData.getString('consumerManagementEnrollButton');
On 2014/09/04 23:12:37, Dan Beam wrote:
> strId = 'consumerManagementEnrollButton';

Done.

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:2006: }
On 2014/09/04 23:12:37, Dan Beam wrote:
> button.textContent = loadTimeData.getString(strId);

Done.

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:44: *
@param {string} status Consumer management service status string.
On 2014/09/04 23:12:37, Dan Beam wrote:
> can this be an enum?

Done.

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:47: //
Status should only be enrolled, or unenrolled.
On 2014/09/04 23:12:37, Dan Beam wrote:
> assert(status == 'StatusEnrolled' || status == 'StatusUnenrolled');

Done.

https://codereview.chromium.org/493613002/diff/120001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:47: //
Status should only be enrolled, or unenrolled.
On 2014/09/04 23:12:37, Dan Beam wrote:
> s/,//

Done.

[Dan Beam, 2014-09-05 19:26:49]

lgtm

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1975:
BrowserOptions.setConsumerManagementStatus = function(status) {
nit: consider a switch() (which works on strings in JS)

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:62:
status == this.Status.STATUS_UNENROLLED);
nit:

  assert(status == this.Status.STATUS_ENROLLED ||
         status == this.Status.STATUS_UNENROLLED);

https://codereview.chromium.org/493613002/diff/140001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/493613002/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.cc:1928: "consumer_management.enrollment_state";
so what happens to this pref?  is it migrated or deleted?

[davidyu, 2014-09-09 03:35:00]

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/browser_options.js (right):

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/browser_options.js:1975:
BrowserOptions.setConsumerManagementStatus = function(status) {
On 2014/09/05 19:26:49, Dan Beam wrote:
> nit: consider a switch() (which works on strings in JS)

Done.

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
File chrome/browser/resources/options/chromeos/consumer_management_overlay.js
(right):

https://codereview.chromium.org/493613002/diff/140001/chrome/browser/resource...
chrome/browser/resources/options/chromeos/consumer_management_overlay.js:62:
status == this.Status.STATUS_UNENROLLED);
On 2014/09/05 19:26:49, Dan Beam wrote:
> nit:
> 
>   assert(status == this.Status.STATUS_ENROLLED ||
>          status == this.Status.STATUS_UNENROLLED);

Done.

https://codereview.chromium.org/493613002/diff/140001/chrome/common/pref_name...
File chrome/common/pref_names.cc (left):

https://codereview.chromium.org/493613002/diff/140001/chrome/common/pref_name...
chrome/common/pref_names.cc:1928: "consumer_management.enrollment_state";
On 2014/09/05 19:26:49, Dan Beam wrote:
> so what happens to this pref?  is it migrated or deleted?

I renamed it. This part is not launched yet, so it should be alright.

[davidyu, 2014-09-09 03:35:08]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-09-09 03:41:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidyu@chromium.org/493613002/160001

[commit-bot: I haz the power, 2014-09-09 05:32:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 05:32:31]

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_...)

[davidyu, 2014-09-09 05:34:26]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-09-09 05:37:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidyu@chromium.org/493613002/160001

[commit-bot: I haz the power, 2014-09-09 06:55:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 06:55:45]

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_...)

[davidyu, 2014-09-09 07:07:32]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-09-09 07:12:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidyu@chromium.org/493613002/160001

[commit-bot: I haz the power, 2014-09-09 08:31:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-09 08:31:25]

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_...)

[davidyu, 2014-09-10 02:36:42]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-09-10 02:41:09]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/davidyu@chromium.org/493613002/160001

[commit-bot: I haz the power, 2014-09-10 04:09:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-10 04:09:49]

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_...)

[davidyu, 2014-09-11 03:56:15]

The CQ bit was checked by davidyu@chromium.org

[commit-bot: I haz the power, 2014-09-11 03:56:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/493613002/180001

[commit-bot: I haz the power, 2014-09-11 05:33:45]

Committed patchset #10 (id:180001) as 10e5c642216e13707b0a616af3f4e96cca0ded9f

[commit-bot: I haz the power, 2014-09-11 05:40:11]

Patchset 10 (id:??) landed as
https://crrev.com/51ffb80c42001d78b3728e9b13cac6fd57b1a6fc
Cr-Commit-Position: refs/heads/master@{#294323}