Eliminate NOTIFICATION_HISTORY_LOADED notification

chrome/browser/ui/cocoa/history_menu_bridge.mm

 // 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/ui/cocoa/history_menu_bridge.h"
 
 #include "base/bind.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/sys_string_conversions.h"
@@ skipping 47 matching lines @@
 
 HistoryMenuBridge::HistoryItem::~HistoryItem() {
 }
 
 HistoryMenuBridge::HistoryMenuBridge(Profile* profile)
     : controller_([[HistoryMenuCocoaController alloc] initWithBridge:this]),
       profile_(profile),
       history_service_(NULL),
       tab_restore_service_(NULL),
       create_in_progress_(false),
-      need_recreate_(false) {
+      need_recreate_(false),
+      history_service_observer_(this) {
   // If we don't have a profile, do not bother initializing our data sources.
   // This shouldn't happen except in unit tests.
   if (profile_) {
     // Check to see if the history service is ready. Because it loads async, it
     // may not be ready when the Bridge is created. If this happens, register
     // for a notification that tells us the HistoryService is ready.
     HistoryService* hs = HistoryServiceFactory::GetForProfile(
         profile_, Profile::EXPLICIT_ACCESS);
-    if (hs != NULL && hs->BackendLoaded()) {
+    if (hs && hs->BackendLoaded()) {

[Peter Kasting, 2014/11/17 18:45:31]

Nit: Longer, but seems a little clearer to me:

    if (hs) {
      if (hs->BackendLoaded()) {
        history_service_ = hs;
        Init();
      } else {
        history_service_observer_.Add(hs);
      }
    }

[nshaik, 2014/11/17 23:24:29]

Done.

       history_service_ = hs;
       Init();
+    } else if (hs) {
+      history_service_observer_.Add(hs);
     }
 
     tab_restore_service_ = TabRestoreServiceFactory::GetForProfile(profile_);
     if (tab_restore_service_) {
       tab_restore_service_->AddObserver(this);
       // If the tab entries are already loaded, invoke the observer method to
       // build the "Recently Closed" section. Otherwise it will be when the
       // backend loads.
       if (!tab_restore_service_->IsLoaded())
         tab_restore_service_->LoadTabsFromLastSession();
       else
         TabRestoreServiceChanged(tab_restore_service_);
     }
   }
 
   ResourceBundle& rb = ResourceBundle::GetSharedInstance();
   default_favicon_.reset(
       rb.GetNativeImageNamed(IDR_DEFAULT_FAVICON).CopyNSImage());
 
   // Set the static icons in the menu.
   NSMenuItem* item = [HistoryMenu() itemWithTag:IDC_SHOW_HISTORY];
   [item setImage:rb.GetNativeImageNamed(IDR_HISTORY_FAVICON).ToNSImage()];
 
-  // The service is not ready for use yet, so become notified when it does.
-  if (!history_service_) {
-    registrar_.Add(
-        this, chrome::NOTIFICATION_HISTORY_LOADED,
-        content::Source<Profile>(profile_));
-  }
 }
 
 // Note that all requests sent to either the history service or the favicon
 // service will be automatically cancelled by their respective Consumers, so
 // task cancellation is not done manually here in the dtor.
 HistoryMenuBridge::~HistoryMenuBridge() {
   // Unregister ourselves as observers and notifications.
   DCHECK(profile_);
   if (history_service_) {
     registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_URLS_DELETED,
                       content::Source<Profile>(profile_));
-    history_service_->RemoveObserver(this);
-  } else {
-    registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_LOADED,
-                      content::Source<Profile>(profile_));
   }
 
   if (tab_restore_service_)
     tab_restore_service_->RemoveObserver(this);
 
   // Since the map owns the HistoryItems, delete anything that still exists.
   std::map<NSMenuItem*, HistoryItem*>::iterator it = menu_item_map_.begin();
   while (it != menu_item_map_.end()) {
     HistoryItem* item  = it->second;
     menu_item_map_.erase(it++);
     delete item;
   }
 }
 
 void HistoryMenuBridge::Observe(int type,
                                 const content::NotificationSource& source,
                                 const content::NotificationDetails& details) {
-  // A history service is now ready. Check to see if it's the one for the main
-  // profile. If so, perform final initialization.
-  if (type == chrome::NOTIFICATION_HISTORY_LOADED) {
-    HistoryService* hs = HistoryServiceFactory::GetForProfile(
-        profile_, Profile::EXPLICIT_ACCESS);
-    if (hs != NULL && hs->BackendLoaded()) {
-      history_service_ = hs;
-      Init();
-
-      // Found our HistoryService, so stop listening for this notification.
-      registrar_.Remove(this,
-                        chrome::NOTIFICATION_HISTORY_LOADED,
-                        content::Source<Profile>(profile_));
-    }
-  }
-
   // All other notification types that we observe indicate that the history has
   // changed.

[Peter Kasting, 2014/11/17 18:45:32]

Nit: Please update the comment ("other" is now confusing).

[nshaik, 2014/11/17 23:24:29]

Done.

   OnHistoryChanged();
 }
 
 void HistoryMenuBridge::TabRestoreServiceChanged(TabRestoreService* service) {
   const TabRestoreService::Entries& entries = service->entries();
 
   // Clear the history menu before rebuilding.
   NSMenu* menu = HistoryMenu();
   ClearMenuSection(menu, kRecentlyClosed);
 
@@ skipping 107 matching lines @@
                                      const history::RedirectList& redirects,
                                      base::Time visit_time) {
   OnHistoryChanged();
 }
 
 void HistoryMenuBridge::OnURLsModified(HistoryService* history_service,
                                        const history::URLRows& changed_urls) {
   OnHistoryChanged();
 }
 
+void HistoryMenuBridge::OnHistoryServiceLoaded(
+    HistoryService* history_service) {
+  DCHECK(history_service);
+  DCHECK(history_service->BackendLoaded());

[Peter Kasting, 2014/11/17 18:45:31]

Nit: I dunno how valuable these DCHECKs are (doesn't the function name sort of
imply them?)

[nshaik, 2014/11/17 23:24:29]

Done.

+  history_service_ = history_service;
+  Init();
+}
+
 HistoryMenuBridge::HistoryItem* HistoryMenuBridge::HistoryItemForMenuItem(
     NSMenuItem* item) {
   std::map<NSMenuItem*, HistoryItem*>::iterator it = menu_item_map_.find(item);
   if (it != menu_item_map_.end()) {
     return it->second;
   }
   return NULL;
 }
 
 HistoryService* HistoryMenuBridge::service() {
@@ skipping 182 matching lines @@
 }
 
 void HistoryMenuBridge::CancelFaviconRequest(HistoryItem* item) {
   DCHECK(item);
   if (item->icon_requested) {
     cancelable_task_tracker_.TryCancel(item->icon_task_id);
     item->icon_requested = false;
     item->icon_task_id = base::CancelableTaskTracker::kBadTaskId;
   }
 }