Turn on and use the AppInfo data.

chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc

Index: chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
diff --git a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
index 71ad19878ff406ce21adebde32714b620454c8a1..981796922077256960894555e5878832cd0266be 100644
--- a/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
+++ b/chrome/browser/notifications/sync_notifier/chrome_notifier_service.cc
@@ -24,6 +24,9 @@
 #include "chrome/browser/notifications/notification_ui_manager.h"
 #include "chrome/browser/notifications/sync_notifier/chrome_notifier_delegate.h"
 #include "chrome/browser/notifications/sync_notifier/chrome_notifier_service_factory.h"
+#include "chrome/browser/notifications/sync_notifier/synced_notification_app_info.h"
+#include "chrome/browser/notifications/sync_notifier/synced_notification_app_info_service.h"
+#include "chrome/browser/notifications/sync_notifier/synced_notification_app_info_service_factory.h"
 #include "chrome/browser/notifications/sync_notifier/welcome_delegate.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/pref_names.h"
@@ -50,55 +53,6 @@ const char kFirstSyncedNotificationServiceId[] = "Google+";
 const char kSyncedNotificationsWelcomeOrigin[] =
     "synced-notifications://welcome";
 
-// SyncedNotificationAppInfoTemp is a class that contains the information
-// necessary to produce a welcome notification and the app badges for all synced
-// notification.
-// TODO(dewittj): Convert this into a sync protobuf-backed data structure.
-class SyncedNotificationAppInfoTemp {
- public:
-  explicit SyncedNotificationAppInfoTemp(const std::string& app_id,
-                                         const base::string16& service_name);
-  ~SyncedNotificationAppInfoTemp();
-
-  const std::string& app_id() const { return app_id_; }
-  const base::string16& service_name() const { return service_name_; }
-  const base::string16& title() const { return title_; }
-
-  void set_icon(const gfx::Image& icon) { icon_ = icon; }
-  const gfx::Image& icon() const { return icon_; }
-
-  void set_small_icon(const gfx::Image& small_icon) {
-    small_icon_ = small_icon;
-  }
-  const gfx::Image& small_icon() const { return small_icon_; }
-
-  const message_center::NotifierId GetNotifierId() const;
-
- private:
-  std::string app_id_;
-  base::string16 service_name_;
-  gfx::Image icon_;
-  gfx::Image small_icon_;
-  base::string16 title_;
-  base::string16 message_;
-};
-
-SyncedNotificationAppInfoTemp::SyncedNotificationAppInfoTemp(
-    const std::string& app_id,
-    const base::string16& service_name)
-    : app_id_(app_id), service_name_(service_name) {
-  title_ =
-      l10n_util::GetStringFUTF16(IDS_NOTIFIER_WELCOME_TITLE, service_name_);
-}
-
-SyncedNotificationAppInfoTemp::~SyncedNotificationAppInfoTemp() {}
-
-const message_center::NotifierId SyncedNotificationAppInfoTemp::GetNotifierId()
-    const {
-  return message_center::NotifierId(
-      message_center::NotifierId::SYNCED_NOTIFICATION_SERVICE, app_id());
-}
-
 bool ChromeNotifierService::avoid_bitmap_fetching_for_test_ = false;
 
 ChromeNotifierService::ChromeNotifierService(Profile* profile,
@@ -106,19 +60,27 @@ ChromeNotifierService::ChromeNotifierService(Profile* profile,
     : profile_(profile),
       notification_manager_(manager),
       synced_notification_first_run_(false) {
-  SyncedNotificationAppInfoTemp* temp_app_info =
-      new SyncedNotificationAppInfoTemp(
-      kFirstSyncedNotificationServiceId,
-      l10n_util::GetStringUTF16(IDS_FIRST_SYNCED_NOTIFICATION_SERVICE_NAME));
-  temp_app_info->set_small_icon(
-      ui::ResourceBundle::GetSharedInstance().GetImageNamed(
-          IDR_TEMPORARY_GOOGLE_PLUS_ICON));
-  app_info_data_.push_back(temp_app_info);
 
   InitializePrefs();
+
+  // Get a pointer to the app info service so we can get app info data.
+  synced_notification_app_info_service_ =
+      SyncedNotificationAppInfoServiceFactory::GetForProfile(
+          profile_, Profile::EXPLICIT_ACCESS);
+
+  // TODO(petewil): The feature is dead if this fails, how loudly should I
+  // complain?  Should we exit chrome?
+  if (NULL == synced_notification_app_info_service_) {

[dewittj, 2014/03/17 21:43:43]

I think a DCHECK is appropriate here.

[Pete Williamson, 2014/03/21 01:22:31]

Done.

+    LOG(ERROR) << "No Synced Notification App Info Service available.";
+  } else {
+    synced_notification_app_info_service_->SetChromeNotifierService(this);
+  }
 }
 
-ChromeNotifierService::~ChromeNotifierService() {}
+ChromeNotifierService::~ChromeNotifierService() {
+  if (synced_notification_app_info_service_)
+    synced_notification_app_info_service_->SetChromeNotifierService(NULL);
+}
 
 // Methods from BrowserContextKeyedService.
 void ChromeNotifierService::Shutdown() {}
@@ -148,9 +110,7 @@ syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
     scoped_ptr<SyncedNotification> incoming(CreateNotificationFromSyncData(
         sync_data));
     if (!incoming) {
-      // TODO(petewil): Turn this into a NOTREACHED() call once we fix the
-      // underlying problem causing bad data.

[dewittj, 2014/03/17 21:43:43]

What was the underlying problem?

[dewittj, 2014/03/25 22:31:13]

Do you have an answer for this comment?

[Pete Williamson, 2014/03/26 18:12:55]

Oops, my bad, sorry I missed it the first time.  It was on the sync side and had
long since been fixed, I forget the exact cause.

-      LOG(WARNING) << "Badly formed sync data in incoming notification";
+      NOTREACHED();
       continue;
     }
 
@@ -401,13 +361,25 @@ void ChromeNotifierService::GetSyncedNotificationServices(
     std::vector<message_center::Notifier*>* notifiers) {
   // TODO(mukai|petewil): Check the profile's eligibility before adding the

[dewittj, 2014/03/17 21:43:43]

Does this TODO still apply?

[Pete Williamson, 2014/03/21 01:22:31]

Yes.  We have a bug tracking this.  For instance, if the user is not signed up
for G+, we wouldn't show G+ as an option.

   // sample app.
-  ScopedVector<SyncedNotificationAppInfoTemp>::iterator it =
-      app_info_data_.begin();
-  for (; it != app_info_data_.end(); ++it) {
-    SyncedNotificationAppInfoTemp* app_info = *it;
-    message_center::NotifierId notifier_id = app_info->GetNotifierId();
+  std::vector<std::string> sending_service_names;
+
+  if (synced_notification_app_info_service_ == NULL)
+    return;
+
+  sending_service_names =
+      synced_notification_app_info_service_->GetAllSendingServiceNames();
+
+  for (size_t ii = 0; ii < sending_service_names.size(); ++ii) {
+    // Make a notifier ID corresponding to this sending service name.
+    message_center::NotifierId notifier_id = message_center::NotifierId(
+        message_center::NotifierId::SYNCED_NOTIFICATION_SERVICE,
+        sending_service_names[ii]);
 
-    // Enable or disable the sending service per saved settings.
+    SyncedNotificationAppInfo* synced_notification_app_info =
+        synced_notification_app_info_service_->
+            FindSyncedNotificationAppInfoByName(sending_service_names[ii]);
+
+    // Enable or disable the sending service per saved preferences.
     bool app_enabled = false;
     std::set<std::string>::iterator iter;
     iter = find(enabled_sending_services_.begin(),

[dewittj, 2014/03/17 21:43:43]

n^2 search in this array.  Probably do better with YASD (yet another set
difference)

[Pete Williamson, 2014/03/21 01:22:31]

Today the set is small, and it should be for the forseeable future.  I added a
TODO to change the code if the set of sending services becomes large or we
complete third party support.

@@ -416,15 +388,66 @@ void ChromeNotifierService::GetSyncedNotificationServices(
     app_enabled = iter != enabled_sending_services_.end();
 
     message_center::Notifier* app_info_notifier = new message_center::Notifier(
-        notifier_id, app_info->service_name(), app_enabled);
+        notifier_id,
+        base::UTF8ToUTF16(
+            synced_notification_app_info->settings_display_name()),
+        app_enabled);
 
-    app_info_notifier->icon = app_info->small_icon();
+    app_info_notifier->icon = synced_notification_app_info->icon();
 
     // |notifiers| takes ownership of |app_info_notifier|.
     notifiers->push_back(app_info_notifier);
   }
 }
 
+void ChromeNotifierService::SetAddedAppIds(
+    std::vector<std::string> added_app_ids) {
+
+  std::vector<std::string>::const_iterator app_id_iter;
+  for (app_id_iter = added_app_ids.begin(); app_id_iter != added_app_ids.end();
+       ++app_id_iter) {
+    // Make sure this is not a dup, if it is, do nothing.
+    // TODO(petewil): consider a case insensitive compare.
+    std::set<std::string>::iterator sending_service_iter;
+    sending_service_iter = enabled_sending_services_.find(*app_id_iter);
+    if (sending_service_iter != enabled_sending_services_.end())
+      continue;
+
+    // Find any newly enabled notifications and call display on them.
+    // Show the welcome toast if required.
+    ScopedVector<SyncedNotification>::iterator notification_iter;
+    for (notification_iter = notification_data_.begin();
+         notification_iter != notification_data_.end();
+         ++notification_iter) {
+      (*notification_iter)->ShowIfNewlyEnabled(
+          notification_manager_, this, profile_, *app_id_iter);
+    }
+  }
+}
+
+void ChromeNotifierService::SetRemovedAppIds(
+    std::vector<std::string> removed_app_ids) {
+  // Remove from enabled sending services.
+  // Don't remove from initialized sending services. If it gets re-added later,
+  // we want to remember the user's decision, so we also leave prefs intact.
+
+  // Find any displayed notifications and remove them from the notification
+  // center.
+  std::vector<std::string>::const_iterator app_id_iter;
+  for (app_id_iter = removed_app_ids.begin();
+       app_id_iter != removed_app_ids.end();
+       ++app_id_iter) {
+    // Find any newly disabled notifications and remove them.
+    ScopedVector<SyncedNotification>::iterator notification_iter;
+    for (notification_iter = notification_data_.begin();
+         notification_iter != notification_data_.end();
+         ++notification_iter) {
+      (*notification_iter)->HideIfNewlyRemoved(
+          notification_manager_, this, profile_, *app_id_iter);
+    }
+  }
+}
+
 void ChromeNotifierService::MarkNotificationAsRead(
     const std::string& key) {
   DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI));
@@ -452,14 +475,14 @@ void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
   notification_data_.push_back(notification.release());
 
   // If the user is not interested in this type of notification, ignore it.
-  std::set<std::string>::iterator iter =
-      find(enabled_sending_services_.begin(),
-           enabled_sending_services_.end(),
-           notification_copy->GetSendingServiceId());
+  std::string sending_service_id = GetSendingServiceId(notification_copy);
+  std::set<std::string>::iterator iter = find(enabled_sending_services_.begin(),
+                                              enabled_sending_services_.end(),
+                                              sending_service_id);
   if (iter == enabled_sending_services_.end()) {
     iter = find(initialized_sending_services_.begin(),
                 initialized_sending_services_.end(),
-                notification_copy->GetSendingServiceId());
+                sending_service_id);
     if (iter != initialized_sending_services_.end())
       return;
   }
@@ -467,6 +490,19 @@ void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
   UpdateInMessageCenter(notification_copy);
 }
 
+std::string ChromeNotifierService::GetSendingServiceId(
+    const SyncedNotification* synced_notification) {
+  // Get the App Id from the notification, and look it up in the synced
+  // notification app info service.
+  std::string app_id = synced_notification->GetAppId();
+
+  if (synced_notification_app_info_service_ == NULL)
+    return std::string();

[dewittj, 2014/03/17 21:43:43]

Is this ever expected to happen?  Please document the cases.

[Pete Williamson, 2014/03/21 01:22:31]

It should never be able to happen.  I replaced the check with a DCHECK.  It was
left over from an earlier version of the code and not removed.

+
+  return synced_notification_app_info_service_->FindSendingServiceNameFromAppId(
+      app_id);
+}
+
 void ChromeNotifierService::AddForTest(
     scoped_ptr<notifier::SyncedNotification> notification) {
   notification_data_.push_back(notification.release());
@@ -608,7 +644,7 @@ void ChromeNotifierService::DisplayUnreadNotificationsFromSource(
           notification_data_.begin();
        iter != notification_data_.end();
        ++iter) {
-    if ((*iter)->GetSendingServiceId() == notifier_id &&
+    if (GetSendingServiceId(*iter) == notifier_id &&
         (*iter)->GetReadState() == SyncedNotification::kUnread)
       Display(*iter);
   }
@@ -620,7 +656,7 @@ void ChromeNotifierService::RemoveUnreadNotificationsFromSource(
           notification_data_.begin();
        iter != notification_data_.end();
        ++iter) {
-    if ((*iter)->GetSendingServiceId() == notifier_id &&
+    if (GetSendingServiceId(*iter) == notifier_id &&
         (*iter)->GetReadState() == SyncedNotification::kUnread) {
       notification_manager_->CancelById((*iter)->GetKey());
     }
@@ -716,7 +752,7 @@ void ChromeNotifierService::ShowWelcomeToastIfNecessary(
     const SyncedNotification* synced_notification,
     NotificationUIManager* notification_ui_manager) {
   const std::string& sending_service_id =
-      synced_notification->GetSendingServiceId();
+      GetSendingServiceId(synced_notification);
 
   std::set<std::string>::iterator iter;
   iter = find(initialized_sending_services_.begin(),
@@ -731,17 +767,14 @@ void ChromeNotifierService::ShowWelcomeToastIfNecessary(
   // If there is no app info, we can't show a welcome toast.  Ideally all synced
   // notifications will be delayed until an app_info data structure can be
   // constructed for them.
-  // TODO(dewittj): Refactor when app_info is populated asynchronously.
-  SyncedNotificationAppInfoTemp* app_info = FindAppInfo(sending_service_id);
+  notifier::SyncedNotificationAppInfo* app_info =

[dewittj, 2014/03/17 21:43:43]

Does the comment still need addressing?  Is there ever a case when it's possible
for app info not to exist for a notification?

[Pete Williamson, 2014/03/21 01:22:31]

Changed the comment to clarify that we have addressed the issue.  (although to
be fair, there is a bug here, and the bug fix is in my next changelist after
this one).

+      FindAppInfoByAppId(synced_notification->GetAppId());
   if (!app_info)
     return;
 
-  // TODO(dewittj): Ensure that the app info icon is set before this point.
-  if (app_info->icon().IsEmpty()) {
+  if (app_info->settings_icon_url().is_empty()) {
     gfx::Image notification_app_icon = synced_notification->GetAppIcon();
-    if (!notification_app_icon.IsEmpty()) {
-      app_info->set_icon(notification_app_icon);
-    } else {
+    if (notification_app_icon.IsEmpty()) {
       // This block should only be reached in tests since the downloads are
       // already finished for |synced_notification|.
       DVLOG(1) << "Unable to find the app icon for the welcome notification. "
@@ -776,25 +809,23 @@ void ChromeNotifierService::ShowWelcomeToastIfNecessary(
       initialized_sending_services);
 }
 
-SyncedNotificationAppInfoTemp* ChromeNotifierService::FindAppInfo(
+notifier::SyncedNotificationAppInfo* ChromeNotifierService::FindAppInfoByAppId(
     const std::string& app_id) const {
-  ScopedVector<SyncedNotificationAppInfoTemp>::const_iterator iter =
-      app_info_data_.begin();
-  while (iter != app_info_data_.end()) {
-    if ((*iter)->app_id() == app_id)
-      return (*iter);
-
-    ++iter;
-  }
+  if (NULL == synced_notification_app_info_service_)
+    return NULL;
 
-  return NULL;
+  return synced_notification_app_info_service_->
+      FindSyncedNotificationAppInfoByAppId(app_id);
 }
 
 const Notification ChromeNotifierService::CreateWelcomeNotificationForService(
-    SyncedNotificationAppInfoTemp* app_info) {
+    SyncedNotificationAppInfo* app_info) {
   std::string welcome_notification_id = base::GenerateGUID();
-  scoped_refptr<WelcomeDelegate> delegate(new WelcomeDelegate(
-      welcome_notification_id, profile_, app_info->GetNotifierId()));
+  message_center::NotifierId notifier_id = message_center::NotifierId(

[dewittj, 2014/03/17 21:43:43]

Probably should make a SyncedNotificationAppInfo::GetNotifierId() method.

[Pete Williamson, 2014/03/21 01:22:31]

Done.

+      message_center::NotifierId::SYNCED_NOTIFICATION_SERVICE,
+      app_info->settings_display_name());
+  scoped_refptr<WelcomeDelegate> delegate(
+      new WelcomeDelegate(welcome_notification_id, profile_, notifier_id));
 
   message_center::ButtonInfo button_info(
       l10n_util::GetStringUTF16(IDS_NOTIFIER_WELCOME_BUTTON));
@@ -806,11 +837,11 @@ const Notification ChromeNotifierService::CreateWelcomeNotificationForService(
   return Notification(
       message_center::NOTIFICATION_TYPE_BASE_FORMAT,
       GURL(kSyncedNotificationsWelcomeOrigin),
-      app_info->title(),
+      base::UTF8ToUTF16(app_info->settings_display_name()),

[dewittj, 2014/03/17 21:43:43]

Has the encoding of this string been set before hand?  Do we have any way to
internationalize this?

[Pete Williamson, 2014/03/21 01:22:31]

I'm not sure I understand the first question.  The string comes to us in UTF-8,
already localized for this user, so no further internationalization should be
needed.

[dewittj, 2014/03/25 22:31:13]

I was just wondering if the expected encoding is explicitly defined in the
protobuf or other spec.

[Pete Williamson, 2014/03/26 18:12:55]

No, it is not defined in the protobuf, but IIRC, UTF-8 is the default for google
for all protobufs.

       l10n_util::GetStringUTF16(IDS_NOTIFIER_WELCOME_BODY),
       app_info->icon(),
       blink::WebTextDirectionDefault,
-      app_info->GetNotifierId(),
+      notifier_id,
       l10n_util::GetStringUTF16(IDS_NOTIFICATION_WELCOME_DISPLAY_SOURCE),
       base::UTF8ToUTF16(welcome_notification_id),
       rich_notification_data,