Minor cleanup of EnrollmentHandlerChromeOS.

Minor cleanup of EnrollmentHandlerChromeOS.

Rename private methods for consistency: Start*() for something that invokes
an async operation and Handle*() for something that gets called back by such
an operation.  That leaves On*() reserved for observers.  Also update some
comments.  Lastly, use the current state key from the parameter passed to
HandleStateKeysResult() instead of requesting it again from the broker (line
222).

EnrollmentHandlerChromeOS and EnrollmentScreen: Don't report metrics on
unreached code paths.  Generally, remove some obsolete instances of
NOTREACHED() after switches over enums:  The compiler ensures that all enum
values are covered, there's no need to complicate the code by checking the
same thing again.

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=290841

Committed: https://crrev.com/5376cbf34cac7704894ab5d6955f15063df5ebda
Cr-Commit-Position: refs/heads/master@{#293640}

[Thiemo Nagel, 2014-08-13 15:48:00]

Hi Joao,

could you please be so kind to take a look?

Many thanks!
Thiemo

[Joao da Silva, 2014-08-14 12:11:11]

While some people think that NOTREACHED() shouldn't have any logic around it, I
think that having UMA samples in those flows is useful to learn if they happened
in the field and our assumptions were wrong. See inline.

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (left):

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:128:
UMAFailure(policy::kMetricEnrollmentOtherFailed);
Why was this sample removed? Even though it's not specific it's still better
than nothing; without it we won't learn that devices out there may be going
through the NOTREACHED flows.

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:306:
UMAFailure(policy::kMetricEnrollmentOtherFailed);
Same here

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:300: NOTREACHED();
Would be nice to have a sample here too, as this is a bug

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (left):

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:399:
EnrollmentStatus::STATUS_LOCK_ERROR));
Don't remove this. If this flow enters somehow then we need to call
ReportResult, otherwise the device will just get stuck. If we report this error
then the user has something specific to send to support, and we can debug the
problem.

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

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:222:
current_state_key_ = state_keys.front();
Why? What if the logic for current_state_key() changes?

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:237: // Do
nothing.  StartRegistration() will be called again from
nit: single space after "." (I've seen other files with double spaces, but we
use single spaces consistently)

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

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/server_backed_state_keys_broker.h:27: // Brokers
server-backed FRE state keys for the device. Retrieves them from
Currently FRE is the sole user of the state keys but I'm not sure if we won't
build other things on this. I don't mind this fix for now.

[Thiemo Nagel, 2014-08-14 14:52:37]

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (left):

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:128:
UMAFailure(policy::kMetricEnrollmentOtherFailed);
It's after a switch on an enum.  The compiler will throw an error if a case has
been missed.  It will not be reached.  Anyways, I've refactored the switch to
use break instead of return.

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:306:
UMAFailure(policy::kMetricEnrollmentOtherFailed);
Same as above.

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enrollment/enrollment_screen.cc (right):

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enrollment/enrollment_screen.cc:300: NOTREACHED();
This case is treated in line 221.  I've refactored the switch to make it less
confusing.

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
File chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc (left):

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:399:
EnrollmentStatus::STATUS_LOCK_ERROR));
Control cannot get there because each of the switch cases return.  The style
guide explictly forbids code after NOTREACHED():

http://www.chromium.org/developers/coding-style#TOC-CHECK-DCHECK-and-NOTREACHED-

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

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:222:
current_state_key_ = state_keys.front();
With the current implementation, we can't be sure that current_state_key_ is
non-empty without making assumptions on StateKeysBroker behaviour.  Anyways,
you're right, my proposal is too brittle, I'm keeping the redundant call to
StateKeysBroker but check the return value.

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:237: // Do
nothing.  StartRegistration() will be called again from
I don't agree.  The sample comments in the style guide use double space:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S...

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

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/server_backed_state_keys_broker.h:27: // Brokers
server-backed FRE state keys for the device. Retrieves them from
It's just to give some context so that a casual reader can have an idea what
it's all about.

[Thiemo Nagel, 2014-08-20 11:45:38]

Hi Joao,

I think this is ready now.  Could you please take another look?

Thank you!
Thiemo

[Joao da Silva, 2014-08-20 12:32:58]

lgtm

I'd still favor the single space in the comment for consistency reasons. Since
it goes against the style guide I won't argue over it.

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

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:237: // Do
nothing.  StartRegistration() will be called again from
On 2014/08/14 14:52:37, Thiemo Nagel wrote:
> I don't agree.  The sample comments in the style guide use double space:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S...

Look around this file and other policy/ files. We use single spaces in our
comments, and many other pieces of the codebase do that too.

I see it's against the style guide, but I'd favor consistency within a file.

[Thiemo Nagel, 2014-08-20 13:26:15]

Thank you!

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

https://codereview.chromium.org/465413002/diff/1/chrome/browser/chromeos/poli...
chrome/browser/chromeos/policy/enrollment_handler_chromeos.cc:237: // Do
nothing.  StartRegistration() will be called again from
On 2014/08/20 12:32:58, Joao da Silva wrote:
> On 2014/08/14 14:52:37, Thiemo Nagel wrote:
> > I don't agree.  The sample comments in the style guide use double space:
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Punctuation,_S...
> 
> Look around this file and other policy/ files. We use single spaces in our
> comments, and many other pieces of the codebase do that too.
> 
> I see it's against the style guide, but I'd favor consistency within a file.

Done.

[Thiemo Nagel, 2014-08-20 13:26:22]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-08-20 13:27:19]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/465413002/140001

[commit-bot: I haz the power, 2014-08-20 14:25:40]

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

[commit-bot: I haz the power, 2014-08-20 14:30:10]

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

[commit-bot: I haz the power, 2014-08-20 14:30:12]

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

[Thiemo Nagel, 2014-08-20 14:37:26]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-08-20 14:38:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/tnagel@chromium.org/465413002/140001

[commit-bot: I haz the power, 2014-08-20 14:43:25]

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

[commit-bot: I haz the power, 2014-08-20 14:51:39]

Committed patchset #7 (140001) as 290841

[jennyz, 2014-08-20 16:55:34]

A revert of this CL (patchset #7) has been created in
https://codereview.chromium.org/477243003/ by jennyz@chromium.org.

The reason for reverting is: Two of the bots have been failed.
http://build.chromium.org/p/chromiumos.chromium/builders/X86%20%28chromium%29...
http://build.chromium.org/p/chromiumos.chromium/builders/AMD64%20%28chromium%...
The error message looks like related to this cl..

[Thiemo Nagel, 2014-09-06 20:31:22]

Fixed compile errors, re-landing.

[Thiemo Nagel, 2014-09-06 20:31:33]

The CQ bit was checked by tnagel@chromium.org

[commit-bot: I haz the power, 2014-09-06 20:32:42]

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

[commit-bot: I haz the power, 2014-09-06 22:23:00]

Committed patchset #8 (id:160001) as 0d8b901df85e7714335737eecdce443653dcf5e7

[commit-bot: I haz the power, 2014-09-10 03:43:32]

Patchset 8 (id:??) landed as
https://crrev.com/5376cbf34cac7704894ab5d6955f15063df5ebda
Cr-Commit-Position: refs/heads/master@{#293640}