Add field to monitor last visited time for each search engine

components/search_engines/template_url_service.cc

 // Copyright 2014 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 "components/search_engines/template_url_service.h"
 
 #include <algorithm>
 #include <utility>
 
 #include "base/auto_reset.h"
@@ skipping 707 matching lines @@
     DefaultSearchManager::Source source;
     const TemplateURLData* new_dse =
         default_search_manager_.GetDefaultSearchEngine(&source);
     // ApplyDefaultSearchChange will notify observers once it is done.
     ApplyDefaultSearchChange(new_dse, source);
   } else {
     NotifyObservers();
   }
 }
 
+bool TemplateURLService::UpdateTemplateURLVisitTime(TemplateURL* url) {
+  TemplateURLData data(url->data());
+  data.last_visited = clock_->Now();
+  const bool updated = UpdateNoNotify(url, TemplateURL(data));
+  if (updated)
+    NotifyObservers();
+  return updated;
+}
+
 void TemplateURLService::AddObserver(TemplateURLServiceObserver* observer) {
   model_observers_.AddObserver(observer);
 }
 
 void TemplateURLService::RemoveObserver(TemplateURLServiceObserver* observer) {
   model_observers_.RemoveObserver(observer);
 }
 
 void TemplateURLService::Load() {
   if (loaded_ || load_handle_ || disable_load_)
@@ skipping 1032 matching lines @@
 void TemplateURLService::UpdateKeywordSearchTermsForURL(
     const URLVisitedDetails& details) {
   if (!details.url.is_valid())
     return;
 
   const TemplateURLSet* urls_for_host =
       provider_map_->GetURLsForHost(details.url.host());
   if (!urls_for_host)
     return;
 
+  TemplateURL* visited_url = nullptr;
   for (TemplateURLSet::const_iterator i = urls_for_host->begin();
        i != urls_for_host->end(); ++i) {
     base::string16 search_terms;
     if ((*i)->ExtractSearchTermsFromURL(details.url, search_terms_data(),
                                         &search_terms) &&
         !search_terms.empty()) {
       if (details.is_keyword_transition) {
         // The visit is the result of the user entering a keyword, generate a
         // KEYWORD_GENERATED visit for the KEYWORD so that the keyword typed
         // count is boosted.
         AddTabToSearchVisit(**i);
       }
       if (client_) {
         client_->SetKeywordSearchTermsForURL(
             details.url, (*i)->id(), search_terms);
       }
+      visited_url = *i;

[Peter Kasting, 2016/12/01 07:38:24]

Why copy this into a temp and then make the call below?  Why not just do that
directly here?

[ltian, 2016/12/01 10:02:58]

Directly call it here will cause the error "attempt to increment a singular
iterator." which seems mean the container of iterator is modified (maybe by
UpdateNoNotify()?). Caching it and calling it later could avoid this problem. 

[Peter Kasting, 2016/12/01 19:32:35]

I understand this now.  You're correct that ultimately UpdateNoNotify will ask
the host-to-url map to remove an entry, which will modify the container as we
touch it.

This can be fixed in a way that will save underlying work.  If
UpdateTemplateURLVisitTime() just changes the last visited time directly
(instead of using UpdateNoNotify()), you should be fine putting that call here. 
This is legal because TemplateURLService is a friend of TemplateURL, and safe
because you're not touching any fields that require the map updates
UpdateNoNotify() performs.

     }
   }
+  if (visited_url != nullptr) {

[Peter Kasting, 2016/12/01 07:38:25]

Nit: No {}

I would elide the "!= nullptr" as well.

[ltian, 2016/12/01 10:02:58]

Done.

+    UpdateTemplateURLVisitTime(visited_url);
+  }
 }
 
 void TemplateURLService::AddTabToSearchVisit(const TemplateURL& t_url) {
   // Only add visits for entries the user hasn't modified. If the user modified
   // the entry the keyword may no longer correspond to the host name. It may be
   // possible to do something more sophisticated here, but it's so rare as to
   // not be worth it.
   if (!t_url.safe_for_autoreplace())
     return;
 
@@ skipping 667 matching lines @@
 
   if (most_recently_intalled_default) {
     base::AutoReset<DefaultSearchChangeOrigin> change_origin(
         &dsp_change_origin_, DSP_CHANGE_OVERRIDE_SETTINGS_EXTENSION);
     default_search_manager_.SetExtensionControlledDefaultSearchEngine(
         most_recently_intalled_default->data());
   } else {
     default_search_manager_.ClearExtensionControlledDefaultSearchEngine();
   }
 }