Turn on and use the AppInfo data.

chrome/browser/notifications/sync_notifier/chrome_notifier_service.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.
 
 // The ChromeNotifierService works together with sync to maintain the state of
 // user notifications, which can then be presented in the notification center,
 // via the Notification UI Manager.
 
 #include "chrome/browser/notifications/sync_notifier/chrome_notifier_service.h"
 
 #include <set>
 #include <string>
 #include <vector>
 
 #include "base/command_line.h"
 #include "base/guid.h"
 #include "base/metrics/histogram.h"
 #include "base/prefs/pref_service.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/values.h"
 #include "chrome/browser/notifications/desktop_notification_service.h"
 #include "chrome/browser/notifications/desktop_notification_service_factory.h"
 #include "chrome/browser/notifications/notification.h"
 #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"
 #include "components/user_prefs/pref_registry_syncable.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/user_metrics.h"
 #include "grit/generated_resources.h"
 #include "grit/theme_resources.h"
 #include "sync/api/sync_change.h"
 #include "sync/api/sync_change_processor.h"
 #include "sync/api/sync_error_factory.h"
 #include "sync/protocol/sync.pb.h"
 #include "sync/protocol/synced_notification_specifics.pb.h"
 #include "third_party/WebKit/public/web/WebTextDirection.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/message_center/notifier_settings.h"
 #include "url/gurl.h"
 
 using base::UserMetricsAction;
 
 namespace notifier {
 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,
                                              NotificationUIManager* manager)
     : 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() {}
 
 // syncer::SyncableService implementation.
 
 // This is called at startup to sync with the server.
 // This code is not thread safe.
 syncer::SyncMergeResult ChromeNotifierService::MergeDataAndStartSyncing(
     syncer::ModelType type,
     const syncer::SyncDataList& initial_sync_data,
     scoped_ptr<syncer::SyncChangeProcessor> sync_processor,
     scoped_ptr<syncer::SyncErrorFactory> error_handler) {
   thread_checker_.CalledOnValidThread();
   DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, type);
   syncer::SyncMergeResult merge_result(syncer::SYNCED_NOTIFICATIONS);
   // A list of local changes to send up to the sync server.
   syncer::SyncChangeList new_changes;
   sync_processor_ = sync_processor.Pass();
 
   for (syncer::SyncDataList::const_iterator it = initial_sync_data.begin();
        it != initial_sync_data.end(); ++it) {
     const syncer::SyncData& sync_data = *it;
     DCHECK_EQ(syncer::SYNCED_NOTIFICATIONS, sync_data.GetDataType());
 
     // Build a local notification object from the sync data.
     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;
     }
 
     // Process each incoming remote notification.
     const std::string& key = incoming->GetKey();
     DCHECK_GT(key.length(), 0U);
     SyncedNotification* found = FindNotificationById(key);
 
     if (NULL == found) {
       // If there are no conflicts, copy in the data from remote.
@@ skipping 228 matching lines @@
     SyncedNotification* notification = *it;
     if (notification_id == notification->GetKey()) {
       notification_data_.erase(it);
       return;
     }
   }
 }
 
 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;
 
-    // Enable or disable the sending service per saved settings.
+  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]);
+
+    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.

                 enabled_sending_services_.end(),
                 notifier_id.id);
     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));
   SyncedNotification* notification = FindNotificationById(key);
   CHECK(notification != NULL);
 
   notification->NotificationHasBeenRead();
   syncer::SyncChangeList new_changes;
 
   syncer::SyncData sync_data = CreateSyncDataFromNotification(*notification);
   new_changes.push_back(
       syncer::SyncChange(FROM_HERE,
                          syncer::SyncChange::ACTION_UPDATE,
                          sync_data));
 
   // Send up the changes that were made locally.
   sync_processor_->ProcessSyncChanges(FROM_HERE, new_changes);
 }
 
 // Add a new notification to our data structure.  This takes ownership
 // of the passed in pointer.
 void ChromeNotifierService::Add(scoped_ptr<SyncedNotification> notification) {
   SyncedNotification* notification_copy = notification.get();
   // Take ownership of the object and put it into our local storage.
   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;
   }
 
   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());
 }
 
 void ChromeNotifierService::UpdateInMessageCenter(
     SyncedNotification* notification) {
   // If the feature is disabled, exit now.
   if (!notifier::ChromeNotifierServiceFactory::UseSyncedNotifications(
       CommandLine::ForCurrentProcess()))
@@ skipping 121 matching lines @@
     list_value->Append(string_value);
   }
 }
 
 void ChromeNotifierService::DisplayUnreadNotificationsFromSource(
     const std::string& notifier_id) {
   for (std::vector<SyncedNotification*>::const_iterator iter =
           notification_data_.begin();
        iter != notification_data_.end();
        ++iter) {
-    if ((*iter)->GetSendingServiceId() == notifier_id &&
+    if (GetSendingServiceId(*iter) == notifier_id &&
         (*iter)->GetReadState() == SyncedNotification::kUnread)
       Display(*iter);
   }
 }
 
 void ChromeNotifierService::RemoveUnreadNotificationsFromSource(
     const std::string& notifier_id) {
   for (std::vector<SyncedNotification*>::const_iterator iter =
           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());
     }
   }
 }
 
 void ChromeNotifierService::OnEnabledSendingServiceListPrefChanged(
     std::set<std::string>* ids_field) {
   ids_field->clear();
   const std::vector<std::string> pref_list =
@@ skipping 75 matching lines @@
   OnInitializedSendingServiceListPrefChanged(&initialized_sending_services_);
 
   synced_notification_first_run_ =
       profile_->GetPrefs()->GetBoolean(prefs::kSyncedNotificationFirstRun);
 }
 
 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(),
               initialized_sending_services_.end(),
               sending_service_id);
 
   // If we already initialized the sending service, then return early since no
   // welcome toast is necessary.
   if (iter != initialized_sending_services_.end())
     return;
 
   // 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. "
                << "Service ID: " << sending_service_id;
     }
   }
 
   message_center::NotifierId notifier_id(
       message_center::NotifierId::SYNCED_NOTIFICATION_SERVICE,
       sending_service_id);
@@ skipping 14 matching lines @@
   profile_->GetPrefs()->Set(prefs::kEnabledSyncedNotificationSendingServices,
                             enabled_sending_services);
   // Mark any new services as initialized in preferences.
   BuildServiceListValueInplace(initialized_sending_services_,
                                &initialized_sending_services);
   profile_->GetPrefs()->Set(
       prefs::kInitializedSyncedNotificationSendingServices,
       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);
+  if (NULL == synced_notification_app_info_service_)
+    return NULL;
 
-    ++iter;
-  }
-
-  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));
   button_info.icon = ui::ResourceBundle::GetSharedInstance().GetImageNamed(
       IDR_NOTIFIER_BLOCK_BUTTON);
 
   message_center::RichNotificationData rich_notification_data;
   rich_notification_data.buttons.push_back(button_info);
   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,
       delegate.get());
 }
 
 }  // namespace notifier