Add field to monitor last visited time for each search engine

components/search_engines/template_url_fetcher.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_fetcher.h"
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 131 matching lines @@
 void TemplateURLFetcher::RequestDelegate::AddSearchProvider() {
   DCHECK(template_url_.get());
   DCHECK(!keyword_.empty());
   TemplateURLService* model = fetcher_->template_url_service_;
   DCHECK(model);
   DCHECK(model->loaded());
 
   TemplateURL* existing_url = NULL;
   if (!model->CanAddAutogeneratedKeyword(keyword_, GURL(template_url_->url()),
                                          &existing_url)) {
+    model->UpdateTemplateURLVisitTime(existing_url);

[Peter Kasting, 2016/11/21 03:35:08]

I don't think the changes in this file are the right place to update the
last_visited time.

Basically, these changes will update the time whenever we would have re-fetched
the TemplateURL.  But I think we want to update instead whenever there's a
history visit that matches the template_url in question.  To implement this,
you'd want to instead modify
TemplateURLService::UpdateKeywordSearchTermsForURL() to update the timestamp.

Behavioral differences, assuming some site like search_engine.com that has a
page with an OSDD and a search box, and a search results page with no OSDD:
* The existing patch would mark search_engine visited if the user navigates to
search_engine.com and then closes the tab; my proposal does not
* The existing patch would not mark search_engine visited if the user uses
search_engine directly from the omnibox; my patch would
* The existing patch would not mark search_engine visited if the user navigates
directly to a search results page on search_engine; my patch would

The second and third bullets above definitely seem like things we should change.
 The first is more arguable, but I would claim the user hasn't actually "used"
search_engine in this case.

[ltian, 2016/11/28 22:08:02]

Thanks for the info, Peter. Originally I was trying to implement in that way but
had no idea this function existed. Modifying
TemplateURLService::UpdateKeywordSearchTermsForURL() is definitely better than
my current implementation.

     fetcher_->RequestCompleted(this);  // WARNING: Deletes us!
     return;
   }
 
   if (existing_url)
     model->Remove(existing_url);
 
   // The short name is what is shown to the user. We preserve original names
   // since it is better when generated keyword in many cases.
   TemplateURLData data(template_url_->data());
@@ skipping 32 matching lines @@
   DCHECK(osdd_url.is_valid());
   DCHECK(!keyword.empty());
 
   if (!template_url_service_->loaded()) {
     // We could try to set up a callback to this function again once the model
     // is loaded but meh.
     template_url_service_->Load();
     return;
   }
 
-  const TemplateURL* template_url =
+  TemplateURL* template_url =
       template_url_service_->GetTemplateURLForKeyword(keyword);
   if (template_url && (!template_url->safe_for_autoreplace() ||
-                       template_url->originating_url() == osdd_url))
+                       template_url->originating_url() == osdd_url)) {
+    template_url_service_->UpdateTemplateURLVisitTime(template_url);
     return;
+  }
 
   // Make sure we aren't already downloading this request.
   for (const auto& request : requests_) {
     if ((request->url() == osdd_url) || (request->keyword() == keyword))
       return;
   }
 
   requests_.push_back(base::MakeUnique<RequestDelegate>(
       this, keyword, osdd_url, favicon_url, url_fetcher_customize_callback));
 }
 
 void TemplateURLFetcher::RequestCompleted(RequestDelegate* request) {
   auto i = std::find_if(requests_.begin(), requests_.end(),
                         [request](const std::unique_ptr<RequestDelegate>& ptr) {
                           return ptr.get() == request;
                         });
   DCHECK(i != requests_.end());
   requests_.erase(i);
 }