Chrome-side lockbox bindings

Chrome to cryptohome::InstallAttrutes bindings.

BUG=chrome-os:14119
TEST=Manual.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=81794

[pastarmovj, 2011-04-13 14:57:34]

Hi this is a sneak-peak preview of the binding for the loginbox. This is the
chrome side binding and some sample code that uses it (which should be removed
from there).

Not much to change here as long as the interface doesn't change. I implemented
all the mocks too but still nobody to use them either :)

[pastarmovj, 2011-04-13 15:00:35]

http://codereview.chromium.org/6821075/diff/6001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enterprise_enrollment_view.cc (right):

http://codereview.chromium.org/6821075/diff/6001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:116: if
(chromeos::CrosLibrary::Get()->EnsureLoaded()) {
This code is here only as an example how to use that will disappear of course
:).

[pastarmovj, 2011-04-13 21:31:57]

This is much more like what the end code is going to be. I only kept a more
verbose version of the actual communication with the lockbox for testing
purposes but of course this will disappear from here before I commit.

@Chris: Will and Mattias advised me that you will be the right person to ask for
a review of this three commits touching libcros and chrome. Can you please have
a look and I don't want to be pushy but we are really on a tight schedule to get
this in before M12 freezes as you know, so I will appreciate it if you could
take a look at that as soon as you can. Thanks!

@all Please notify me if you don't consider your LGTM is needed to land this
change so that I don't loose time to wait for someone.

[Chris Masone, 2011-04-14 00:07:18]

the cryptohome library stuff lgtm, can't speak to the rest.

On 2011/04/13 21:31:57, pastarmovj wrote:
> This is much more like what the end code is going to be. I only kept a more
> verbose version of the actual communication with the lockbox for testing
> purposes but of course this will disappear from here before I commit.
> 
> @Chris: Will and Mattias advised me that you will be the right person to ask
for
> a review of this three commits touching libcros and chrome. Can you please
have
> a look and I don't want to be pushy but we are really on a tight schedule to
get
> this in before M12 freezes as you know, so I will appreciate it if you could
> take a look at that as soon as you can. Thanks!
> 
> @all Please notify me if you don't consider your LGTM is needed to land this
> change so that I don't loose time to wait for someone.

[Mattias Nissler (ping if slow), 2011-04-14 11:43:17]

I guess there's some refactoring going on... :)

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enterprise_enrollment_view.h (right):

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_view.h:44: void
WriteLockboxData();
this needs to go into EnterpriseEnrollmentScreen, which acts as the controller
for enrollment.

[kmixter1, 2011-04-14 16:52:22]

Where is the code that sets the lockbox values (email address for instance)?

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/cros...
File chrome/browser/chromeos/cros/cryptohome_library.cc (right):

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/cros...
chrome/browser/chromeos/cros/cryptohome_library.cc:382: lockbox_[name] = value;
why?  Is this just until the actual implementation lands?

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/enterprise_enrollment_view.cc (right):

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:208: if
(!cryptohome->InstallAttributesIsReady()) {
How does this look to the user?  Do they provide credentials and then the
login/authenticate seems to take a long time, do we say we're waiting for
hardware, etc?

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:219: /*
remove?

http://codereview.chromium.org/6821075/diff/5002/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:229: LOG(ERROR) <<
"### FI : " << cryptohome->InstallAttributesIsFirstInstall();
remove?

[pastarmovj, 2011-04-14 20:40:20]

Please have a look at the CL. This is what I hope to land for M12 after fixing
whatever review comments come. There are some rough edges in the user
interaction but these does not prevent usability and can be solved with later
releases too.

[kmixter1, 2011-04-15 02:06:51]

Where is the code that handles when the enterprise-owned status in nvram does
not match the status on disk?

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:193: // Lockbox is
not read yet, reschedule pulling.
ready

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_view.cc (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:124:
cryptohome->TpmCanAttemptOwnership();
indentation

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:134:
args.SetString("user", value);
A comment here may be in order: is this code enforcing once you've set the
enterprise user you can't change it?

[Will Drewry, 2011-04-15 02:38:18]

A few nits and questions, but nothing blocking from me!

http://codereview.chromium.org/6821075/diff/11002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_view.cc (right):

http://codereview.chromium.org/6821075/diff/11002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:139:
UpdateGaiaLogin(args);
Is this only used during enrollment or will enterprise.user be retrieved and
updated on each boot?

(I haven't quite pieced together that part, and being able to ensure we use the
right username for subsequent pulls is important, though fixable if we're not
that far along yet :)

http://codereview.chromium.org/6821075/diff/11002/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/6821075/diff/11002/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:217: // Own the lockbox if still
not owned. This is not critical if it fails
s/own/Finalize/gi (lock, seal, whatever :)

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cryptohome_library.cc (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cryptohome_library.cc:419: std::map<std::string,
std::string> lockbox_;
nit: if you feel like a search&replace, I ended up using install_attrs_ in
cryptohomed's code.  I don't think it's a big deal though.

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:215: << "has been
altered already!";
s/altered/finalized  (or locked :)

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.h (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:83: void
WriteLockboxData(const ClientLoginResult& result);
nit: Would it make sense to call it WriteInstallAttributesData now? :/

I don't mind that being filed as a cleanup bug after this lands either since it
is purely cosmetic.

[pastarmovj, 2011-04-15 10:05:01]

Addressed comments.

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/cro...
File chrome/browser/chromeos/cros/cryptohome_library.cc (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/cro...
chrome/browser/chromeos/cros/cryptohome_library.cc:419: std::map<std::string,
std::string> lockbox_;
On 2011/04/15 02:38:19, Will Drewry wrote:
> nit: if you feel like a search&replace, I ended up using install_attrs_ in
> cryptohomed's code.  I don't think it's a big deal though.

Done.

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:193: // Lockbox is
not read yet, reschedule pulling.
On 2011/04/15 02:06:51, kmixter1 wrote:
> ready

Done.

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:215: << "has been
altered already!";
On 2011/04/15 02:38:19, Will Drewry wrote:
> s/altered/finalized  (or locked :)

Done.

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.h (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:83: void
WriteLockboxData(const ClientLoginResult& result);
On 2011/04/15 02:38:19, Will Drewry wrote:
> nit: Would it make sense to call it WriteInstallAttributesData now? :/
> 
> I don't mind that being filed as a cleanup bug after this lands either since
it
> is purely cosmetic.

Done.

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_view.cc (right):

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:124:
cryptohome->TpmCanAttemptOwnership();
On 2011/04/15 02:06:51, kmixter1 wrote:
> indentation

Done.

http://codereview.chromium.org/6821075/diff/14001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:134:
args.SetString("user", value);
On 2011/04/15 02:06:51, kmixter1 wrote:
> A comment here may be in order: is this code enforcing once you've set the
> enterprise user you can't change it?

Done.

[pastarmovj, 2011-04-15 16:27:09]

Final cleanups and refactorings. PTAL.

[Chris Masone, 2011-04-15 16:59:25]

This doesn't LGTM...

I don't understand why we need to separate the notions of OWNERSHIP_USER and
OWNERSHIP_ENTERPRISE.  I didn't go through with a fine-toothed comb, but my
cursory inspection doesn't show anywhere that switches behavior based on this. 
We should absolutely set the appropriate values, but I don't think we need to
check them right now as we're not doing anything different anyhow.  Am I wrong?

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/login_utils.cc (right):

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/login_utils.cc:224:
cryptohome->InstallAttributesFinalize();
Putting 4 synchronous dbus calls in the way of login seems like a really bad
idea, especially if it's not critical.  Do this later, or asynchronously, or not
at all.

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ownership_service.cc (right):

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ownership_service.cc:77: status = is_owned ?
DetermineOwnerType() : OWNERSHIP_NONE;
You will wind up doing this from the file thread in some cases, which is bad and
can lead to crashes.

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ownership_service.cc:124:
SetStatus(DetermineOwnerType());
why do we need to do this all over the code?

[pastarmovj, 2011-04-15 17:49:15]

Removed the OWNERSHIP types.

[Chris Masone, 2011-04-15 17:52:29]

After the fix below, the cryptohome_library changes LGTM.

Can't speak to anything else.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ownership_service.cc (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ownership_service.cc:190: DCHECK(new_status !=
OWNERSHIP_UNKNOWN);
Please revert this check to what it was before you changed it

[Mattias Nissler (ping if slow), 2011-04-15 18:17:58]

Looks pretty good for the enrollment part, mostly style nits.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:25: // have not
domne that yet).
s/domne/done/

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:29: if
(cryptohome->TpmIsEnabled() && !cryptohome->TpmIsBeingOwned() &&
for readability ,you might want to break after the first && (your call)

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:79:
!cryptohome->InstallAttributesIsFirstInstall()) {
same here.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:82: 0 ==
value.compare("true")) {
value == "true"?

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:88:
view()->editable_user(false);
make it set_editable_user :)

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:88:
view()->editable_user(false);
Please wrap in if (view())

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:223: 
if (!view())
  return;

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:241:
kLockboxRetryIntervalMs);
early return and remove block below?

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:244: if
(cryptohome->TpmIsEnabled() && !cryptohome->TpmIsBeingOwned() &&
line break condition breaking?

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:252:
view()->ShowFatalEnrollmentError();
return;

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:257: 0 ==
value.compare("true")) {
value == "true". Might want to introduce a char constant since you're using it a
lot :)

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:272:
view()->ShowFatalEnrollmentError();
early return and remove else block?

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.h (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:86: // Starts the
Lockbox storage process. The paramter must be a copy and not a
s/paramter/parameter/. But the comment seems obsolete :)

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_view.cc (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:78: 
no need for newline.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_view.h (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.h:59: virtual bool
SetInitialUser(std::string* user) OVERRIDE;
I would rather name it set GetInitialUser.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.h:76: // Local
variables.
unnecessary.

[pastarmovj, 2011-04-15 18:44:49]

Addressed comments from Chris and Mattias. Thanks for the reviews!

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:25: // have not
domne that yet).
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> s/domne/done/

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:29: if
(cryptohome->TpmIsEnabled() && !cryptohome->TpmIsBeingOwned() &&
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> for readability ,you might want to break after the first && (your call)

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:79:
!cryptohome->InstallAttributesIsFirstInstall()) {
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> same here.

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:82: 0 ==
value.compare("true")) {
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> value == "true"?

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:88:
view()->editable_user(false);
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> Please wrap in if (view())

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:88:
view()->editable_user(false);
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> make it set_editable_user :)

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:223: 
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> if (!view())
>   return;

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:241:
kLockboxRetryIntervalMs);
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> early return and remove block below?

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:244: if
(cryptohome->TpmIsEnabled() && !cryptohome->TpmIsBeingOwned() &&
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> line break condition breaking?

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:252:
view()->ShowFatalEnrollmentError();
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> return;

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:257: 0 ==
value.compare("true")) {
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> value == "true". Might want to introduce a char constant since you're using it
a
> lot :)

Done the == but for now i don't see much need in introducing yet another name
for "true" :)

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:272:
view()->ShowFatalEnrollmentError();
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> early return and remove else block?

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.h (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.h:86: // Starts the
Lockbox storage process. The paramter must be a copy and not a
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> s/paramter/parameter/. But the comment seems obsolete :)

Removed that it is not true anymore.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_view.cc (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.cc:78: 
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> no need for newline.

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_view.h (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.h:59: virtual bool
SetInitialUser(std::string* user) OVERRIDE;
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> I would rather name it set GetInitialUser.

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_view.h:76: // Local
variables.
On 2011/04/15 18:17:58, Mattias Nissler wrote:
> unnecessary.

Done.

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/ownership_service.cc (right):

http://codereview.chromium.org/6821075/diff/25001/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/ownership_service.cc:190: DCHECK(new_status !=
OWNERSHIP_UNKNOWN);
On 2011/04/15 17:52:29, Chris Masone wrote:
> Please revert this check to what it was before you changed it

Done.

[Mattias Nissler (ping if slow), 2011-04-15 18:48:03]

LGTM

http://codereview.chromium.org/6821075/diff/16021/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/16021/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:240: // Lockbox is
not ready yet, reschedule pulling.
nit: pulling?

http://codereview.chromium.org/6821075/diff/16021/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:298: }
nit: newline

[pastarmovj, 2011-04-15 19:19:12]

Fixed nits will land upon trybot happyness.

http://codereview.chromium.org/6821075/diff/16021/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/16021/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:240: // Lockbox is
not ready yet, reschedule pulling.
On 2011/04/15 18:48:03, Mattias Nissler wrote:
> nit: pulling?

Done.

http://codereview.chromium.org/6821075/diff/16021/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:298: }
On 2011/04/15 18:48:03, Mattias Nissler wrote:
> nit: newline

Done.

[kmixter1, 2011-04-15 19:42:26]

LGTM with nits (no need to address the naming convention issue now)

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:24: // Init
lockbox if it has not been done until now (in debug build we might
+1 on Will's comment to use common terminology throughout the system. 
install_attrs vs nvram vs lockbox.  Finalize vs own.  I probably would have
picked lockbox but InstallAttrs is all over cryptohomed and dbus now, so maybe
that's the best to converge on.

In this case, the code is triggering TPM ownership which is different from
initializing the lockbox/nvram/install_attrs (other than install_attrs requires
an owned tpm to be initialized).

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:234: // Lockbox is
not ready yet, reschedule pulling.
do you mean polling here?

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:255: std::string 
value;
extra space

[pastarmovj, 2011-04-15 19:59:16]

Addressed Ken's nits.

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
File chrome/browser/chromeos/login/enterprise_enrollment_screen.cc (right):

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:24: // Init
lockbox if it has not been done until now (in debug build we might
On 2011/04/15 19:42:26, kmixter1 wrote:
> +1 on Will's comment to use common terminology throughout the system. 
> install_attrs vs nvram vs lockbox.  Finalize vs own.  I probably would have
> picked lockbox but InstallAttrs is all over cryptohomed and dbus now, so maybe
> that's the best to converge on.
> 
> In this case, the code is triggering TPM ownership which is different from
> initializing the lockbox/nvram/install_attrs (other than install_attrs
requires
> an owned tpm to be initialized).

You are right should be tpm here and i replaced lockbox with InstallAttrs
everywhere else.

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:234: // Lockbox is
not ready yet, reschedule pulling.
On 2011/04/15 19:42:26, kmixter1 wrote:
> do you mean polling here?

Has been fixed already.

http://codereview.chromium.org/6821075/diff/15003/chrome/browser/chromeos/log...
chrome/browser/chromeos/login/enterprise_enrollment_screen.cc:255: std::string 
value;
On 2011/04/15 19:42:26, kmixter1 wrote:
> extra space

Done.