NOT FOR COMMIT: Add DeviceInfo type and ChangeProcessor

chrome/browser/sync/glue/synced_device_tracker.cc

+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/sync/glue/synced_device_tracker.h"
+
+#include "base/stringprintf.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "chrome/common/chrome_version_info.h"
+#include "content/public/browser/browser_thread.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "sync/internal_api/public/read_node.h"
+#include "sync/internal_api/public/read_transaction.h"
+#include "sync/internal_api/public/write_node.h"
+#include "sync/internal_api/public/write_transaction.h"
+#include "sync/syncable/directory.h"
+#include "sync/util/get_session_name.h"
+
+namespace browser_sync {
+
+SyncedDeviceTracker::SyncedDeviceTracker() : ChangeProcessor(NULL) {
+}
+
+SyncedDeviceTracker::~SyncedDeviceTracker() { }
+
+void SyncedDeviceTracker::StartImpl(Profile* profile) { }
+
+void SyncedDeviceTracker::StopImpl() { }
+
+void SyncedDeviceTracker::ApplyChangesFromSyncModel(
+      const syncer::BaseTransaction* trans,
+      const syncer::ImmutableChangeRecordList& changes) {
+  // If desired, we could maintain a cache of device info.  This method will be
+  // called with a transaction every time the device info is modified, so this
+  // would be the right place to update the cache.
+}
+
+void SyncedDeviceTracker::CommitChangesFromSyncModel() {
+  // TODO(sync): notify our listeners.
+}
+
+namespace {
+
+std::string DeviceInfoLookupString(const std::string& cache_guid) {

[Nicolas Zea, 2012/09/13 00:45:55]

nit: I find it a bit clearer when local helper function like these are at the
top of the file. Up to you though.

Also, a comment about what this method is for

[rlarocque, 2012/09/14 01:03:07]

Done.

+  return base::StringPrintf("DeviceInfo_%s", cache_guid.c_str());
+}
+
+std::string GetPlatformString() {
+#if defined(OS_CHROMEOS)
+  return "ChromeOS";
+#elif defined(OS_LINUX)
+  return "Linux";
+#elif defined(OS_MACOSX)
+  return "Mac";
+#elif defined(OS_WIN)
+  return "Windows";
+#endif
+  return "Unknown";
+}
+
+}  // namespace
+
+bool SyncedDeviceTracker::GetLocalDeviceInfo(DeviceInfo* out) {
+  syncer::ReadTransaction trans(FROM_HERE, share_handle());
+  return GetLocalDeviceInfo(trans, out);
+}
+
+bool SyncedDeviceTracker::GetLocalDeviceInfo(

[Nicolas Zea, 2012/09/13 00:45:55]

Does this need to be public? If not, GetLocalDeviceInfoImpl might be more
appropriate, and have it be private.

[rlarocque, 2012/09/14 01:03:07]

It's not currently used anywhere, but I expect this will be part of the
functionality we want to expose to clients of this class.

+    syncer::BaseTransaction& trans, DeviceInfo* out) {
+  syncer::ReadNode node(&trans);
+  const std::string lookup_string =

[Nicolas Zea, 2012/09/13 00:45:55]

make const ref?

[rlarocque, 2012/09/14 01:03:07]

Done.

+      DeviceInfoLookupString(share_handle()->directory->cache_guid());
+  syncer::BaseNode::InitByLookupResult lookup_result =
+      node.InitByClientTagLookup(syncer::DEVICE_INFO, lookup_string);
+  if (lookup_result != syncer::BaseNode::INIT_OK)

[Nicolas Zea, 2012/09/13 00:45:55]

nit: may as well just do the InitByClient.. call inside of the if statement

[rlarocque, 2012/09/14 01:03:07]

Done.

+    return false;
+
+  const sync_pb::DeviceInfoSpecifics& specifics = node.GetDeviceInfoSpecifics();

[Nicolas Zea, 2012/09/13 00:45:55]

I'm a bit hesitant about how this will discard unknown fields, but I guess since
this is the local device info it doesn't really matter...

[rlarocque, 2012/09/14 01:03:07]

The client has no business processing fields that it didn't know about at
compile time.  I think dropping those fields is the right thing to do.

+  out->session_name = specifics.name();
+  out->platform = specifics.platform();
+  out->chrome_version = specifics.chrome_version();
+
+  return true;
+}
+
+void SyncedDeviceTracker::InitLocalDeviceInfo(const base::Closure& callback) {
+  const scoped_refptr<base::TaskRunner> blocking_task_runner(
+      content::BrowserThread::GetBlockingPool());
+  syncer::GetSessionName(

[rlarocque, 2012/09/08 01:20:44]

I'd prefer it if we could just block the sync thread here.  We can't do anything
while we're waiting.  It would also help avoid some races (ie. what happens if
we are told to shut down while this call is in progress?)

[Nicolas Zea, 2012/09/13 00:45:55]

Since you're using weak pointers, this class can be destroyed during shutdown
without problem right?

[rlarocque, 2012/09/14 01:03:07]

In theory, yes.  I think this code is thread safe.  I'd prefer to not have to
think about it at all, though.  

+      blocking_task_runner.get(),
+      base::Bind(&SyncedDeviceTracker::InitLocalDeviceInfoContinuation,
+                 AsWeakPtr(), callback));
+}
+
+void SyncedDeviceTracker::InitLocalDeviceInfoContinuation(
+    const base::Closure& callback, const std::string& session_name) {
+  DCHECK(running());
+  DCHECK(!share_handle()->directory->cache_guid().empty());
+
+  chrome::VersionInfo version_info;
+
+  DeviceInfo info;
+  info.session_name = session_name;
+  info.platform = GetPlatformString();
+  info.chrome_version = version_info.CreateVersionString();
+
+  WriteLocalDeviceInfo(info);
+
+  callback.Run();
+}
+
+void SyncedDeviceTracker::WriteLocalDeviceInfo(const DeviceInfo& info) {
+  sync_pb::DeviceInfoSpecifics specifics;
+  specifics.set_cache_guid(share_handle()->directory->cache_guid());
+  specifics.set_name(info.session_name);
+  specifics.set_platform(info.platform);
+  specifics.set_chrome_version(info.chrome_version);
+
+  syncer::WriteTransaction trans(FROM_HERE, share_handle());
+  syncer::WriteNode node(&trans);
+
+  std::string lookup_string =
+      DeviceInfoLookupString(share_handle()->directory->cache_guid());
+
+  syncer::BaseNode::InitByLookupResult lookup_result =
+      node.InitByClientTagLookup(syncer::DEVICE_INFO, lookup_string);
+
+  if (lookup_result == syncer::BaseNode::INIT_OK) {

[Nicolas Zea, 2012/09/13 00:45:55]

nit: no need to have lookup_result in it's own var right?

[rlarocque, 2012/09/14 01:03:07]

Done.

+    node.SetDeviceInfoSpecifics(specifics);
+  } else {
+    syncer::ReadNode type_root(&trans);
+    syncer::BaseNode::InitByLookupResult type_root_lookup_result =
+        type_root.InitByTagLookup(ModelTypeToRootTag(syncer::DEVICE_INFO));
+    DCHECK(type_root_lookup_result == syncer::BaseNode::INIT_OK);

[Nicolas Zea, 2012/09/13 00:45:55]

dcheck_eq here and below.

[rlarocque, 2012/09/14 01:03:07]

Done.

+
+    syncer::WriteNode new_node(&trans);
+    syncer::WriteNode::InitUniqueByCreationResult create_result =
+        new_node.InitUniqueByCreation(syncer::DEVICE_INFO,
+                                      type_root,
+                                      lookup_string);
+    DCHECK(create_result == syncer::WriteNode::INIT_SUCCESS);
+    new_node.SetDeviceInfoSpecifics(specifics);
+  }
+}
+
+}  // namespace browser_sync