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_
+

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

remove extra newline

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

Done.

+
+#include "base/basictypes.h"
+#include "base/logging.h"

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

Why do you need this header here?

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

Done.

+#include "base/memory/scoped_vector.h"
+#include "chrome/browser/notifier/synced_notification.h"
+#include "chrome/browser/profiles/profile_keyed_service.h"
+#include "sync/api/syncable_service.h"
+
+// The ChromeNotifierService holds an array of notifications which
+// represent the state of outstanding notifications for chrome.

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

What is an outstanding notification? Is the "array" detail important (it doesn't
feel like it is, it's just an implementation detail, so why mention it here?)

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

Comment clarified to remove the word array and explain outstanding.

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

Comment clarified, the words array and outstanding replaced

+// This array is obtained from the sync service and kept up to date.
+class ChromeNotifierService : public syncer::SyncableService,
+                              public ProfileKeyedService {
+
+ public:
+  ChromeNotifierService();
+  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);
+
+  // Convert from SyncData representation to internal representation.
+  static sync_pb::SyncedNotification* CreateNotificationFromSyncData(
+      const syncer::SyncData& sync_data);

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

These static methods don't really feel like they belong here. I'd prefer to see
them moved into the .cc as helper functions. Opinions vary on this, but they
feel like an implementation detail that does not need to be exposed in the .h.

+
+  bool Add(sync_pb::SyncedNotification* notification);

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

Who calls this? What does the return value mean?

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

This is called by unit tests and by other functions in the file.  I removed the
return value

+
+  // Get a pointer to a notification.  ChromeNotifierService owns this pointer.
+  // The caller must not free the it.
+  sync_pb::SyncedNotification* GetNotification(const std::string& value);
+
+ private:
+  void LogContents();

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

Please describe what this method does. Is it only for debugging? Do we want this
in production code? (It feels like the answer to the latter question should be
"no")

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

Removed.

+
+  // For now keep data in a vector, later likely move to a map.

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

I feel like this comment ought to be a TODO or simply removed. In addition, why
would it move to a map?

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

Done.

+  ScopedVector<sync_pb::SyncedNotification> our_data_;

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

While pronouns don't always need to avoided, I don't feel like "our_" adds
useful information here. It would be more useful to describe what kind of data
this vector holds...

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

Changed to "notification_data"

+};

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

DISALLOW_COPY_AND_ASSIGN.

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

Done.

+

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

Extra new line. In general, you don't really need two newlines in a row
anywhere.

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

Done.

+
+#endif  // CHROME_BROWSER_NOTIFIER_CHROME_NOTIFIER_SERVICE_H_