Added the AutoEnrollmentClient and unit tests for it.

Added the AutoEnrollmentClient and unit tests for it.

BUG=chromium-os:23063
TEST=unit_tests pass on chromeos


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

[Joao da Silva, 2011-12-08 20:06:32]

Please review, thanks!

[Mattias Nissler (ping if slow), 2011-12-09 15:40:49]

a couple of comments.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
File chrome/browser/policy/auto_enrollment_client.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:137:
DCHECK(!serial_number_hash_.empty());
These should probably be CHECKs or if (!cond) { NOTREACHED(); return; }

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:144: for (int i = 0; 8*i <
power; ++i) {
space around *

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:148: remainder = remainder &
(((uint64) 1 << power) - 1);
instead of the cast (which shouldn't be C-style), just write 1ULL

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:210: for (int i = 0; i <
hashes.size(); ++i) {
Does RepeatedPtrField have iterators that work with std::binary_search? If so,
we should use that.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
File chrome/browser/policy/auto_enrollment_client.h (right):

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.h:21: // Interacts with the
DMServer and determines whether this machine should
nit: I prefer "device management service" over "DMServer" in the Chromium
codebase, since we're not necessarily tied to a single server-side
implementation.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.h:31: virtual void
OnAutoEnrollmentComplete(AutoEnrollmentClient* client) = 0;
Any reason you decided for a full-blown delegate instead of passing in a simple
callback?

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.h:102: // Used to communicate with
the DMServer.
nit: DMServer vs. device management service

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:48: class
AutoEnrollmentClientTest : public testing::Test {
I think the test class itself usually doesn't go into the anonymous namespace.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:49: public:
can be protected.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:62: .Times(AnyNumber())
indentation.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:92:
base::RandBytesAsString(crypto::kSHA256Length));
Might this cause indeterminism? Are we sure the random seed gets set to a
defined value before running each test? If not, please do so. Or just use a
fixed list of hashes from static data. In addition, there is the negligible
chance that you generate kSerialHash here accidentally, so this test is flaky by
definition :)

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:115: void
OnProcessAutoEnrollmentRequest(
Maybe call this CaptureRequest()?

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:150:
TEST_F(AutoEnrollmentClientTest, AskForMoreThenFail) {
Could use a comment that the expectations need to be installed in reverse order.
Actually, using a googlemock sequence would probably easier to understand.

http://codereview.chromium.org/8872032/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:1208: // auto-enrollment client. E.g. "4" means
the modulus will be 2^4 = 16.
Might be useful to mention "enterprise" somewhere in the sentence so people know
where this belongs to.

[Joao da Silva, 2011-12-09 17:26:06]

Thanks for the comments! Please have another look.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
File chrome/browser/policy/auto_enrollment_client.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:137:
DCHECK(!serial_number_hash_.empty());
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> These should probably be CHECKs or if (!cond) { NOTREACHED(); return; }

Done. Using an "if", since a CHECK would kill a production image. I'd rather
just let auto-enrollment fail in that case :-)

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:144: for (int i = 0; 8*i <
power; ++i) {
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> space around *

Done.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:148: remainder = remainder &
(((uint64) 1 << power) - 1);
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> instead of the cast (which shouldn't be C-style), just write 1ULL

Done.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:210: for (int i = 0; i <
hashes.size(); ++i) {
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> Does RepeatedPtrField have iterators that work with std::binary_search? If so,
> we should use that.

Yes, it has iterators that should work with binary_search (since they have the
random access stuff). I'm not sure |hashes| is always sorted though. I'll check
with Bin.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
File chrome/browser/policy/auto_enrollment_client.h (right):

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.h:21: // Interacts with the
DMServer and determines whether this machine should
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> nit: I prefer "device management service" over "DMServer" in the Chromium
> codebase, since we're not necessarily tied to a single server-side
> implementation.

Done (here and elsewhere too).

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.h:31: virtual void
OnAutoEnrollmentComplete(AutoEnrollmentClient* client) = 0;
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> Any reason you decided for a full-blown delegate instead of passing in a
simple
> callback?

No particular reason. Changed to use a callback since it's a single call.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.h:102: // Used to communicate with
the DMServer.
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> nit: DMServer vs. device management service

Done.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:48: class
AutoEnrollmentClientTest : public testing::Test {
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> I think the test class itself usually doesn't go into the anonymous namespace.

I usually do this (see url_blacklist_manager_unittest, login_utils_browsertest).
It was first suggested by Will:

http://codereview.chromium.org/7747018/diff/3001/chrome/browser/policy/host_b...

I think it makes sense since this code isn't meant to be used anywhere else.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:49: public:
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> can be protected.

Done.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:62: .Times(AnyNumber())
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> indentation.

Done.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:92:
base::RandBytesAsString(crypto::kSHA256Length));
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> Might this cause indeterminism? Are we sure the random seed gets set to a
> defined value before running each test? If not, please do so. Or just use a
> fixed list of hashes from static data. In addition, there is the negligible
> chance that you generate kSerialHash here accidentally, so this test is flaky
by
> definition :)

Done! :-)

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:115: void
OnProcessAutoEnrollmentRequest(
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> Maybe call this CaptureRequest()?

Done.

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client_unittest.cc:150:
TEST_F(AutoEnrollmentClientTest, AskForMoreThenFail) {
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> Could use a comment that the expectations need to be installed in reverse
order.
> Actually, using a googlemock sequence would probably easier to understand.

Indeed, InSequence is cooler. Also removed the "RetiresOnSaturation" from above,
which isn't necessary anymore.

http://codereview.chromium.org/8872032/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:1208: // auto-enrollment client. E.g. "4" means
the modulus will be 2^4 = 16.
On 2011/12/09 15:40:52, Mattias Nissler wrote:
> Might be useful to mention "enterprise" somewhere in the sentence so people
know
> where this belongs to.

Done.

[Joao da Silva, 2011-12-10 13:17:17]

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
File chrome/browser/policy/auto_enrollment_client.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/browser/policy/auto_enro...
chrome/browser/policy/auto_enrollment_client.cc:210: for (int i = 0; i <
hashes.size(); ++i) {
On 2011/12/09 17:26:06, Joao da Silva wrote:
> On 2011/12/09 15:40:52, Mattias Nissler wrote:
> > Does RepeatedPtrField have iterators that work with std::binary_search? If
so,
> > we should use that.
> 
> Yes, it has iterators that should work with binary_search (since they have the
> random access stuff). I'm not sure |hashes| is always sorted though. I'll
check
> with Bin.

Update from Bin: this data is not sorted

[Mattias Nissler (ping if slow), 2011-12-12 10:20:31]

LGTM with nits.

http://codereview.chromium.org/8872032/diff/1/chrome/common/chrome_switches.cc
File chrome/common/chrome_switches.cc (right):

http://codereview.chromium.org/8872032/diff/1/chrome/common/chrome_switches.c...
chrome/common/chrome_switches.cc:1208: // auto-enrollment client. E.g. "4" means
the modulus will be 2^4 = 16.
On 2011/12/09 17:26:06, Joao da Silva wrote:
> On 2011/12/09 15:40:52, Mattias Nissler wrote:
> > Might be useful to mention "enterprise" somewhere in the sentence so people
> know
> > where this belongs to.
> 
> Done.

I intended to say that mentioning in the comment is good enough, but having it
in the constant identifier is also cool with me.

http://codereview.chromium.org/8872032/diff/8001/chrome/browser/policy/auto_e...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

http://codereview.chromium.org/8872032/diff/8001/chrome/browser/policy/auto_e...
chrome/browser/policy/auto_enrollment_client_unittest.cc:75:
&AutoEnrollmentClientTest::CaptureRequest),
nit: seems like there is more line breaking than required going on here  now.

http://codereview.chromium.org/8872032/diff/8001/chrome/browser/policy/auto_e...
chrome/browser/policy/auto_enrollment_client_unittest.cc:99:
&AutoEnrollmentClientTest::CaptureRequest),
ditto.

[Joao da Silva, 2011-12-12 11:54:13]

http://codereview.chromium.org/8872032/diff/8001/chrome/browser/policy/auto_e...
File chrome/browser/policy/auto_enrollment_client_unittest.cc (right):

http://codereview.chromium.org/8872032/diff/8001/chrome/browser/policy/auto_e...
chrome/browser/policy/auto_enrollment_client_unittest.cc:75:
&AutoEnrollmentClientTest::CaptureRequest),
On 2011/12/12 10:20:31, Mattias Nissler wrote:
> nit: seems like there is more line breaking than required going on here  now.

Done.

http://codereview.chromium.org/8872032/diff/8001/chrome/browser/policy/auto_e...
chrome/browser/policy/auto_enrollment_client_unittest.cc:99:
&AutoEnrollmentClientTest::CaptureRequest),
On 2011/12/12 10:20:31, Mattias Nissler wrote:
> ditto.

Done.

[commit-bot: I haz the power, 2011-12-12 11:54:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8872032/17001

[commit-bot: I haz the power, 2011-12-12 13:36:44]

Try job failure for 8872032-17001 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "browser_tests, ui_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-12 14:06:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/8872032/17001

[commit-bot: I haz the power, 2011-12-12 15:12:23]

Change committed as 114016