Synced Notification Sync Change Processor

chrome/browser/notifier/synced_notification.h

Index: chrome/browser/notifier/synced_notification.h
diff --git a/chrome/browser/notifier/synced_notification.h b/chrome/browser/notifier/synced_notification.h
new file mode 100644
index 0000000000000000000000000000000000000000..97c85bbd822640fa23f81fa9255d2beb824bc2dd
--- /dev/null
+++ b/chrome/browser/notifier/synced_notification.h
@@ -0,0 +1,72 @@
+// 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.
+
+// This class represents the data for a single Synced Notification.
+// It should map 1-1 to all the data in synced_notification_sepcifics.proto,
+// and the data and render protobufs that the specifics protobuf contains.
+
+#ifndef CHROME_BROWSER_NOTIFIER_SYNCED_NOTIFICATION_H_
+#define CHROME_BROWSER_NOTIFIER_SYNCED_NOTIFICATION_H_
+
+#include "sync/api/sync_data.h"
+#include "sync/protocol/sync.pb.h"
+#include "sync/protocol/synced_notification_specifics.pb.h"
+
+namespace sync_pb {

[Nicolas Zea, 2013/01/05 01:18:48]

This shouldn't be inside of the sync_pb namespace. That's reserved for files
generated directly from the protos.

[Pete Williamson, 2013/01/18 18:58:39]

What namespace would be better?  None?  A new namespace?  Some pre-existing sync
namespace?

[Nicolas Zea, 2013/01/18 23:09:25]

Notifier perhaps? Might also make sense to have the other classes within
browser/notifier be in a notifier namespace.

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+
+class SyncedNotificationSpecifics;
+
+class SyncedNotification {

[Nicolas Zea, 2013/01/05 01:18:48]

What are you trying to accomplish with this class? If it's just a 1:1 c++
version of the proto, how is it different from
sync_pb::SyncNotificationSpecifics?

[Pete Williamson, 2013/01/18 18:58:39]

The primary point of this class is to do all the grungy logic of getting things
out of a deeply nested sync class and making them easily available to the rest
of the code.

+ public:
+  explicit SyncedNotification(const syncer::SyncData sync_data);
+
+  ~SyncedNotification();
+
+  // Here are some helper functions to get individual data parts out of a
+  // syncedNotification protobuf.
+  const std::string title() const;

[dcheng, 2013/01/07 20:42:10]

Why return a const string?

[Pete Williamson, 2013/01/18 18:58:39]

Would you prefer a const string&?
I don't think I understand the intent of your question.
The data souldn't change, hence the const.
The data is in string form, hence the string.

[dcheng, 2013/01/18 21:56:13]

Returning a const string doesn't guarantee that, and it just makes things harder
for people that want a mutable string (as they just have to make another copy
then).

[Pete Williamson, 2013/01/23 01:45:55]

Done.

+  const std::string app_id() const;
+  const std::string coalescing_key() const;
+
+  const std::string get_first_external_id() const;
+
+  // TODO(petewil): Revisit this when I add more data.
+  bool Equals(const SyncedNotification& other) {
+    // Two notifications are equal if the <appId/coalescingKey> pair matches
+    return true;
+  }
+
+  void set_has_local_changes(bool has_local_changes) {
+    has_local_changes_ = has_local_changes;
+  }
+
+  bool has_local_changes() {

[dcheng, 2013/01/07 20:42:10]

This method should be const.

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+    return has_local_changes_;
+  }
+
+  syncer::SyncData* sync_data() {

[dcheng, 2013/01/07 20:42:10]

Should this return a const pointer instead? If not, this should probably be
called mutable_sync_data().

[Pete Williamson, 2013/01/18 18:58:39]

Done.

+    return &sync_data_;
+  }
+
+  void NotificationHasBeenRead();
+
+  void NotificationHasBeenDeleted();
+
+  // This gets a pointer to the SyncedNotificationSpecifics part
+  // of the sync data.  This is owned by the SyncedNotification class
+  // (actually refcounted, so it is jointly owned with sync).
+  // Don't free this pointer!
+  const sync_pb::SyncedNotificationSpecifics* GetSyncedNotificationSpecifics();
+
+ private:
+  // This keeps the SyncNotificationSpecifics alive.

[dcheng, 2013/01/07 20:42:10]

How does it keep it alive?

[Pete Williamson, 2013/01/18 18:58:39]

Comment updated to explain how it is kept alive.

+  syncer::SyncData  sync_data_;
+  // Set this to true if we make any changes to the client data.
+  bool has_local_changes_;
+};
+
+
+}  // namespace sync_pb
+
+#endif  // CHROME_BROWSER_NOTIFIER_SYNCED_NOTIFICATION_H_