Have URL Modifications sent Regardless of Typed.

URL visit adds and modifications are now noticed regardless of typed count. Filtering is no longer done before making notifications. Clients of the notifications filter on typed count if desired. Also, if a change notification indicates that a page title is now blank, ignore that change in the InMemoryURLIndex.

Renamed NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED to NOTIFICATION_HISTORY_URLS_MODIFIED to better reflect its new purpose.

Reviewers:
  pkasting: Overall general changes.
  sky: Owner review for chrome/browser/history and minor changes in chrome/browser/ui/omnibox.
  akalin: TBR: Owner review for minor changes in chrome/browser/sync.

BUG=102957,87803
TEST=All unit tests pass.
TBR=akalin@chromium.org
Review URL: https://chromiumcodereview.appspot.com/9852002

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@131865 0039d316-1c4b-4281-b951-d872f2087c98

9 files changed, 46 insertions, 59 deletions

diff --git a/chrome/browser/history/expire_history_backend_unittest.cc b/chrome/browser/history/expire_history_backend_unittest.cc
index b545a70..691681f 100644
--- a/chrome/browser/history/expire_history_backend_unittest.cc
+++ b/chrome/browser/history/expire_history_backend_unittest.cc
@@ -391,7 +391,7 @@ void ExpireHistoryTest::EnsureURLInfoGone(const URLRow& row) {
       EXPECT_NE(notifications_[i].first,
                 chrome::NOTIFICATION_HISTORY_URL_VISITED);
       EXPECT_NE(notifications_[i].first,
-                chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED);
+                chrome::NOTIFICATION_HISTORY_URLS_MODIFIED);
     }
   }
   EXPECT_TRUE(found_delete_notification);
diff --git a/chrome/browser/history/history_backend.cc b/chrome/browser/history/history_backend.cc
index fe305f8..7f3bafb 100644
--- a/chrome/browser/history/history_backend.cc
+++ b/chrome/browser/history/history_backend.cc
@@ -874,7 +874,7 @@ void HistoryBackend::AddPagesWithDetails(const URLRows& urls,
   //
   // TODO(brettw) bug 1140015: Add an "add page" notification so the history
   // views can keep in sync.
-  BroadcastNotifications(chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+  BroadcastNotifications(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                          modified.release());
 
   ScheduleCommit();
@@ -889,6 +889,10 @@ void HistoryBackend::SetPageTitle(const GURL& url,
   if (!db_.get())
     return;
 
+  // Update the full text index.
+  if (text_database_.get())
+    text_database_->AddPageTitle(url, title);
+
   // Search for recent redirects which should get the same title. We make a
   // dummy list containing the exact URL visited if there are no redirects so
   // the processing below can be the same.
@@ -908,43 +912,24 @@ void HistoryBackend::SetPageTitle(const GURL& url,
     redirects = &dummy_list;
   }
 
-  bool typed_url_changed = false;
-  URLRows changed_urls;
+  scoped_ptr<URLsModifiedDetails> details(new URLsModifiedDetails);
   for (size_t i = 0; i < redirects->size(); i++) {
     URLRow row;
     URLID row_id = db_->GetRowForURL(redirects->at(i), &row);
     if (row_id && row.title() != title) {
       row.set_title(title);
       db_->UpdateURLRow(row_id, row);
-      changed_urls.push_back(row);
-      if (row.typed_count() > 0)
-        typed_url_changed = true;
+      details->changed_urls.push_back(row);
     }
   }
 
-  // Broadcast notifications for typed URLs that have changed. This will
-  // update the in-memory database.
-  //
-  // TODO(brettw) bug 1140020: Broadcast for all changes (not just typed),
-  // in which case some logic can be removed.
-  if (typed_url_changed) {
-    URLsModifiedDetails* modified =
-        new URLsModifiedDetails;
-    for (size_t i = 0; i < changed_urls.size(); i++) {
-      if (changed_urls[i].typed_count() > 0)
-        modified->changed_urls.push_back(changed_urls[i]);
-    }
-    BroadcastNotifications(chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
-                           modified);
-  }
-
-  // Update the full text index.
-  if (text_database_.get())
-    text_database_->AddPageTitle(url, title);
-
-  // Only bother committing if things changed.
-  if (!changed_urls.empty())
+  // Broadcast notifications for any URLs that have changed. This will
+  // update the in-memory database and the InMemoryURLIndex.
+  if (!details->changed_urls.empty()) {
+    BroadcastNotifications(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
+                           details.release());
     ScheduleCommit();
+  }
 }
 
 void HistoryBackend::AddPageNoVisitForBookmark(const GURL& url) {
diff --git a/chrome/browser/history/in_memory_history_backend.cc b/chrome/browser/history/in_memory_history_backend.cc
index 9d72450..b0dac72 100644
--- a/chrome/browser/history/in_memory_history_backend.cc
+++ b/chrome/browser/history/in_memory_history_backend.cc
@@ -51,7 +51,7 @@ void InMemoryHistoryBackend::AttachToHistoryService(Profile* profile) {
   // We only want notifications for the associated profile.
   content::Source<Profile> source(profile_);
   registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URL_VISITED, source);
-  registrar_.Add(this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+  registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                  source);
   registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_DELETED, source);
   registrar_.Add(this,
@@ -82,7 +82,7 @@ void InMemoryHistoryBackend::Observe(
       OnKeywordSearchTermUpdated(
           *content::Details<history::KeywordSearchTermDetails>(details).ptr());
       break;
-    case chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED:
+    case chrome::NOTIFICATION_HISTORY_URLS_MODIFIED:
       OnTypedURLsModified(
           *content::Details<history::URLsModifiedDetails>(details).ptr());
       break;
@@ -112,12 +112,14 @@ void InMemoryHistoryBackend::OnTypedURLsModified(
   // have Sync(), which would take the ID if it's given and add it.
   URLRows::const_iterator i;
   for (i = details.changed_urls.begin();
-       i != details.changed_urls.end(); i++) {
-    URLID id = db_->GetRowForURL(i->url(), NULL);
-    if (id)
-      db_->UpdateURLRow(id, *i);
-    else
-      id = db_->AddURL(*i);
+       i != details.changed_urls.end(); ++i) {
+    if (i->typed_count() > 0) {
+      URLID id = db_->GetRowForURL(i->url(), NULL);
+      if (id)
+        db_->UpdateURLRow(id, *i);
+      else
+        db_->AddURL(*i);
+    }
   }
 }
 
diff --git a/chrome/browser/history/in_memory_url_index.cc b/chrome/browser/history/in_memory_url_index.cc
index 5fe28e3..de92820 100644
--- a/chrome/browser/history/in_memory_url_index.cc
+++ b/chrome/browser/history/in_memory_url_index.cc
@@ -102,7 +102,7 @@ InMemoryURLIndex::InMemoryURLIndex(Profile* profile,
     // TODO(mrossetti): Register for language change notifications.
     content::Source<Profile> source(profile);
     registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URL_VISITED, source);
-    registrar_.Add(this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+    registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                    source);
     registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_DELETED, source);
   }
@@ -164,7 +164,7 @@ void InMemoryURLIndex::Observe(int notification_type,
     case chrome::NOTIFICATION_HISTORY_URL_VISITED:
       OnURLVisited(content::Details<URLVisitedDetails>(details).ptr());
       break;
-    case chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED:
+    case chrome::NOTIFICATION_HISTORY_URLS_MODIFIED:
       OnURLsModified(
           content::Details<history::URLsModifiedDetails>(details).ptr());
       break;
diff --git a/chrome/browser/sync/glue/typed_url_change_processor.cc b/chrome/browser/sync/glue/typed_url_change_processor.cc
index 6bd7be0..194f822 100644
--- a/chrome/browser/sync/glue/typed_url_change_processor.cc
+++ b/chrome/browser/sync/glue/typed_url_change_processor.cc
@@ -68,16 +68,14 @@ void TypedUrlChangeProcessor::Observe(
 
   DVLOG(1) << "Observed typed_url change.";
   DCHECK(running());
-  DCHECK(chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED == type ||
-         chrome::NOTIFICATION_HISTORY_URLS_DELETED == type ||
-         chrome::NOTIFICATION_HISTORY_URL_VISITED == type);
-  if (type == chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED) {
+  if (type == chrome::NOTIFICATION_HISTORY_URLS_MODIFIED) {
     HandleURLsModified(
         content::Details<history::URLsModifiedDetails>(details).ptr());
   } else if (type == chrome::NOTIFICATION_HISTORY_URLS_DELETED) {
     HandleURLsDeleted(
         content::Details<history::URLsDeletedDetails>(details).ptr());
-  } else if (type == chrome::NOTIFICATION_HISTORY_URL_VISITED) {
+  } else {
+    DCHECK_EQ(chrome::NOTIFICATION_HISTORY_URL_VISITED, type);
     HandleURLsVisited(
         content::Details<history::URLVisitedDetails>(details).ptr());
   }
@@ -89,9 +87,11 @@ void TypedUrlChangeProcessor::HandleURLsModified(
   sync_api::WriteTransaction trans(FROM_HERE, share_handle());
   for (history::URLRows::iterator url = details->changed_urls.begin();
        url != details->changed_urls.end(); ++url) {
-    // Exit if we were unable to update the sync node.
-    if (!CreateOrUpdateSyncNode(*url, &trans))
-      return;
+    if (url->typed_count() > 0) {
+      // Exit if we were unable to update the sync node.
+      if (!CreateOrUpdateSyncNode(*url, &trans))
+        return;
+    }
   }
 }
 
@@ -300,7 +300,7 @@ void TypedUrlChangeProcessor::StartObserving() {
   DCHECK(expected_loop_ == MessageLoop::current());
   DCHECK(profile_);
   notification_registrar_.Add(
-      this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+      this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
       content::Source<Profile>(profile_));
   notification_registrar_.Add(
       this, chrome::NOTIFICATION_HISTORY_URLS_DELETED,
@@ -314,7 +314,7 @@ void TypedUrlChangeProcessor::StopObserving() {
   DCHECK(expected_loop_ == MessageLoop::current());
   DCHECK(profile_);
   notification_registrar_.Remove(
-      this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+      this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
       content::Source<Profile>(profile_));
   notification_registrar_.Remove(
       this, chrome::NOTIFICATION_HISTORY_URLS_DELETED,
diff --git a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
index 6138d3b..7ebe6b5 100644
--- a/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
+++ b/chrome/browser/sync/profile_sync_service_typed_url_unittest.cc
@@ -477,7 +477,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeAdd) {
   history::URLsModifiedDetails details;
   details.changed_urls.push_back(added_entry);
   scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_));
-  notifier->Notify(chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+  notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                    content::Source<Profile>(&profile_),
                    content::Details<history::URLsModifiedDetails>(&details));
 
@@ -507,7 +507,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeAddWithBlank) {
   details.changed_urls.push_back(empty_entry);
   details.changed_urls.push_back(added_entry);
   scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_));
-  notifier->Notify(chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+  notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                    content::Source<Profile>(&profile_),
                    content::Details<history::URLsModifiedDetails>(&details));
 
@@ -544,7 +544,7 @@ TEST_F(ProfileSyncServiceTypedUrlTest, ProcessUserChangeUpdate) {
   history::URLsModifiedDetails details;
   details.changed_urls.push_back(updated_entry);
   scoped_refptr<ThreadNotifier> notifier(new ThreadNotifier(&history_thread_));
-  notifier->Notify(chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+  notifier->Notify(chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                    content::Source<Profile>(&profile_),
                    content::Details<history::URLsModifiedDetails>(&details));
 
diff --git a/chrome/browser/ui/cocoa/history_menu_bridge.mm b/chrome/browser/ui/cocoa/history_menu_bridge.mm
index b166940..d48e2c0 100644
--- a/chrome/browser/ui/cocoa/history_menu_bridge.mm
+++ b/chrome/browser/ui/cocoa/history_menu_bridge.mm
@@ -117,7 +117,7 @@ HistoryMenuBridge::~HistoryMenuBridge() {
   // Unregister ourselves as observers and notifications.
   DCHECK(profile_);
   if (history_service_) {
-    registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+    registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                       content::Source<Profile>(profile_));
     registrar_.Remove(this, chrome::NOTIFICATION_HISTORY_URL_VISITED,
                       content::Source<Profile>(profile_));
@@ -368,7 +368,7 @@ NSMenuItem* HistoryMenuBridge::AddItemToMenu(HistoryItem* item,
 }
 
 void HistoryMenuBridge::Init() {
-  registrar_.Add(this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+  registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                  content::Source<Profile>(profile_));
   registrar_.Add(this, chrome::NOTIFICATION_HISTORY_URL_VISITED,
                  content::Source<Profile>(profile_));
diff --git a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
index 596bfe2..bac517a 100644
--- a/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
+++ b/chrome/browser/ui/omnibox/omnibox_view_browsertest.cc
@@ -333,7 +333,7 @@ class OmniboxViewTest : public InProcessBrowserTest,
     // Wait at least for the AddPageWithDetails() call to finish.
     {
       content::NotificationRegistrar registrar;
-      registrar.Add(this, chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+      registrar.Add(this, chrome::NOTIFICATION_HISTORY_URLS_MODIFIED,
                     content::Source<Profile>(profile));
       ui_test_utils::RunMessageLoop();
       // We don't want to return until all observers have processed this
@@ -376,7 +376,7 @@ class OmniboxViewTest : public InProcessBrowserTest,
       case chrome::NOTIFICATION_AUTOCOMPLETE_CONTROLLER_RESULT_READY:
       case chrome::NOTIFICATION_BOOKMARK_MODEL_LOADED:
       case chrome::NOTIFICATION_HISTORY_LOADED:
-      case chrome::NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED:
+      case chrome::NOTIFICATION_HISTORY_URLS_MODIFIED:
       case chrome::NOTIFICATION_TEMPLATE_URL_SERVICE_LOADED:
         break;
       default:
diff --git a/chrome/common/chrome_notification_types.h b/chrome/common/chrome_notification_types.h
index c612300..86c556a 100644
--- a/chrome/common/chrome_notification_types.h
+++ b/chrome/common/chrome_notification_types.h
@@ -206,14 +206,14 @@ enum NotificationType {
   // HistoryService.
   NOTIFICATION_HISTORY_LOADED,
 
-  // Sent when a URL that has been typed has been added or modified. This is
-  // used by the in-memory URL database (used by autocomplete) to track
+  // Sent when a URL has been added or modified. This is used by the in-memory
+  // URL database and the InMemoryURLIndex (both used by autocomplete) to track
   // changes to the main history system.
   //
   // The source is the profile owning the history service that changed, and
   // the details is history::URLsModifiedDetails that lists the modified or
   // added URLs.
-  NOTIFICATION_HISTORY_TYPED_URLS_MODIFIED,
+  NOTIFICATION_HISTORY_URLS_MODIFIED,
 
   // Sent when the user visits a URL.
   //