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 706 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();
   }
 }
 
+void TemplateURLService::UpdateTemplateURLVisitTime(TemplateURL* url) {
+  TemplateURLData data(url->data());
+  data.last_visited = clock_->Now();
+  if(UpdateNoNotify(url, TemplateURL(data)))

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

Nit: There must be a space between "if" and "("; see
http://google.github.io/styleguide/cppguide.html#Conditionals .  (Multiple
places)

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

Done.

+    NotifyObservers();

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

Nit: There are at least a couple other places that do "if (UpdateNoNotify(...))
NotifyObservers();".  Might be worth adding a private helper called Update()
that does this.

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

There are only two places simply doing "if (UpdateNoNotify(...))
NotifyObservers();", the rest places not only call NotifyObservers() but also do
some customized operations in UpdateNoNotify(...), so I am not sure about how to
add an Update() function to handle those cases. 

[Peter Kasting, 2016/11/28 22:20:24]

Hmm, I see four, plus the one you're adding:

https://cs.chromium.org/chromium/src/components/search_engines/template_url_s...
https://cs.chromium.org/chromium/src/components/search_engines/template_url_s...
https://cs.chromium.org/chromium/src/components/search_engines/template_url_s...
https://cs.chromium.org/chromium/src/components/search_engines/template_url_s...

For the second one, your Update() function would need to return a bool so the
caller could still do the conditional MaybeUpdateDSEAfterSync() call.

[ltian, 2016/11/30 00:22:18]

Do you mean my update function also needs to call MaybeUpdateDSEAfterSync()
after calling NotifyObservers()? I think MaybeUpdateDSEAfterSync() is used to
deal with sync data, my update function only handles local update, so is this
function call needed?

[Peter Kasting, 2016/11/30 00:39:37]

No.  The caller would do that.  Your function would need to return a bool
indicating whether an update happened (as UpdateNoNotify() currently does), so
the caller can decide to do that.

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

Note: This whole conversation was about the Update() method I was suggesting,
NOT about UpdateTemplateURLVisitTime().

+}
+
 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 521 matching lines @@
     se_specifics->set_search_url_post_params(turl.search_url_post_params());
   if (!turl.suggestions_url_post_params().empty()) {
     se_specifics->set_suggestions_url_post_params(
         turl.suggestions_url_post_params());
   }
   if (!turl.instant_url_post_params().empty())
     se_specifics->set_instant_url_post_params(turl.instant_url_post_params());
   if (!turl.image_url_post_params().empty())
     se_specifics->set_image_url_post_params(turl.image_url_post_params());
   se_specifics->set_last_modified(turl.last_modified().ToInternalValue());
+  se_specifics->set_last_visited(turl.last_visited().ToInternalValue());

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

I'm confused.  I thought you weren't going to sync this field's value at all in
this CL, and handle all sync issues in a followup CL?

I think it would be less confusing to just not touch anything sync-related here
than to add the field, but only restore it in certain cases.

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

For sync part, there are operations for UPDATE and ADD. For ADD operation, I use
the value from sync data to populate the new object. However, for UPDATE, it
will not update |last_visited| from sync data. 
So what I do in this CL is having the field passed through sync data but not
prevent updating it during synchronization. 

The next CL will add logic to sync separately for |last_visited| dealing with
UPDATE operation.

The reason I do it in this way is I am not quite sure if we add the field
without any sync-related, whether it will crash the app for certain case.
 

[Peter Kasting, 2016/11/28 22:20:24]

How would the app crash?  Can you say more?

I am extremely leery of writing half the sync code and not the rest.  It seems
like this has a higher possibility of putting things into an inconsistent state.

[ltian, 2016/11/30 00:22:18]

So for current code, the |last_visited| will be sent together with other fields
in the sync data. But when populating a TemplateURL from sync data in
CreateTemplateURLFromTemplateURLAndSyncData(), it will only be used when the
|existing_turl| is nullptr, which means there is no matched TemplateURL to be
updated (so a new TemplateURL needs to be created from sync data). The reason I
do in this way is I am not sure if we totally unsynced |last_visited|, what
value is appropriate to set when Chrome finds a new TemplateURL needs to be
created from sync data. And from my understanding, this should not crash the
app?

[Peter Kasting, 2016/11/30 00:39:37]

I would leave it as whatever the default value provided by the constructor is
(i.e. Time()).
I don't see a reason to think things would crash.

   se_specifics->set_sync_guid(turl.sync_guid());
   for (size_t i = 0; i < turl.alternate_urls().size(); ++i)
     se_specifics->add_alternate_urls(turl.alternate_urls()[i]);
   se_specifics->set_search_terms_replacement_key(
       turl.search_terms_replacement_key());
 
   return syncer::SyncData::CreateLocalData(se_specifics->sync_guid(),
                                            se_specifics->keyword(),
                                            specifics);
 }
@@ skipping 53 matching lines @@
   data.input_encodings = base::SplitString(
       specifics.input_encodings(), ";",
       base::TRIM_WHITESPACE, base::SPLIT_WANT_ALL);
   // If the server data has duplicate encodings, we'll want to push an update
   // below to correct it.  Note that we also fix this in
   // GetSearchProvidersUsingKeywordResult(), since otherwise we'd never correct
   // local problems for clients which have disabled search engine sync.
   bool deduped = DeDupeEncodings(&data.input_encodings);
   data.date_created = base::Time::FromInternalValue(specifics.date_created());
   data.last_modified = base::Time::FromInternalValue(specifics.last_modified());
+  // TODO(ltian): sync separtely for this field.
+  // Now if existing_turl is existed, keep last_visited locally without sync.
+  if(existing_turl == nullptr) {
+    data.last_visited = base::Time::FromInternalValue(specifics.last_visited());
+  }
   data.prepopulate_id = specifics.prepopulate_id();
   data.sync_guid = specifics.sync_guid();
   data.alternate_urls.clear();
   for (int i = 0; i < specifics.alternate_urls_size(); ++i)
     data.alternate_urls.push_back(specifics.alternate_urls(i));
   data.search_terms_replacement_key = specifics.search_terms_replacement_key();
 
   std::unique_ptr<TemplateURL> turl(new TemplateURL(data));
   // If this TemplateURL matches a built-in prepopulated template URL, it's
   // possible that sync is trying to modify fields that should not be touched.
@@ skipping 1122 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();
   }
 }