Add field to monitor last visited time for each search engine

components/search_engines/template_url_service.cc

Index: components/search_engines/template_url_service.cc
diff --git a/components/search_engines/template_url_service.cc b/components/search_engines/template_url_service.cc
index 5efc50df69b55bdbf1e17f31fa4201d59d861d2d..6626074c08c161812f4628a7e778d63176e7bbb2 100644
--- a/components/search_engines/template_url_service.cc
+++ b/components/search_engines/template_url_service.cc
@@ -724,6 +724,13 @@ void TemplateURLService::RepairPrepopulatedSearchEngines() {
   }
 }
 
+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);
 }
@@ -1265,6 +1272,7 @@ syncer::SyncData TemplateURLService::CreateSyncDataFromTemplateURL(
   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]);
@@ -1338,6 +1346,11 @@ TemplateURLService::CreateTemplateURLFromTemplateURLAndSyncData(
   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();