sync: Add DeviceInfo protobuf and supporting code

sync: Add DeviceInfo protobuf and supporting code

This commit introduces the DeviceInfo type and protobuf.  It also
introduces the DeviceInfo class, which provides an interface for the
rest of the code to access the information stored within the DeviceInfo
type.  The DeviceInfo class takes over some functions that used to
belong to the SessionModelAssociator.

The ChangeProcessor that keeps this information up to date and exposes
notifications of device info changes will be added in a future commit.

At the time of this commit, this should all be mostly dead code.  The
server does not support this type yet, so we do not yet attempt to
download or manipulate any actual DeviceInfo nodes.

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

[rlarocque, 2012-09-24 23:33:30]

The big device info patch (http://codereview.chromium.org/10911073/) has been
split into a few small patches and two big ones.  This is the first of the big
patches.  

zea: Please re-review.  Most of this should be familiar from the old patch.
raz: Please review the protos.  They've been updated based on your comments.

[Nicolas Zea, 2012-09-26 01:50:59]

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/DEPS
File chrome/browser/sync/glue/DEPS (right):

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/DEPS#...
chrome/browser/sync/glue/DEPS:20: # TODO(123674): This shouldn't be needed.
TODO(sync): crbug.com/123674

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
File chrome/browser/sync/glue/device_info.cc (right):

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.cc:75: #elif defined(OS_LINUX)
FYI, Android now has a field here (change just landed today)

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.cc:102: syncer::GetSessionName(
GetSessionName(
  content::BrowserThread::GetBlockingPool(), ...)

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
File chrome/browser/sync/glue/device_info.h (right):

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.h:8: #include <string>
newline after

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.h:27: const std::string& sync_user_agent()
const;
Does anything care about this? It seems odd to expose a sync internal value like
this, and expect other services to parse it for whatever info they need.

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.h:47: std::string client_name_;
can these be const?

http://codereview.chromium.org/10985008/diff/1/sync/protocol/device_info_spec...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10985008/diff/1/sync/protocol/device_info_spec...
sync/protocol/device_info_specifics.proto:29: // The platform of the device
(ChromeOS, Linux, Mac, or Windows).
remove the examples of platforms, it's better to just let someone look at the
enum def.

http://codereview.chromium.org/10985008/diff/1/sync/protocol/device_info_spec...
sync/protocol/device_info_specifics.proto:33: optional string sync_user_agent =
4;
I don't think it makes sense to sync this around. We had the version previously
for the sake of dashboard's consumption (I believe). The sync user agent is in
my view a union of various fields, which if we need we should set explicitly.

http://codereview.chromium.org/10985008/diff/1/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

http://codereview.chromium.org/10985008/diff/1/sync/syncable/model_type.cc#ne...
sync/syncable/model_type.cc:261: set.Remove(DEVICE_INFO);
What is this for?

[rlarocque, 2012-09-26 20:44:36]

Updated.

I rebased, but didn't see any conflicts from new OS_ANDROID #defines.  Do you
have the revision number for that commit?  I'd like to make sure my rebase
didn't destroy those changes.

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/DEPS
File chrome/browser/sync/glue/DEPS (right):

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/DEPS#...
chrome/browser/sync/glue/DEPS:20: # TODO(123674): This shouldn't be needed.
On 2012/09/26 01:50:59, nzea wrote:
> TODO(sync): crbug.com/123674

Done.

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
File chrome/browser/sync/glue/device_info.cc (right):

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.cc:75: #elif defined(OS_LINUX)
On 2012/09/26 01:50:59, nzea wrote:
> FYI, Android now has a field here (change just landed today)

Thanks.  I'll be sure to rebase before commit.

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.cc:102: syncer::GetSessionName(
On 2012/09/26 01:50:59, nzea wrote:
> GetSessionName(
>   content::BrowserThread::GetBlockingPool(), ...)

Done.

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
File chrome/browser/sync/glue/device_info.h (right):

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.h:8: #include <string>
On 2012/09/26 01:50:59, nzea wrote:
> newline after

Done.

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.h:27: const std::string& sync_user_agent()
const;
On 2012/09/26 01:50:59, nzea wrote:
> Does anything care about this? It seems odd to expose a sync internal value
like
> this, and expect other services to parse it for whatever info they need.

I added this at Raz's suggestion.  It's meant to make this easier to process on
the server.

http://codereview.chromium.org/10985008/diff/1/chrome/browser/sync/glue/devic...
chrome/browser/sync/glue/device_info.h:47: std::string client_name_;
On 2012/09/26 01:50:59, nzea wrote:
> can these be const?

Sure, let's do that.

http://codereview.chromium.org/10985008/diff/1/sync/protocol/device_info_spec...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10985008/diff/1/sync/protocol/device_info_spec...
sync/protocol/device_info_specifics.proto:29: // The platform of the device
(ChromeOS, Linux, Mac, or Windows).
On 2012/09/26 01:50:59, nzea wrote:
> remove the examples of platforms, it's better to just let someone look at the
> enum def.

Done.

http://codereview.chromium.org/10985008/diff/1/sync/protocol/device_info_spec...
sync/protocol/device_info_specifics.proto:33: optional string sync_user_agent =
4;
On 2012/09/26 01:50:59, nzea wrote:
> I don't think it makes sense to sync this around. We had the version
previously
> for the sake of dashboard's consumption (I believe). The sync user agent is in
> my view a union of various fields, which if we need we should set explicitly.

Let's bring this up with Raz.  We should also double-check to see what the
dashboard's requirements are.

http://codereview.chromium.org/10985008/diff/1/sync/syncable/model_type.cc
File sync/syncable/model_type.cc (right):

http://codereview.chromium.org/10985008/diff/1/sync/syncable/model_type.cc#ne...
sync/syncable/model_type.cc:261: set.Remove(DEVICE_INFO);
On 2012/09/26 01:50:59, nzea wrote:
> What is this for?

The server doesn't currently support this type.  If it wasn't for this line,
sync would fail to initialize.

[rlarocque, 2012-10-01 20:53:49]

Patch updated.  I think this latest protobuf definition matches what we agreed
on during Friday's discussion.

http://codereview.chromium.org/10985008/diff/15001/chrome/browser/sync/glue/DEPS
File chrome/browser/sync/glue/DEPS (right):

http://codereview.chromium.org/10985008/diff/15001/chrome/browser/sync/glue/D...
chrome/browser/sync/glue/DEPS:21: "+sync/syncable/entry.h",
Note that this change will cause us to fail deps checks unless we commit
http://codereview.chromium.org/10966002/ first.

I think it's very likely that patch will land before this one, so I've changed
the deps in advance.

[Nicolas Zea, 2012-10-01 21:24:29]

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.cc (right):

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:16: sync_user_agent_("Unknown"),
any reason these are different? (unset vs unknown)

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:76: const chrome::VersionInfo
&version_info) {
VersionInfo& version_info

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:90: user_agent += "MAC ";
you're missing android

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.h (right):

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:42: base::Callback<void(const DeviceInfo&
local_info)> callback);
typedef the callback?

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:48: const chrome::VersionInfo
&version_info);
VersionInfo& version_info

http://codereview.chromium.org/10985008/diff/18001/sync/protocol/device_info_...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10985008/diff/18001/sync/protocol/device_info_...
sync/protocol/device_info_specifics.proto:20: // We are mapping the unique
per-device cache guid to more specific information
given the guid includes the word unique, calling it a unique guid seems
redundant (here and below)

[rlarocque, 2012-10-01 22:24:12]

Fixed some issues.

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.cc (right):

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:16: sync_user_agent_("Unknown"),
On 2012/10/01 21:24:30, nzea wrote:
> any reason these are different? (unset vs unknown)

I seem to remember that this is the way it was before, but I'm having trouble
digging up the historical evidence to support that claim.  Maybe I'm just
imagining things.

We can make them uniform.  Do you prefer "Unset", "Unknown", an empty string, or
some other value?  Personally, I prefer "Unset".

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:76: const chrome::VersionInfo
&version_info) {
On 2012/10/01 21:24:30, nzea wrote:
> VersionInfo& version_info

Done.

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:90: user_agent += "MAC ";
On 2012/10/01 21:24:30, nzea wrote:
> you're missing android

!!!

I specifically looked for this when I updated the patch.  I guess I fail at
using git. It wasn't in my local repo, and I don't remember resolving any
conflicts involving this code, but it's definitely in trunk.

Fixed.

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.h (right):

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:42: base::Callback<void(const DeviceInfo&
local_info)> callback);
On 2012/10/01 21:24:30, nzea wrote:
> typedef the callback?

It's not duplicated very often, and the syntax isn't too ugly.  I'd prefer to
not typedef, though I'm willing to change it if you feel strongly.

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.h:48: const chrome::VersionInfo
&version_info);
On 2012/10/01 21:24:30, nzea wrote:
> VersionInfo& version_info

Done.

http://codereview.chromium.org/10985008/diff/18001/sync/protocol/device_info_...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10985008/diff/18001/sync/protocol/device_info_...
sync/protocol/device_info_specifics.proto:20: // We are mapping the unique
per-device cache guid to more specific information
On 2012/10/01 21:24:30, nzea wrote:
> given the guid includes the word unique, calling it a unique guid seems
> redundant (here and below)

Good point.  Fixed.

[Nicolas Zea, 2012-10-05 20:28:51]

LGTM

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.cc (right):

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:16: sync_user_agent_("Unknown"),
On 2012/10/01 22:24:12, rlarocque wrote:
> On 2012/10/01 21:24:30, nzea wrote:
> > any reason these are different? (unset vs unknown)
> 
> I seem to remember that this is the way it was before, but I'm having trouble
> digging up the historical evidence to support that claim.  Maybe I'm just
> imagining things.
> 
> We can make them uniform.  Do you prefer "Unset", "Unknown", an empty string,
or
> some other value?  Personally, I prefer "Unset".

I'd prefer the same value. Either one works though.

http://codereview.chromium.org/10985008/diff/22001/sync/protocol/device_info_...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10985008/diff/22001/sync/protocol/device_info_...
sync/protocol/device_info_specifics.proto:20: // are mapping the per-device
cache guid to more specific information about the
per-device -> per-client?

[rlarocque, 2012-10-05 21:05:00]

Thanks, Nicolas!  I've uploaded a new patch to address your last two comments.

Raz, are you OK with the current protobuf definition?

If so, we have a decision to make.  I can commit this patch as-is, but a lot of
the code is disabled right now.  Enabling the functionality would require
another commit once server support is available.

If it's going to be a while before the server supports this (ie. weeks), then I
think it makes sense to commit now to ensure this patch doesn't get stale. 
Otherwise, I'd prefer to wait until the server support is available before
committing.

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
File chrome/browser/sync/glue/device_info.cc (right):

http://codereview.chromium.org/10985008/diff/18001/chrome/browser/sync/glue/d...
chrome/browser/sync/glue/device_info.cc:16: sync_user_agent_("Unknown"),
On 2012/10/05 20:28:51, nzea wrote:
> On 2012/10/01 22:24:12, rlarocque wrote:
> > On 2012/10/01 21:24:30, nzea wrote:
> > > any reason these are different? (unset vs unknown)
> > 
> > I seem to remember that this is the way it was before, but I'm having
trouble
> > digging up the historical evidence to support that claim.  Maybe I'm just
> > imagining things.
> > 
> > We can make them uniform.  Do you prefer "Unset", "Unknown", an empty
string,
> or
> > some other value?  Personally, I prefer "Unset".
> 
> I'd prefer the same value. Either one works though.

Done.

http://codereview.chromium.org/10985008/diff/22001/sync/protocol/device_info_...
File sync/protocol/device_info_specifics.proto (right):

http://codereview.chromium.org/10985008/diff/22001/sync/protocol/device_info_...
sync/protocol/device_info_specifics.proto:20: // are mapping the per-device
cache guid to more specific information about the
On 2012/10/05 20:28:52, nzea wrote:
> per-device -> per-client?

Good catch.  Done.

[Raz Mathias, 2012-10-08 15:49:07]

Proto looks good.  Go ahead and check it in, I think we can have a server for
you this week.

[commit-bot: I haz the power, 2012-10-08 18:42:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10985008/29001

[commit-bot: I haz the power, 2012-10-08 18:43:16]

Presubmit check for 10985008-29001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 2.7s to calculate.

[rlarocque, 2012-10-08 19:21:43]

+brettw for OWNERS review of chrome/chrome_browser.gypi.

[brettw, 2012-10-08 21:51:38]

I only looked at the gypi. lgtm

[commit-bot: I haz the power, 2012-10-08 23:10:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10985008/29001

[commit-bot: I haz the power, 2012-10-08 23:28:42]

Retried try job too often for step(s) crypto_unittests, unit_tests,
cacheinvalidation_unittests, jingle_unittests, ipc_tests, interactive_ui_tests,
browser_tests, net_unittests, content_unittests, gpu_unittests, base_unittests,
media_unittests, content_browsertests, sql_unittests, printing_unittests,
sync_unit_tests, sandbox_linux_unittests, check_deps

[rlarocque, 2012-10-09 00:04:31]

This fixes some compile issues on Android and undoes the modifications to DEPS. 


The DEPS changes were dependent on https://codereview.chromium.org/10966002/,
which is currently stalled.  The changes can be reapplied if and when that patch
is committed.

[commit-bot: I haz the power, 2012-10-11 22:53:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10985008/38003

[commit-bot: I haz the power, 2012-10-12 04:01:41]

Change committed as 161496