Sync: Refactoring of DEVICE_INFO syncable type - Part 1

chrome/browser/sync/glue/local_device_info_provider.h

+// Copyright 2014 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.
+
+#ifndef CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_H_
+#define CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_H_
+
+#include <string>
+#include "base/callback_list.h"
+
+namespace browser_sync {
+
+class DeviceInfo;
+
+class LocalDeviceInfoProvider {

[maniscalco, 2014/07/08 21:17:52]

This would be a good place to describe the purpose of this class.

[stanisc, 2014/07/09 21:58:36]

Done.

+ public:
+  typedef base::CallbackList<void(void)>::Subscription Subscription;
+
+  // Returns sync's representation of the local device info.
+  // Return value is an empty scoped_ptr if the device info is unavailable.
+  virtual const DeviceInfo* GetLocalDeviceInfo() const = 0;

[maniscalco, 2014/07/08 21:17:52]

The comment mentions the returned value is a scoped_ptr, but the return type is
a const DeviceInfo*.  Is that intentional?

Also, if you do plan to return a dumb pointer be sure to document the ownership
and lifetime semantics (caller is/is-not responsible for free'ing; pointer is
invalid at the next method call, etc.).  This applies to anywhere a dumb pointer
is passed or returned.

[stanisc, 2014/07/09 21:58:36]

Good catch. This comment was copied from the previous implementation. I updated
the comment and added the ownership/lifetime information.

+
+  // Used for creation of the machine tag for this local session.
+  virtual std::string GetLocalSyncCacheGUID() const = 0;

[maniscalco, 2014/07/08 21:17:52]

Will this method return a non-empty string?  Even when IsInitialized() returns
false?

[stanisc, 2014/07/09 21:58:36]

Added comment.

+
+  // Specifies whether local device info is available
+  virtual bool IsInitialized() const = 0;

[maniscalco, 2014/07/08 21:17:52]

Is this method necessary?  Given the description of GetLocalDeviceInfo, I would
expect callers to check the return value of GetLocalDeviceInfo instead.

[stanisc, 2014/07/09 21:58:35]

I guess you are right. It is somewhat redundant and could be removed.

+
+  // Registers a callback to be called when local device info becomes available
+  virtual scoped_ptr<Subscription> RegisterOnInitializedCallback(

[maniscalco, 2014/07/08 21:17:52]

What's the return value for?  Does the lifetime of the returned Subscription
object affect whether callback is invoked?

[stanisc, 2014/07/09 21:58:36]

This follows the callback subscription pattern described in callback_list.h.
I've added a comment.

+      const base::Closure& callback) = 0;
+};
+
+}  // namespace browser_sync
+
+#endif  // CHROME_BROWSER_SYNC_GLUE_LOCAL_DEVICE_INFO_PROVIDER_H_