Synced Notification Sync Change Processor

chrome/browser/notifier/synced_notification.cc

+// 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.
+
+#include "chrome/browser/notifier/synced_notification.h"
+
+#include "sync/protocol/sync.pb.h"
+#include "sync/protocol/synced_notification_specifics.pb.h"
+
+using namespace sync_pb;
+
+SyncedNotification::SyncedNotification(const syncer::SyncData sync_data)
+    : sync_data_(sync_data), has_local_changes_(false) {
+}
+
+SyncedNotification::~SyncedNotification() {}
+
+void SyncedNotification::NotificationHasBeenRead() {
+  // We set the is_local_ flag to true since we modified it locally
+  // then we create a sync change object to pass back up
+  // TODO(petewil): Implement.
+}
+
+void SyncedNotification::NotificationHasBeenDeleted() {
+  // We set the is_deleted_ flag to true since we modified it locally
+  // then we create a sync change object to pass back up.
+  // TODO(petewil): Implement.
+}
+
+// TODO(petewil): Consider whether the repeated code below can be re-used.
+//  A first attempt to do so failed.
+
+const SyncedNotificationSpecifics*
+SyncedNotification::GetSyncedNotificationSpecifics() {
+  return &(sync_data_.GetSpecifics().synced_notification());
+}
+
+const std::string SyncedNotification::get_first_external_id() const {
+  if (!sync_data_.GetSpecifics().synced_notification().
+      has_coalesced_notification())
+    return "";
+  if (sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().notification_size() < 1)
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().notification(0).has_external_id())
+    return "";
+
+  return sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().notification(0).external_id();
+}
+
+const std::string SyncedNotification::title() const {
+

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

Remove extra newlines at the beginning of all blocks please. Same comment on
line 91 and elsewhere.

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

Done.

+  if (!sync_data_.GetSpecifics().synced_notification().

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

1) This seems overly complicated. I think a high level comment would help for
all of these helper functions.

2) Given the complexity of this method, I'm not really sure calling it "title()"
is appropriate (as that is used for simple accessors, which this is definitely
not).

3) Consider aliasing some variables:
const SyncedNotificationSpecifics& specifics =
sync_data_.GetSpecifics().synced_notification();

if (!specifics.has_coalesced_notification() ||
    !specifics.coalesced_notification().has_render_info() ||
    etc...)

Alternatively, do we really need to nest so many protobufs? This seems to be a
big reason you need to do so many has_* checks. Can we flatten the structure
some?

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

All functions renamed so they no longer have a name like a a simple accessor.

I tried aliasing at first, but for some reason that I wasn't quickly able to
debug, the aliasing failed.  I'll come back and try it later (There is a
TODO(petewil) tracking this).

The protobuf format is given to us by the server, I'm not in control of it.

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

Sure but we can still change it at this point in time...
I'm not convinced that having lots of deeply nested messages like we currently
have defined results in cleaner code (look at how many checks you have to do in
SyncedNotification!) so I'd really like to see it change, but if I'm alone in
that opinion, I will defer to the majority.

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

I used Nicolas's idea to eliminate a lot of the extra checks, see if you like it
better now.  If not, I can investigate changing the protobufs, but I have to do
that in   cooperation with the sync team.

+      has_coalesced_notification())
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().has_render_info())
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().render_info().has_layout())
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().render_info().layout().has_layout_type())
+    return "";
+
+  const RenderInfo_Layout_LayoutType layout_type =
+      sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().render_info().layout().layout_type();
+
+  // Depending on the layout type, get the proper title.
+  if (RenderInfo_Layout_LayoutType_TITLE_AND_SUBTEXT == layout_type) {
+
+    // If we have title and subtext, get that title.
+    if (!sync_data_.GetSpecifics().synced_notification().
+        coalesced_notification().render_info().layout().
+        has_title_and_subtext_data())
+      return "";
+    if (!sync_data_.GetSpecifics().synced_notification().
+        coalesced_notification().render_info().layout().
+        title_and_subtext_data().has_title())
+      return "";
+
+    // We have a title after all, return it.
+    return sync_data_.GetSpecifics().synced_notification().
+        coalesced_notification().render_info().layout().
+        title_and_subtext_data().title();
+
+  } else if (RenderInfo_Layout_LayoutType_TITLE_AND_IMAGE == layout_type) {
+
+    // If we have title and image, get that title.
+    if (!sync_data_.GetSpecifics().synced_notification().
+        coalesced_notification().render_info().layout().
+        has_title_and_image_data())
+      return "";
+    if (!sync_data_.GetSpecifics().synced_notification().
+        coalesced_notification().render_info().layout().
+        title_and_image_data().has_title())
+      return "";
+
+    // We have a title after all, return it.
+    return sync_data_.GetSpecifics().synced_notification().
+        coalesced_notification().render_info().layout().
+        title_and_image_data().title();
+  } else {
+    // This is an error case, we should never get here unless the protobuf
+    // is bad, or a new type is introduced and this code does not get updated.
+    DCHECK(false);
+    return "";
+  }
+}
+
+const std::string SyncedNotification::app_id() const {

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

Similar comment about incorrect naming (these are not simple accessors) applies.

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

Done.

+
+  if (!sync_data_.GetSpecifics().synced_notification().
+      has_coalesced_notification())
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().has_id())
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().id().
+      has_app_id())
+    return "";
+  return sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().id().app_id();
+}
+
+const std::string SyncedNotification::coalescing_key() const {
+
+  if (!sync_data_.GetSpecifics().synced_notification().
+      has_coalesced_notification())
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().has_id())
+    return "";
+  if (!sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().id().
+      has_coalescing_key())
+    return "";
+  return sync_data_.GetSpecifics().synced_notification().
+      coalesced_notification().id().coalescing_key();
+}
+