Synced Notification Sync Change Processor

chrome/browser/notifier/synced_notification.h

+// 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 {
+
+class SyncedNotificationSpecifics;
+
+class SyncedNotification {
+ public:
+  explicit SyncedNotification(const syncer::SyncData sync_data);

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

Pass by const reference please.

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

Done.

+
+  ~SyncedNotification();
+
+  // Here are some helper functions to get individual data parts out of a
+  // syncedNotification protobuf.
+  const std::string get_title() const;

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

GetTitle. Ditto for everything else.

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

Done.

+  const std::string get_app_id() const;
+  const std::string get_coalescing_key() const;
+  const std::string get_origin_url() const;
+  const std::string get_icon_url() const;
+
+  const std::string get_first_external_id() const;
+  const std::string get_notification_id() const;
+  const std::string get_body() const;
+
+  bool Equals(const SyncedNotification& other) const;
+
+  void set_has_local_changes(bool has_local_changes) {
+    has_local_changes_ = has_local_changes;
+  }
+
+  bool has_local_changes() const {
+    return has_local_changes_;
+  }
+
+  syncer::SyncData* mutable_sync_data() {
+    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:
+  // helper function to mark a notification as read or dismissed.
+  bool SetReadState(
+      SyncedNotificationCoalescedNotification_ReadState readState);
+  // This keeps the SyncNotificationSpecifics alive by putting
+  // it into the contained sync entity.

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

I really don't understand what this comment means. There's no other reference to
SyncedNotificationSpecifics in the .h. Maybe just remove this comment or move it
to where it applies in the .cc.

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

Is the only reason you keep around the sync data the fact that all your get*
methods query it directly? Would it be better to just have local variables that
are parsed directly from the specifics?

E.g. have a static ParseFromSyncData method that fills the local variables
(title, app id, coalescing key, etc.), as well as a CreateSyncData method to
create a new SyncData from all the local variables if local changes have been
made.

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

Moved comment to the .cc file

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

My thinking in keeping it around is that I need to compare it (which comes in
off the wire) with my local data, so I need two copies.  If you think I don't
need to do that, I can just copy the data.  Also, if you think it is better, I
could have two lists of my structure, one for data from the wire, one for local
data.

[Nicolas Zea, 2013/01/25 00:18:45]

I think it would be better to compare by having two separate SyncedNotification
objects, which then have comparison functions (::Equals(const
SyncedNotification& other)). Having an implicit copy of the data in here seems
like a roundabout way of just having two independent objects.

[Pete Williamson, 2013/01/25 19:58:36]

As it turns out, there is another reason to keep the sync_data around, we need
to hand it back out in response to the GetAllSync data method, which requires us
to hand it out  as sync_data instead of as a SyncedNotification object. 

I still changed the class to use local variables and initialize them at startup
as you suggest, so in case you can tell me that I don't need to support
GetAllSyncData, I can remove the sync_data_ object from the SyncedNotification.

+  syncer::SyncData  sync_data_;

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

Extra space.

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

Done.

+  // Set this to true if we make any changes to the client data.
+  bool has_local_changes_;

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

DISALLOW_COPY_AND_ASSIGN.

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

Done.

+};
+
+
+}  // namespace sync_pb
+
+#endif  // CHROME_BROWSER_NOTIFIER_SYNCED_NOTIFICATION_H_