Synced Notification Sync Change Processor

chrome/browser/notifier/chrome_notifier_service.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.
+
+#ifndef CHROME_BROWSER_NOTIFIER_CHROME_NOTIFIER_SERVICE_H_
+#define CHROME_BROWSER_NOTIFIER_CHROME_NOTIFIER_SERVICE_H_
+
+#include "base/basictypes.h"
+#include "base/memory/scoped_vector.h"
+#include "base/threading/non_thread_safe.h"
+#include "chrome/browser/notifier/synced_notification.h"
+#include "chrome/browser/profiles/profile_keyed_service.h"
+#include "sync/api/syncable_service.h"
+
+class Profile;
+class NotificationUIManager;
+
+// The ChromeNotifierService holds notifications which

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

Optional/preference/nit: I'm just eyeballing this, but try to wrap comments at
80 columns, not before. 80 columns is very few already, no need to make it
harder!

[Pete Williamson, 2013/01/24 01:48:11]

Done.

+// represent the state of delivered notifications for chrome.
+// These are obtained from the sync service and kept up to date.
+class ChromeNotifierService : public syncer::SyncableService,
+                              public ProfileKeyedService,
+                              public base::NonThreadSafe {

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

Why not use ThreadChecker (which uses composition) instead of inheritance?

Also, note that you have to actually call CalledOnValidThread() for this to do
anything =)

[dcheng, 2013/01/23 18:39:43]

I think you missed the comments in this file.

[Pete Williamson, 2013/01/24 01:48:11]

OK, removed the inheritance from base::NonThreadSafe, I'll go back to my plan to
use thread checker when I add support to the unit test files.

+
+ public:
+  ChromeNotifierService(Profile* profile, NotificationUIManager* manager);
+  virtual ~ChromeNotifierService();
+
+  // Methods from ProfileKeyedService.
+  virtual void Shutdown() OVERRIDE;
+
+  // syncer::SyncableService implementation.
+  virtual syncer::SyncMergeResult MergeDataAndStartSyncing(
+      syncer::ModelType type,
+      const syncer::SyncDataList& initial_sync_data,
+      scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
+      scoped_ptr<syncer::SyncErrorFactory> error_handler) OVERRIDE;
+  virtual void StopSyncing(syncer::ModelType type) OVERRIDE;
+  virtual syncer::SyncDataList GetAllSyncData(
+      syncer::ModelType type) const OVERRIDE;
+  virtual syncer::SyncError ProcessSyncChanges(
+      const tracked_objects::Location& from_here,
+      const syncer::SyncChangeList& change_list) OVERRIDE;
+
+  // Convert from internal representation to SyncData representation.
+  static syncer::SyncData CreateSyncDataFromNotification(
+      sync_pb::SyncedNotification& notification);

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

Pass by const reference.

[Pete Williamson, 2013/01/24 01:48:11]

Done.

+
+  // Convert from SyncData representation to internal representation.
+  static sync_pb::SyncedNotification* CreateNotificationFromSyncData(
+      const syncer::SyncData& sync_data);

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

Optional/preference/nit: I'd personally prefer to see this made private and
public ForTest() versions for the unit test to call. It doesn't seem like anyone
outside of those two use cases should care about calling this function. But I
also don't really care.

+
+  // Add a notification to our list.  This takes ownership of the pointer.
+  void Add(scoped_ptr<sync_pb::SyncedNotification> notification);
+
+  void Show(sync_pb::SyncedNotification* notification);
+
+  // Get a pointer to a notification.  ChromeNotifierService owns this pointer.
+  // The caller must not free the it.

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

s/the//

[Pete Williamson, 2013/01/24 01:48:11]

Done.

+  sync_pb::SyncedNotification* FindNotificationById(const std::string& id);
+
+ private:
+  // Backpointer to the owning profile, do not free at shutdown.

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

Back pointer. I also feel that "do not free" is redundant, as we have a raw
pointer here anyway.

[Pete Williamson, 2013/01/24 01:48:11]

Done.

+  Profile* profile_;
+  NotificationUIManager* notification_manager_;
+
+  // TODO(petewil): consider whether a map would better suit our data.
+  // If there are many entries, lookup time may trump locality of reference.
+  ScopedVector<sync_pb::SyncedNotification> notification_data_;
+
+  DISALLOW_COPY_AND_ASSIGN(ChromeNotifierService);
+};
+
+#endif  // CHROME_BROWSER_NOTIFIER_CHROME_NOTIFIER_SERVICE_H_