Implement device token fetcher

Implement device token fetcher 

First step in mechanism for fetching cloud-based policy for CrOS.

BUG=none
TEST=DeviceTokenFetcher*

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

[danno, 2010-10-26 09:50:55]

Please review. Zelidrag: proto buffer build and usage, chron token service
additions and usage, Mattias device management backend API + token fetcher.

Thanks!

[Paweł Hajdan Jr., 2010-10-26 09:56:38]

Drive-by with test comments. Please let me take another look before committing.

http://codereview.chromium.org/4121003/diff/10002/12006
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4121003/diff/10002/12006#newcode23
chrome/browser/policy/device_token_fetcher_unittest.cc:23: ScopedTempDir
temp_user_data_dir_;
nit: Members should be listed last, after all methods.

http://codereview.chromium.org/4121003/diff/10002/12006#newcode36
chrome/browser/policy/device_token_fetcher_unittest.cc:36:
ASSERT_TRUE(test_launcher_utils::OverrideUserDataDir(
This will carry state over to the next test. That's bad. What refers to
USER_DATA_DIR here and why? Can we mock it? If not, how about using the
browser_tests launcher which will override the directory for you and provide
better test isolation?

[danno, 2010-10-26 11:30:14]

Paweł: take a look at the code that I uploaded. I think it takes care of your
concerns.

[Paweł Hajdan Jr., 2010-10-26 11:44:35]

Yes, that addresses the main concern. Just a few minor comments to make it even
better.

http://codereview.chromium.org/4121003/diff/42001/7033
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4121003/diff/42001/7033#newcode52
chrome/browser/policy/device_token_fetcher_unittest.cc:52:
path_provider_->SetPath(temp_user_data_dir_.path());
Would it be possible to do that at mock PathProvider's construction time? Maybe
it can just own the ScopedTempDir... Generally when there is a setter, there is
always a temptation to use it twice for some tests etc which makes them more
complicated.

http://codereview.chromium.org/4121003/diff/42001/7033#newcode73
chrome/browser/policy/device_token_fetcher_unittest.cc:73: ScopedTempDir
temp_user_data_dir_;
Do all of those need to be protected and not private?

http://codereview.chromium.org/4121003/diff/42001/7033#newcode83
chrome/browser/policy/device_token_fetcher_unittest.cc:83: private:
nit: This is already in the private section, the line is redundant.

[danno, 2010-10-26 13:15:27]

Hi Pawel, how about this? :-)

http://codereview.chromium.org/4121003/diff/42001/7033
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4121003/diff/42001/7033#newcode52
chrome/browser/policy/device_token_fetcher_unittest.cc:52:
path_provider_->SetPath(temp_user_data_dir_.path());
On 2010/10/26 11:44:35, Paweł Hajdan Jr. wrote:
> Would it be possible to do that at mock PathProvider's construction time?
Maybe
> it can just own the ScopedTempDir... Generally when there is a setter, there
is
> always a temptation to use it twice for some tests etc which makes them more
> complicated.

Done.

http://codereview.chromium.org/4121003/diff/42001/7033#newcode73
chrome/browser/policy/device_token_fetcher_unittest.cc:73: ScopedTempDir
temp_user_data_dir_;
On 2010/10/26 11:44:35, Paweł Hajdan Jr. wrote:
> Do all of those need to be protected and not private?

Done.

http://codereview.chromium.org/4121003/diff/42001/7033#newcode83
chrome/browser/policy/device_token_fetcher_unittest.cc:83: private:
On 2010/10/26 11:44:35, Paweł Hajdan Jr. wrote:
> nit: This is already in the private section, the line is redundant.

Done.

[Mattias Nissler (ping if slow), 2010-10-26 13:20:05]

A couple of comments. I didn't look at the proto stuff in the gyp files.

http://codereview.chromium.org/4121003/diff/42001/7029
File chrome/browser/net/gaia/token_service.h (right):

http://codereview.chromium.org/4121003/diff/42001/7029#newcode1
chrome/browser/net/gaia/token_service.h:1: 
Remove empty line.

http://codereview.chromium.org/4121003/diff/42001/7030
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/4121003/diff/42001/7030#newcode19
chrome/browser/policy/device_management_backend.h:19: class
DeviceManagementBackend : NonThreadSafe {
How about a class comment that explain what this is here for?

http://codereview.chromium.org/4121003/diff/42001/7031
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4121003/diff/42001/7031#newcode14
chrome/browser/policy/device_token_fetcher.cc:14: #include
"chrome/common/net/gaia/gaia_constants.h"
alphabetize

http://codereview.chromium.org/4121003/diff/42001/7031#newcode16
chrome/browser/policy/device_token_fetcher.cc:16: #include
"chrome/browser/policy/mock_device_management_backend.h"
alphabetize

http://codereview.chromium.org/4121003/diff/42001/7031#newcode126
chrome/browser/policy/device_token_fetcher.cc:126: std::string
DeviceTokenFetcher::device_token() {
Shouldn't we rename this to GetDeviceToken()? Blocking makes this a non-trivial
getter IMHO.

http://codereview.chromium.org/4121003/diff/42001/7031#newcode147
chrome/browser/policy/device_token_fetcher.cc:147: *path =
device_token_dir.Append(FILE_PATH_LITERAL("DeviceManagementToken"));
make this a constant?

http://codereview.chromium.org/4121003/diff/42001/7032
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/4121003/diff/42001/7032#newcode26
chrome/browser/policy/device_token_fetcher.h:26: class
StoredDeviceTokenPathProvider {
What's wrong with just passing in the actual path in the DeviceTokenFetcher
ctor?

http://codereview.chromium.org/4121003/diff/42001/7032#newcode85
chrome/browser/policy/device_token_fetcher.h:85: void WaitForToken();
Is that interface actually useful? You'll block the calling thread, so this
might also block operations necessary for the fetching that run on the same
thread.

http://codereview.chromium.org/4121003/diff/42001/7032#newcode106
chrome/browser/policy/device_token_fetcher.h:106: kFailureState
The style guide example uses prefix-style constant names i.e. kStateXXX instead
of kXXXState.

http://codereview.chromium.org/4121003/diff/42001/7032#newcode120
chrome/browser/policy/device_token_fetcher.h:120: // rather then by reference to
ensure they outlive the callee.
As far as I can see, this is not necessary. Use references and have
NewRunnableFunction take care of the details like making copies of the
parameters.

http://codereview.chromium.org/4121003/diff/42001/7032#newcode124
chrome/browser/policy/device_token_fetcher.h:124: // Returns the device id used
to register the device with the device
s/id/ID/? You're the native :)

http://codereview.chromium.org/4121003/diff/42001/7033
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4121003/diff/42001/7033#newcode14
chrome/browser/policy/device_token_fetcher_unittest.cc:14: #include
"chrome/browser/policy/mock_device_management_backend.h"
alphabetize

http://codereview.chromium.org/4121003/diff/42001/7035
File chrome/browser/policy/mock_device_management_backend.h (right):

http://codereview.chromium.org/4121003/diff/42001/7035#newcode17
chrome/browser/policy/mock_device_management_backend.h:17: class
MockDeviceManagementBackend
class comment indicating in which situations I'd want to use this?

http://codereview.chromium.org/4121003/diff/42001/7035#newcode28
chrome/browser/policy/mock_device_management_backend.h:28: std::string
device_id,
The strings in these methods should all be const-ref no?

[Paweł Hajdan Jr., 2010-10-26 13:29:53]

Code I commented in the drive-by LGTM with two comments. Thanks!

http://codereview.chromium.org/4121003/diff/45001/46006
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4121003/diff/45001/46006#newcode25
chrome/browser/policy/device_token_fetcher_unittest.cc:25:
temp_user_data_dir_.CreateUniqueTempDir();
This should EXPECT_TRUE.

http://codereview.chromium.org/4121003/diff/45001/46006#newcode31
chrome/browser/policy/device_token_fetcher_unittest.cc:31: *path =
temp_user_data_dir_.path().Append(
This should return false if temp_user_data_dir_ is not valid.

[Mattias Nissler (ping if slow), 2010-10-26 14:26:33]

http://codereview.chromium.org/4121003/diff/45001/46003
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/4121003/diff/45001/46003#newcode39
chrome/browser/policy/device_management_backend.h:39: const
em::DeviceRegisterResponse& response) = 0;
Parameter should be DeviceUnregisterResponse

[danno, 2010-10-26 15:57:39]

Integrated all feedback from Mattias and Paweł.

http://codereview.chromium.org/4121003/diff/42001/7029
File chrome/browser/net/gaia/token_service.h (right):

http://codereview.chromium.org/4121003/diff/42001/7029#newcode1
chrome/browser/net/gaia/token_service.h:1: 
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> Remove empty line.

Done.

http://codereview.chromium.org/4121003/diff/42001/7030
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/4121003/diff/42001/7030#newcode19
chrome/browser/policy/device_management_backend.h:19: class
DeviceManagementBackend : NonThreadSafe {
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> How about a class comment that explain what this is here for?

Done.

http://codereview.chromium.org/4121003/diff/42001/7031
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4121003/diff/42001/7031#newcode14
chrome/browser/policy/device_token_fetcher.cc:14: #include
"chrome/common/net/gaia/gaia_constants.h"
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> alphabetize

Done.

http://codereview.chromium.org/4121003/diff/42001/7031#newcode16
chrome/browser/policy/device_token_fetcher.cc:16: #include
"chrome/browser/policy/mock_device_management_backend.h"
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> alphabetize

Done.

http://codereview.chromium.org/4121003/diff/42001/7031#newcode126
chrome/browser/policy/device_token_fetcher.cc:126: std::string
DeviceTokenFetcher::device_token() {
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> Shouldn't we rename this to GetDeviceToken()? Blocking makes this a
non-trivial
> getter IMHO.

Done.

http://codereview.chromium.org/4121003/diff/42001/7031#newcode147
chrome/browser/policy/device_token_fetcher.cc:147: *path =
device_token_dir.Append(FILE_PATH_LITERAL("DeviceManagementToken"));
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> make this a constant?

Done.

http://codereview.chromium.org/4121003/diff/42001/7032
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/4121003/diff/42001/7032#newcode26
chrome/browser/policy/device_token_fetcher.h:26: class
StoredDeviceTokenPathProvider {
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> What's wrong with just passing in the actual path in the DeviceTokenFetcher
> ctor?
This seems to me an implementation detail of the TokenFetcher, that's why I
didn't want the client (i.e. the ConfigurationPolicyPrefStore) to have to know
about it.

http://codereview.chromium.org/4121003/diff/42001/7032#newcode85
chrome/browser/policy/device_token_fetcher.h:85: void WaitForToken();
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> Is that interface actually useful? You'll block the calling thread, so this
> might also block operations necessary for the fetching that run on the same
> thread.
Removed in favor of a notification.

http://codereview.chromium.org/4121003/diff/42001/7032#newcode106
chrome/browser/policy/device_token_fetcher.h:106: kFailureState
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> The style guide example uses prefix-style constant names i.e. kStateXXX
instead
> of kXXXState.

Done.

http://codereview.chromium.org/4121003/diff/42001/7032#newcode120
chrome/browser/policy/device_token_fetcher.h:120: // rather then by reference to
ensure they outlive the callee.
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> As far as I can see, this is not necessary. Use references and have
> NewRunnableFunction take care of the details like making copies of the
> parameters.

Done.

http://codereview.chromium.org/4121003/diff/42001/7032#newcode124
chrome/browser/policy/device_token_fetcher.h:124: // Returns the device id used
to register the device with the device
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> s/id/ID/? You're the native :)

Done.

http://codereview.chromium.org/4121003/diff/42001/7033
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4121003/diff/42001/7033#newcode14
chrome/browser/policy/device_token_fetcher_unittest.cc:14: #include
"chrome/browser/policy/mock_device_management_backend.h"
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> alphabetize

Done.

http://codereview.chromium.org/4121003/diff/42001/7035
File chrome/browser/policy/mock_device_management_backend.h (right):

http://codereview.chromium.org/4121003/diff/42001/7035#newcode17
chrome/browser/policy/mock_device_management_backend.h:17: class
MockDeviceManagementBackend
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> class comment indicating in which situations I'd want to use this?

Done.

http://codereview.chromium.org/4121003/diff/42001/7035#newcode28
chrome/browser/policy/mock_device_management_backend.h:28: std::string
device_id,
On 2010/10/26 13:20:06, Mattias Nissler wrote:
> The strings in these methods should all be const-ref no?

Done.

http://codereview.chromium.org/4121003/diff/45001/46003
File chrome/browser/policy/device_management_backend.h (right):

http://codereview.chromium.org/4121003/diff/45001/46003#newcode39
chrome/browser/policy/device_management_backend.h:39: const
em::DeviceRegisterResponse& response) = 0;
On 2010/10/26 14:26:33, Mattias Nissler wrote:
> Parameter should be DeviceUnregisterResponse

Done.

http://codereview.chromium.org/4121003/diff/45001/46006
File chrome/browser/policy/device_token_fetcher_unittest.cc (right):

http://codereview.chromium.org/4121003/diff/45001/46006#newcode25
chrome/browser/policy/device_token_fetcher_unittest.cc:25:
temp_user_data_dir_.CreateUniqueTempDir();
On 2010/10/26 13:29:53, Paweł Hajdan Jr. wrote:
> This should EXPECT_TRUE.

Done.

http://codereview.chromium.org/4121003/diff/45001/46006#newcode31
chrome/browser/policy/device_token_fetcher_unittest.cc:31: *path =
temp_user_data_dir_.path().Append(
On 2010/10/26 13:29:53, Paweł Hajdan Jr. wrote:
> This should return false if temp_user_data_dir_ is not valid.

Done.

[zel, 2010-10-26 16:55:34]

Adding tim@ and nick@ instead of chron@.

[tim (not reviewing), 2010-10-26 17:42:30]

http://codereview.chromium.org/4121003/diff/63001/64005
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/4121003/diff/63001/64005#newcode65
chrome/browser/policy/device_token_fetcher.h:65: StoredDeviceTokenPathProvider*
path_provider);
remove explicit

http://codereview.chromium.org/4121003/diff/63001/64005#newcode83
chrome/browser/policy/device_token_fetcher.h:83: // server.
won't this end up returning true immediately following construction but before
StartFetching was called?  

(also, could be const method)

http://codereview.chromium.org/4121003/diff/63001/64005#newcode87
chrome/browser/policy/device_token_fetcher.h:87: // outstanding requests to the
device management server are satisfied.
what happens on error?

http://codereview.chromium.org/4121003/diff/63001/64009
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/4121003/diff/63001/64009#newcode93
chrome/browser/policy/proto/device_management_backend.proto:93: required string
policy_scope = 1;
I don't think 'required' is truly what you want in all the plcaes in this file.
*Especially* in places with this kind of TODO surrounding it.  It makes this
proto very un-future proof, speaking from first hand experience with sync.  Take
a look at sync.proto; at one point we added a required 'name' to a synced entity
thinking it made sense that we'd always need a name for an object, and now it's
unused but we're stuck with it forever :(

I'll just recommend that you reconsider using optional fields.

http://codereview.chromium.org/4121003/diff/63001/64012
File chrome/common/net/gaia/gaia_constants.cc (right):

http://codereview.chromium.org/4121003/diff/63001/64012#newcode25
chrome/common/net/gaia/gaia_constants.cc:25: // Service name for cloud-based
policy
is this sentence complete or are you missing a final word (and period :P)?

[Mattias Nissler (ping if slow), 2010-10-27 16:29:34]

LGTM on my part. Can somebody with more protoc experience comment on the gyp
changes please?

[danno, 2010-10-27 16:31:26]

uploaded a new version that addresses Tim's comments.

http://codereview.chromium.org/4121003/diff/63001/64005
File chrome/browser/policy/device_token_fetcher.h (right):

http://codereview.chromium.org/4121003/diff/63001/64005#newcode65
chrome/browser/policy/device_token_fetcher.h:65: StoredDeviceTokenPathProvider*
path_provider);
On 2010/10/26 17:42:31, timsteele wrote:
> remove explicit

Done.

http://codereview.chromium.org/4121003/diff/63001/64005#newcode83
chrome/browser/policy/device_token_fetcher.h:83: // server.
On 2010/10/26 17:42:31, timsteele wrote:
> won't this end up returning true immediately following construction but before
> StartFetching was called?  
> 
> (also, could be const method)

I don't think that's the case. It returns the status of the event that gets
signalled either 1) when the token is read off of disk in the constructor if
it's already been fetched from the server or 2) when the server request returns.
It's only really interesting in the 2) case.

Also, I can't use const, since IsSignalled isn't const on the WaitableEvent, so
the call would throw away the const qualifier :-(

http://codereview.chromium.org/4121003/diff/63001/64005#newcode87
chrome/browser/policy/device_token_fetcher.h:87: // outstanding requests to the
device management server are satisfied.
On 2010/10/26 17:42:31, timsteele wrote:
> what happens on error?
Augmented the comment so that it's clear what happens in the error case.

http://codereview.chromium.org/4121003/diff/63001/64009
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/4121003/diff/63001/64009#newcode93
chrome/browser/policy/proto/device_management_backend.proto:93: required string
policy_scope = 1;
This proto buffer interface provided by the team that will be providing the
device management backend server, I'll pass this feedback on to zzuo.

On 2010/10/26 17:42:31, timsteele wrote:
> I don't think 'required' is truly what you want in all the plcaes in this
file.
> *Especially* in places with this kind of TODO surrounding it.  It makes this
> proto very un-future proof, speaking from first hand experience with sync. 
Take
> a look at sync.proto; at one point we added a required 'name' to a synced
entity
> thinking it made sense that we'd always need a name for an object, and now
it's
> unused but we're stuck with it forever :(
> 
> I'll just recommend that you reconsider using optional fields.

http://codereview.chromium.org/4121003/diff/63001/64012
File chrome/common/net/gaia/gaia_constants.cc (right):

http://codereview.chromium.org/4121003/diff/63001/64012#newcode25
chrome/common/net/gaia/gaia_constants.cc:25: // Service name for cloud-based
policy
On 2010/10/26 17:42:31, timsteele wrote:
> is this sentence complete or are you missing a final word (and period :P)?

Done.

[danno, 2010-10-27 20:13:51]

latest round of changes to the .proto file plus a tweak to make try bots work.

http://codereview.chromium.org/4121003/diff/63001/64009
File chrome/browser/policy/proto/device_management_backend.proto (right):

http://codereview.chromium.org/4121003/diff/63001/64009#newcode93
chrome/browser/policy/proto/device_management_backend.proto:93: required string
policy_scope = 1;
Made this optional after discussion with zzuo.
On 2010/10/27 16:31:26, danno wrote:
> This proto buffer interface provided by the team that will be providing the
> device management backend server, I'll pass this feedback on to zzuo.
> 
> On 2010/10/26 17:42:31, timsteele wrote:
> > I don't think 'required' is truly what you want in all the plcaes in this
> file.
> > *Especially* in places with this kind of TODO surrounding it.  It makes this
> > proto very un-future proof, speaking from first hand experience with sync. 
> Take
> > a look at sync.proto; at one point we added a required 'name' to a synced
> entity
> > thinking it made sense that we'd always need a name for an object, and now
> it's
> > unused but we're stuck with it forever :(
> > 
> > I'll just recommend that you reconsider using optional fields.
> 
>

http://codereview.chromium.org/4121003/diff/37003/84004
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4121003/diff/37003/84004#newcode31
chrome/browser/policy/device_token_fetcher.cc:31: *path =
dir_path.Append(FILE_PATH_LITERAL("DeviceManagementToken"));
Turns out that the path literal doesn't coexist nicely with string constants on
Windows, since the macro prepends an "L" to the argument, which changes the
string const symbol name causing the compile to fail. The path literal literally
has to be a literal.

[Mattias Nissler (ping if slow), 2010-10-28 09:36:16]

http://codereview.chromium.org/4121003/diff/37003/84004
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4121003/diff/37003/84004#newcode90
chrome/browser/policy/device_token_fetcher.cc:90:
DeviceManagementBackend::ErrorCode code) {
I believe the argument fits the line, so no need for line break.

[tim (not reviewing), 2010-10-28 21:17:04]

On 2010/10/28 09:36:16, Mattias Nissler wrote:
> http://codereview.chromium.org/4121003/diff/37003/84004
> File chrome/browser/policy/device_token_fetcher.cc (right):
> 
> http://codereview.chromium.org/4121003/diff/37003/84004#newcode90
> chrome/browser/policy/device_token_fetcher.cc:90:
> DeviceManagementBackend::ErrorCode code) {
> I believe the argument fits the line, so no need for line break.

LGTM. The include rules appear fine to me (no need for TODO) although I'm not
sure if there's a way to share some of the genproto rule stuff that we have in
there for sync already.  Perhaps add a TODO there and someone who knows more
about gyp/protoc (such as nick@) can comment to help you fix later.