CrOS policies: Store device ID with device token

CrOS policies: Store device ID with device token

BUG=62798
TEST=unit tests: DeviceTokenFetcherTest.StoreAndLoad

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

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

[Jakob Kummerow (corp), 2010-11-15 17:33:50]

Is this what you had in mind, or should I do anything else? In particular, the
newly stored device_id isn't used anywhere yet, but I don't know where it would
be needed. 
Please review.

[Mattias Nissler (ping if slow), 2010-11-15 18:02:45]

Implementation is well done, need to resolve a few nits and points I'd like to
discuss.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:162: }
no curlies needed here.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:170: }
Same here? Or do you have a reason for changing the style?

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:202:
storable_device_token.SerializeToString(&data);
Should probably put a DCHECK() for the SerializeToString return value.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:205: data.length());
Hm, doesn't this fit on line 203?

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:21: // Encapsulates a
device token and the associated device ID.
The comment suggests you generate a device ID for a token, but actually it's the
other way round.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:22: message
DeviceToken {
Same for the message name here. Can we make it something more generic, maybe
DeviceMetaData? Or DeviceIdentity? Not sure these are good choices, but
DeviceToken is actually missing the ID part.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:24: required string
device_id = 2;
Can we make these optional?

[Jakob Kummerow (corp), 2010-11-16 09:37:02]

Addressed your comments. Please review again.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:162: }
On 2010/11/15 18:02:51, Mattias Nissler wrote:
> no curlies needed here.

Alright, alright, I give up.
Done.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:170: }
On 2010/11/15 18:02:51, Mattias Nissler wrote:
> Same here? Or do you have a reason for changing the style?

Done.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:202:
storable_device_token.SerializeToString(&data);
On 2010/11/15 18:02:51, Mattias Nissler wrote:
> Should probably put a DCHECK() for the SerializeToString return value.

Done.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:205: data.length());
On 2010/11/15 18:02:51, Mattias Nissler wrote:
> Hm, doesn't this fit on line 203?

Done.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:21: // Encapsulates a
device token and the associated device ID.
On 2010/11/15 18:02:51, Mattias Nissler wrote:
> The comment suggests you generate a device ID for a token, but actually it's
the
> other way round.

Done.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:22: message
DeviceToken {
On 2010/11/15 18:02:51, Mattias Nissler wrote:
> Same for the message name here. Can we make it something more generic, maybe
> DeviceMetaData? Or DeviceIdentity? Not sure these are good choices, but
> DeviceToken is actually missing the ID part.

Done.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/proto/dev...
chrome/browser/policy/proto/device_management_local.proto:24: required string
device_id = 2;
On 2010/11/15 18:02:51, Mattias Nissler wrote:
> Can we make these optional?

Done.

[Mattias Nissler (ping if slow), 2010-11-16 09:58:14]

LGTM with a nit to check just in case.

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
File chrome/browser/policy/device_token_fetcher.cc (right):

http://codereview.chromium.org/4949003/diff/1/chrome/browser/policy/device_to...
chrome/browser/policy/device_token_fetcher.cc:162: }
On 2010/11/16 09:37:02, Jakob Kummerow wrote:
> On 2010/11/15 18:02:51, Mattias Nissler wrote:
> > no curlies needed here.
> 
> Alright, alright, I give up.
> Done.

:)

http://codereview.chromium.org/4949003/diff/7001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/4949003/diff/7001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_local.proto:24: optional string
device_token = 2;
Just checking: Now that these are optional, will the accessors just return an
empty string when the fields are not present?

[Jakob Kummerow (corp), 2010-11-16 10:58:43]

ready for landing?

http://codereview.chromium.org/4949003/diff/7001/chrome/browser/policy/proto/...
File chrome/browser/policy/proto/device_management_local.proto (right):

http://codereview.chromium.org/4949003/diff/7001/chrome/browser/policy/proto/...
chrome/browser/policy/proto/device_management_local.proto:24: optional string
device_token = 2;
On 2010/11/16 09:58:14, Mattias Nissler wrote:
> Just checking: Now that these are optional, will the accessors just return an
> empty string when the fields are not present?

Done.
As discussed offline, did some restructuring to
DeviceTokenFetcher::AttemptTokenLoadFromDisk().

[Mattias Nissler (ping if slow), 2010-11-16 11:58:18]

LGTM

[Jakob Kummerow (corp), 2010-11-17 10:29:44]

(moved danno from reviewers to CC; added markusheintz to CC)