[Android] Sort custom search engines based on last visited time and display only top 3 most recentl…

[Android] Sort custom search engines based on last visited time and display only top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360
Committed: https://crrev.com/dffd7c39dddd7ee52ffde3b877a237b5b877a252
Cr-Commit-Position: refs/heads/master@{#439251}

[ltian, 2016-12-05 20:00:25]

The CQ bit was checked by ltian@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-05 20:01:07]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-05 21:01:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-05 21:01:31]

Dry run: This issue passed the CQ dry run.

[ltian, 2016-12-05 22:02:58]

ltian@chromium.org changed reviewers:
+ pkasting@chromium.org

[ltian, 2016-12-05 22:02:59]

Can you take a look of this CL?

Thanks!

[Peter Kasting, 2016-12-06 06:24:55]

The comments I've left below are fairly sketchy.  To implement them, you will
need to learn more about the relevant <algorithm> calls I refer to and think how
they could fit in to what you're trying to do.  Do your best to determine how to
make use of these before you decide to fall back on asking me :)

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:161:
std::sort(template_urls_.begin(), template_urls_.end(), comp);
The sort comparator is getting complicated.

I wonder if it would be better to use a couple of calls to std::partition() to
do this.  This would avoid the need to count the elements in the "recently
visited" list at all, since you would be able to compute the size of that
partition directly.  You would still need a sort() call for the first partition
in this case.

Or maybe a sort() with "prepopulated before non-prepopulated, sorted by ID",
followed by a partition() call.

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:164:
template_urls_.end());
Nit: Use resize()

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:174: }
Nit: Use std::count_if

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:175: return
num_custom_engine > 5 ? num_custom_engine - 5 : 0;
Nit: Use std::max

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_android.h (right):

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.h:92: // then other
non-prepopulated engines based on last_visited in the
Nit: remove "the"

[ltian, 2016-12-06 22:37:43]

The CQ bit was checked by ltian@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-06 22:38:33]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 00:28:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 00:28:27]

Dry run: This issue passed the CQ dry run.

[ltian, 2016-12-07 00:56:43]

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:161:
std::sort(template_urls_.begin(), template_urls_.end(), comp);
On 2016/12/06 06:24:55, Peter Kasting wrote:
> The sort comparator is getting complicated.
> 
> I wonder if it would be better to use a couple of calls to std::partition() to
> do this.  This would avoid the need to count the elements in the "recently
> visited" list at all, since you would be able to compute the size of that
> partition directly.  You would still need a sort() call for the first
partition
> in this case.
> 
> Or maybe a sort() with "prepopulated before non-prepopulated, sorted by ID",
> followed by a partition() call.

Done.

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:164:
template_urls_.end());
On 2016/12/06 06:24:55, Peter Kasting wrote:
> Nit: Use resize()

Done.

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:174: }
On 2016/12/06 06:24:55, Peter Kasting wrote:
> Nit: Use std::count_if

Done.

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.cc:175: return
num_custom_engine > 5 ? num_custom_engine - 5 : 0;
On 2016/12/06 06:24:55, Peter Kasting wrote:
> Nit: Use std::max

Done.

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
File chrome/browser/search_engines/template_url_service_android.h (right):

https://codereview.chromium.org/2555513003/diff/1/chrome/browser/search_engin...
chrome/browser/search_engines/template_url_service_android.h:92: // then other
non-prepopulated engines based on last_visited in the
On 2016/12/06 06:24:55, Peter Kasting wrote:
> Nit: remove "the"

Done.

[Peter Kasting, 2016-12-07 22:02:01]

Quick thoughts as I have to run: I think a larger change is needed here.

Naively, it seems like ShowInDefaultList() should be changed so it returns true
for the most recently-visited engines.  This way desktop also gets the
beneficial behavior of putting "likely defaults" in the top list, and the
Android behavior is basically "the desktop behavior, except we're only showing
the bottom list".

If this is done, I think TemplateURLService::CanReplace() probably shouldn't
rely on ShowInDefaultList() to do what it's trying to do.  I'm not sure if that
check should just be removed, or what.  The question is what functionality that
function is used to gate.

Less naively, maybe ShowInDefaultList() isn't the right way to get at this. 
Maybe TemplateURLService itself needs a function to return the list of URLs in
"the default list"?

Another question is whether the right implementation is to sort the template
URLs by visit time and pick the top n, or to keep a pqueue of template URLs,
sorted by most recently visited, so that at any time we can easily read off the
top n, or similar.  Depends on access patterns.

Another question is whether "5 most recently visited non prepopulated
non-default engines" is the desirable behavior.  That seems like too many.  It
seems like "any engine used in the last <recent time period>, limit to <n>" (for
"2 days" and "3" maybe?) would be better.

[Peter Kasting, 2016-12-08 06:56:27]

This needs tests.

https://codereview.chromium.org/2555513003/diff/40001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/40001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:173: }
Functionally, this looks like it will work.

Here's how I would write this (note: did not compile, test, or git cl format
this; do those if you use this).  Afterwards, I'll explain the differences.

  template_urls_ = template_url_service_->GetTemplateURLs();

  // Move prepopulated and policy-created engines to the front of the list, and
  // sort by prepopulate_id.
  TemplateURLService* template_url_service = template_url_service_;
  auto it = std::partition(
      template_urls_.begin(), template_urls_.end(),
      [template_url_service](const TemplateURL* turl) {
        return template_url_service->IsPrepopulatedOrCreatedByPolicy(turl);
      });
  std::sort(template_urls_.begin(), it,
            [](const TemplateURL* lhs, const TemplateURL* rhs) {
              return lhs->prepopulate_id() < rhs->prepopulate_id();
            });

  // Place any user-selected default engine next.
  TemplateURL* dsp = template_url_service_->GetDefaultSearchProvider();
  it = std::partition(it, template_urls_.end(),
                      [dsp](const TemplateURL* turl) { return turl == dsp; });

  // Sort the remaining engines with the three most recently-visited first.
  constexpr size_t kMaxRecentUrls = 3;
  const auto end = it + std::min(template_urls_.end() - it, kMaxRecentUrls);
  std::partial_sort(it, end, template_urls_.end(),
                    [](const TemplateURL* lhs, const TemplateURL* rhs) {
                      return lhs->last_visited() > rhs->last_visited();
                    });

  // Limit to those engines, which must also have been visited in the last two
  // days.
  constexpr base::TimeDelta kMaxVisitAge = base::TimeDelta::FromDays(2);
  const base::Time cutoff = base::Time::Now() - kMaxVisitAge;
  const auto too_old =
      [cutoff](const TemplateURL* url) { return url->last_visited() < cutoff; };
  std::erase(std::find_if(it, end, too_old), template_urls_.end());

Differences:
* More comments.  In a function this complicated, individual blocks should be
separated by whitespace and given comments to explain briefly what they're
doing.
* Inlined most lambdas into algorithm calls.  Now that these lambdas are
generally short, doing this can make code shorter and doesn't usually hamper
readability.  I did pull |too_old|, near the end, out to its own function to
avoid a bunch of linebreaking that would have been needed otherwise.
* Minimized captures in lambdas.  In particular, used |template_url_service| to
avoid having to capture |this| in the first lambda, which limits the scope of
what it can touch.
* Used second partition() call to simplify comparator for sorting first section.
* Named constexprs for things like the "3 urls" and "2 days" limits.  Pulling
these out to constants makes them visible and as easy as possible to change
safely later.
* Replaced a sort() with partial_sort() to avoid sorting elements we'll throw
away, for efficiency.
* Eliminated ToTimeT() calls.  Time values can be compared without these, so
they're just unnecessary work.
* Pulled |cutoff| calculation out to a const variable.  Not only does this save
work, it means the behavior will be consistent; otherwise, as we iterate through
the loop, we'll keep calling Now() repeatedly, which will keep changing
slightly.
* Renamed |isRecent| to |too_old|.  Chromium C++ does not use camelCase variable
names, including for lambdas stored as variables, and "is recent" is the
opposite of what the lambda actually returns.
* Added "const" to the parameter for |too_old|.
* Replaced remove_if() with find_if() since the list is already sorted, so we
know everything after the first "too old" engine is also too old.
* Performed find_if() only on the elements that wouldn't be discarded by
kMaxRecentUrls, for efficiency.
* Combined erase() and resize() calls for efficiency.
* Eliminated CountRemovedSearchEngineNum().  This function is pretty simple, and
pulling it out separately mostly just adds more verbosity without making things
more readable, IMO.

[ltian, 2016-12-08 20:27:06]

The CQ bit was checked by ltian@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-08 20:28:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 22:15:05]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 22:15:05]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[ltian, 2016-12-09 00:53:08]

https://codereview.chromium.org/2555513003/diff/40001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/40001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:173: }
On 2016/12/08 06:56:27, Peter Kasting wrote:
> Functionally, this looks like it will work.
> 
> Here's how I would write this (note: did not compile, test, or git cl format
> this; do those if you use this).  Afterwards, I'll explain the differences.
> 
>   template_urls_ = template_url_service_->GetTemplateURLs();
> 
>   // Move prepopulated and policy-created engines to the front of the list,
and
>   // sort by prepopulate_id.
>   TemplateURLService* template_url_service = template_url_service_;
>   auto it = std::partition(
>       template_urls_.begin(), template_urls_.end(),
>       [template_url_service](const TemplateURL* turl) {
>         return template_url_service->IsPrepopulatedOrCreatedByPolicy(turl);
>       });
>   std::sort(template_urls_.begin(), it,
>             [](const TemplateURL* lhs, const TemplateURL* rhs) {
>               return lhs->prepopulate_id() < rhs->prepopulate_id();
>             });
> 
>   // Place any user-selected default engine next.
>   TemplateURL* dsp = template_url_service_->GetDefaultSearchProvider();
>   it = std::partition(it, template_urls_.end(),
>                       [dsp](const TemplateURL* turl) { return turl == dsp; });
> 
>   // Sort the remaining engines with the three most recently-visited first.
>   constexpr size_t kMaxRecentUrls = 3;
>   const auto end = it + std::min(template_urls_.end() - it, kMaxRecentUrls);
>   std::partial_sort(it, end, template_urls_.end(),
>                     [](const TemplateURL* lhs, const TemplateURL* rhs) {
>                       return lhs->last_visited() > rhs->last_visited();
>                     });
> 
>   // Limit to those engines, which must also have been visited in the last two
>   // days.
>   constexpr base::TimeDelta kMaxVisitAge = base::TimeDelta::FromDays(2);
>   const base::Time cutoff = base::Time::Now() - kMaxVisitAge;
>   const auto too_old =
>       [cutoff](const TemplateURL* url) { return url->last_visited() < cutoff;
};
>   std::erase(std::find_if(it, end, too_old), template_urls_.end());
> 
> Differences:
> * More comments.  In a function this complicated, individual blocks should be
> separated by whitespace and given comments to explain briefly what they're
> doing.
> * Inlined most lambdas into algorithm calls.  Now that these lambdas are
> generally short, doing this can make code shorter and doesn't usually hamper
> readability.  I did pull |too_old|, near the end, out to its own function to
> avoid a bunch of linebreaking that would have been needed otherwise.
> * Minimized captures in lambdas.  In particular, used |template_url_service|
to
> avoid having to capture |this| in the first lambda, which limits the scope of
> what it can touch.
> * Used second partition() call to simplify comparator for sorting first
section.
> * Named constexprs for things like the "3 urls" and "2 days" limits.  Pulling
> these out to constants makes them visible and as easy as possible to change
> safely later.
> * Replaced a sort() with partial_sort() to avoid sorting elements we'll throw
> away, for efficiency.
> * Eliminated ToTimeT() calls.  Time values can be compared without these, so
> they're just unnecessary work.
> * Pulled |cutoff| calculation out to a const variable.  Not only does this
save
> work, it means the behavior will be consistent; otherwise, as we iterate
through
> the loop, we'll keep calling Now() repeatedly, which will keep changing
> slightly.
> * Renamed |isRecent| to |too_old|.  Chromium C++ does not use camelCase
variable
> names, including for lambdas stored as variables, and "is recent" is the
> opposite of what the lambda actually returns.
> * Added "const" to the parameter for |too_old|.
> * Replaced remove_if() with find_if() since the list is already sorted, so we
> know everything after the first "too old" engine is also too old.
> * Performed find_if() only on the elements that wouldn't be discarded by
> kMaxRecentUrls, for efficiency.
> * Combined erase() and resize() calls for efficiency.
> * Eliminated CountRemovedSearchEngineNum().  This function is pretty simple,
and
> pulling it out separately mostly just adds more verbosity without making
things
> more readable, IMO.

That's a great tutorial! Thanks for the great effort, Peter!

[Peter Kasting, 2016-12-09 01:26:56]

This still needs tests.  A test would have caught the functional error below, I
think.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:145:
TemplateURLService* template_url_serivce = template_url_service_;
Nit: Spelling: serivce -> service

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:147: // and sort
by prepopulated_id.
Nit: I would place this comment above the declaration of the temp above, since
that temp is local to this block.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:158: // Place
user-selected default engine next.
Nit: I suggest saying "any" after "Place" since it implies there may not be one;
the existing comment sounds more like there will always be one.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:163: // Sort the
remaining engines with the three most recently-visited first.
Nit: with -> to place

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:165: const size_t
recentUrls = std::distance(it+1, template_urls_.end());
It's not safe to do +1 here, since |it| may already point to end().

It's also functionally incorrect, AFAICT.  It's off-by-one.

There's no need to use std::distance() with a vector; using math operators is
just as efficient, and more readable.

Style-wise: don't use camelCase naming; spaces around operators.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:168: [](const
TemplateURL* lhs, const TemplateURL* rhs) {
Nit: This seems unlikely to be what git cl format would do?  The indenting
follows no rule I can determine.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:172: // Limit to
those engines which must be visited in the last two days.
Nit: Better (sorry, my original was bad): "Limit to those three engines, which
must also have been visited..."

[ltian, 2016-12-09 02:42:44]

On 2016/12/09 01:26:56, Peter Kasting wrote:
> This still needs tests.  A test would have caught the functional error below,
I
> think.
> 

Do you means I need to write some tests for my change? Should I write some unit
tests for c++ or Java?

>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> File chrome/browser/search_engines/template_url_service_android.cc (right):
> 
>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_android.cc:145:
> TemplateURLService* template_url_serivce = template_url_service_;
> Nit: Spelling: serivce -> service
> 
>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_android.cc:147: // and sort
> by prepopulated_id.
> Nit: I would place this comment above the declaration of the temp above, since
> that temp is local to this block.
> 
>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_android.cc:158: // Place
> user-selected default engine next.
> Nit: I suggest saying "any" after "Place" since it implies there may not be
one;
> the existing comment sounds more like there will always be one.
> 
>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_android.cc:163: // Sort the
> remaining engines with the three most recently-visited first.
> Nit: with -> to place
> 
>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_android.cc:165: const
size_t
> recentUrls = std::distance(it+1, template_urls_.end());
> It's not safe to do +1 here, since |it| may already point to end().
> 
> It's also functionally incorrect, AFAICT.  It's off-by-one.
> 
> There's no need to use std::distance() with a vector; using math operators is
> just as efficient, and more readable.
> 
> Style-wise: don't use camelCase naming; spaces around operators.
> 
>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_android.cc:168: [](const
> TemplateURL* lhs, const TemplateURL* rhs) {
> Nit: This seems unlikely to be what git cl format would do?  The indenting
> follows no rule I can determine.
> 
>
https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
> chrome/browser/search_engines/template_url_service_android.cc:172: // Limit to
> those engines which must be visited in the last two days.
> Nit: Better (sorry, my original was bad): "Limit to those three engines, which
> must also have been visited..."

[Peter Kasting, 2016-12-09 02:44:57]

On 2016/12/09 02:42:44, ltian wrote:
> On 2016/12/09 01:26:56, Peter Kasting wrote:
> > This still needs tests.  A test would have caught the functional error
below,
> I
> > think.
> > 
> 
> Do you means I need to write some tests for my change? Should I write some
unit
> tests for c++ or Java?

Yes, I think you should write C++ unittests for the
template_url_service_android.cc code, at least the stuff you're touching here.

[ltian, 2016-12-09 02:53:54]

On 2016/12/09 02:44:57, Peter Kasting wrote:
> On 2016/12/09 02:42:44, ltian wrote:
> > On 2016/12/09 01:26:56, Peter Kasting wrote:
> > > This still needs tests.  A test would have caught the functional error
> below,
> > I
> > > think.
> > > 
> > 
> > Do you means I need to write some tests for my change? Should I write some
> unit
> > tests for c++ or Java?
> 
> Yes, I think you should write C++ unittests for the
> template_url_service_android.cc code, at least the stuff you're touching here.

Could you point out any reference about unittest for Android native C++? I know
in
Java there are some unittests for correctness of some functions in this class,
is
that ok? 

And do you have any idea which case I should cover? I guess I should test
whether they are sorted correctly and limit to be shown maximum 3 items. But is
there any other case I should also test for?

[Peter Kasting, 2016-12-09 03:04:47]

On 2016/12/09 02:53:54, ltian wrote:
> On 2016/12/09 02:44:57, Peter Kasting wrote:
> > On 2016/12/09 02:42:44, ltian wrote:
> > > On 2016/12/09 01:26:56, Peter Kasting wrote:
> > > > This still needs tests.  A test would have caught the functional error
> > below,
> > > I
> > > > think.
> > > > 
> > > 
> > > Do you means I need to write some tests for my change? Should I write some
> > unit
> > > tests for c++ or Java?
> > 
> > Yes, I think you should write C++ unittests for the
> > template_url_service_android.cc code, at least the stuff you're touching
here.
> 
> Could you point out any reference about unittest for Android native C++? I
know
> in
> Java there are some unittests for correctness of some functions in this class,
> is
> that ok?

Sorry, I don't have a reference for anything Android-specific.  However, this is
basically just C++ code living in the Chromium tree, and could presumably be
tested similarly to other Chromium C++.  Note that
template_url_service_unittest.cc in the same directory tests the cross-platform
base class.  I would assume you could make a simple
template_url_service_android_unittest.cc file.  Codesearch says there are a
couple dozen files called "_android_unittest.cc" in the codebase, so you could
look at them.

> And do you have any idea which case I should cover? I guess I should test
> whether they are sorted correctly and limit to be shown maximum 3 items. But
is
> there any other case I should also test for?

Ideally, unittests provide full code coverage.  So look for the various
conditions and different functions in the code and try to make sure you exercise
each one, along with ways any interact.  (For example, test that the
non-prepopulated section can contain more than three URLs if the user's default
engine is not prepopulated.)

Another way to do this is to think like an adversary (or a security engineer):
if you were trying to break this code and turn up all the obscure, edge-case
bugs that the original engineer hadn't thought of, what devious tests could you
devise to do it?

[ltian, 2016-12-15 01:54:52]

Description was changed from

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on thier
last visited times and display only top 5 most recently visited engines.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 5 engines return false for ShowInDefaultList().

BUG=348360
==========

to

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360
==========

[ltian, 2016-12-15 01:56:38]

Description was changed from

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360
==========

to

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360
==========

[ltian, 2016-12-15 01:56:53]

The CQ bit was checked by ltian@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 01:57:04]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 02:04:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 02:04:27]

Dry run: Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)

[Ted C, 2016-12-15 17:44:16]

The CQ bit was checked by tedchoc@chromium.org

[commit-bot: I haz the power, 2016-12-15 17:44:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 17:44:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 17:44:40]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[ltian, 2016-12-15 17:51:41]

The CQ bit was checked by ltian@chromium.org

[ltian, 2016-12-15 17:51:51]

The CQ bit was unchecked by ltian@chromium.org

[ltian, 2016-12-15 17:52:19]

The CQ bit was checked by ltian@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-15 17:52:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-15 18:32:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-15 18:32:54]

Dry run: This issue passed the CQ dry run.

[Ted C, 2016-12-15 18:47:53]

On 2016/12/15 17:44:40, commit-bot: I haz the power wrote:
> No L-G-T-M from a valid reviewer yet. 
> CQ run can only be started by full committers or once the patch has
> received an L-G-T-M from a full committer.
> Even if an L-G-T-M may have been provided, it was from a non-committer,
> _not_ a full super star committer.
> See http://www.chromium.org/getting-involved/become-a-committer
> Note that this has nothing to do with OWNERS files.

Ah, that was my bad.  Clicked commit instead of dry-run.

[ltian, 2016-12-15 19:02:35]

tedchoc@chromiu.org: could you take a look for the Java changes of my CL?

Thanks!

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:145:
TemplateURLService* template_url_serivce = template_url_service_;
On 2016/12/09 01:26:56, Peter Kasting wrote:
> Nit: Spelling: serivce -> service

Done.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:147: // and sort
by prepopulated_id.
On 2016/12/09 01:26:56, Peter Kasting wrote:
> Nit: I would place this comment above the declaration of the temp above, since
> that temp is local to this block.

Done.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:158: // Place
user-selected default engine next.
On 2016/12/09 01:26:56, Peter Kasting wrote:
> Nit: I suggest saying "any" after "Place" since it implies there may not be
one;
> the existing comment sounds more like there will always be one.

Done.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:163: // Sort the
remaining engines with the three most recently-visited first.
On 2016/12/09 01:26:55, Peter Kasting wrote:
> Nit: with -> to place

Done.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:165: const size_t
recentUrls = std::distance(it+1, template_urls_.end());
On 2016/12/09 01:26:56, Peter Kasting wrote:
> It's not safe to do +1 here, since |it| may already point to end().
> 
> It's also functionally incorrect, AFAICT.  It's off-by-one.
> 
> There's no need to use std::distance() with a vector; using math operators is
> just as efficient, and more readable.
> 
> Style-wise: don't use camelCase naming; spaces around operators.

Done.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:168: [](const
TemplateURL* lhs, const TemplateURL* rhs) {
On 2016/12/09 01:26:55, Peter Kasting wrote:
> Nit: This seems unlikely to be what git cl format would do?  The indenting
> follows no rule I can determine.

Done.

https://codereview.chromium.org/2555513003/diff/60001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:172: // Limit to
those engines which must be visited in the last two days.
On 2016/12/09 01:26:56, Peter Kasting wrote:
> Nit: Better (sorry, my original was bad): "Limit to those three engines, which
> must also have been visited..."

Done.

[Ted C, 2016-12-15 21:38:06]

https://codereview.chromium.org/2555513003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java
(right):

https://codereview.chromium.org/2555513003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:175:
assertEquals(7, searchEngines.size());
I assume the 7 comes from the fact that there are 5 prepopulated engines right?

If so, I would get the initial size before adding any new search engines.  Then
I would track the delta (here's you are expecting two new search engines only). 
If not, adding or removing a default search engine (or if your phone is in a
different region), the test might not pass.

Or, maybe it would be better do use this:
https://developer.android.com/reference/java/util/List.html#subList(int, int)
and just return the search engines after the pre-populated ones.

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:327:
base::Time::Now() - base::TimeDelta::FromDays((int)offset);
space between (int) and offset (and below).

Also, I think offset isn't the clearest name.  I would probably name it
something like age_in_days.

[ltian, 2016-12-15 22:56:54]

https://codereview.chromium.org/2555513003/diff/80001/chrome/android/javatest...
File
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java
(right):

https://codereview.chromium.org/2555513003/diff/80001/chrome/android/javatest...
chrome/android/javatests/src/org/chromium/chrome/browser/search_engines/TemplateUrlServiceTest.java:175:
assertEquals(7, searchEngines.size());
On 2016/12/15 21:38:06, Ted C wrote:
> I assume the 7 comes from the fact that there are 5 prepopulated engines
right?
> 
> If so, I would get the initial size before adding any new search engines. 
Then
> I would track the delta (here's you are expecting two new search engines
only). 
> If not, adding or removing a default search engine (or if your phone is in a
> different region), the test might not pass.
> 
> Or, maybe it would be better do use this:
> https://developer.android.com/reference/java/util/List.html#subList(int, int)
> and just return the search engines after the pre-populated ones.

Return the sublist after pre-populated ones is a great idea, change to it!

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:327:
base::Time::Now() - base::TimeDelta::FromDays((int)offset);
On 2016/12/15 21:38:06, Ted C wrote:
> space between (int) and offset (and below).
> 
> Also, I think offset isn't the clearest name.  I would probably name it
> something like age_in_days.

Done.

[Ted C, 2016-12-15 23:40:37]

lgtm

https://codereview.chromium.org/2555513003/diff/100001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(right):

https://codereview.chromium.org/2555513003/diff/100001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:378:
public String addSearchEngineForTesting(String keyword, int offset) {
update the java var names too

[Peter Kasting, 2016-12-16 08:37:18]

LGTM

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
File chrome/browser/search_engines/template_url_service_android.cc (right):

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:174: // Limit to
those three engines which must also have been visited in the last
Nit: Comma after "engines", or the sentence implies that there must be three
engines that have been so visited.

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:322:
data.favicon_url = GURL("http://favicon.url");
Nit: Why a dot here, but not in "testurl"?  It makes it look as if the
distinction matters.

https://codereview.chromium.org/2555513003/diff/80001/chrome/browser/search_e...
chrome/browser/search_engines/template_url_service_android.cc:327:
base::Time::Now() - base::TimeDelta::FromDays((int)offset);
Don't compute Now() - TimeDelta three times.  This will give three subtly
different values, and is inefficient.

Compute this once, then do e.g. "data.last_modified = data.date_created;" and
similar below.

On 2016/12/15 21:38:06, Ted C wrote:
> space between (int) and offset (and below).

C-style casts are banned entirely (see
http://google.github.io/styleguide/cppguide.html#Casting ); use
static_cast<int>(offset) if a cast is needed.

https://codereview.chromium.org/2555513003/diff/100001/chrome/browser/search_...
File chrome/browser/search_engines/template_url_service_android.h (right):

https://codereview.chromium.org/2555513003/diff/100001/chrome/browser/search_...
chrome/browser/search_engines/template_url_service_android.h:79:
base::android::ScopedJavaLocalRef<jstring> AddSearchEngineForTesting(
Nit: At least for new methods you're adding, but ideally for all of these
methods, give each function a comment indicating what it does and what the
parameters are.

https://codereview.chromium.org/2555513003/diff/100001/components/search_engi...
File components/search_engines/template_url_service.h (right):

https://codereview.chromium.org/2555513003/diff/100001/components/search_engi...
components/search_engines/template_url_service.h:382: void
UpdateTemplateURLVisitTime(TemplateURL* url);
Nit: If you're going to move this, you need to move the definition to match. 
.cc and .h files must stay in the same order.

Consider also where in this list this should go; try to group related things. 
In this case, you've inserted your method right between two "create XXX" static
methods for sync that are closely related to each other, which doesn't make
sense.

Looking at how you use these, it's merely being exposed to a test-only function
on TemplateUrlServiceAndroid, which is almost an implementation-detail class. 
It probably makes sense to leave it where it was and expose via a friend
declaration instead, since we don't really want anyone else calling this method.

[ltian, 2016-12-16 20:19:04]

The CQ bit was checked by ltian@chromium.org

[ltian, 2016-12-16 20:19:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2555513003/#ps120001
(title: "Update based on Peter's comments.")

[commit-bot: I haz the power, 2016-12-16 20:20:11]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-16 20:24:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-16 20:24:11]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[ltian, 2016-12-16 22:53:30]

The CQ bit was checked by ltian@chromium.org

[ltian, 2016-12-16 22:53:30]

The patchset sent to the CQ was uploaded after l-g-t-m from
tedchoc@chromium.org, pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/2555513003/#ps140001
(title: "Fix the conflicts")

[commit-bot: I haz the power, 2016-12-16 22:54:39]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-17 00:23:31]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1481928810031410,
"parent_rev": "9e1fbd19ad0f608da75b9e44c41b8df86b49fe67", "commit_rev":
"572a37dc2369d309e80f5e53dbe08c3fb83ba383"}

[commit-bot: I haz the power, 2016-12-17 00:24:01]

Description was changed from

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360
==========

to

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360

Review-Url: https://codereview.chromium.org/2555513003
==========

[commit-bot: I haz the power, 2016-12-17 00:24:02]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-12-17 00:28:43]

Description was changed from

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360

Review-Url: https://codereview.chromium.org/2555513003
==========

to

==========
[Android] Sort custom search engines based on last visited time and display only
top 5 most recently visited engines

On Android, all visited custom search engines are shown in the same
order provided by TemplateULService.

New design wants to sort the custom search engines based on their
last visited times and display only top 3 most recently visited engines
within 2 days.

Add logic in LoadTemplateURLs() to sort custom search engines based on
the last_visited field and remove redundant engines if there are more
than 3 engines return false for ShowInDefaultList() and visited within
2 days.

BUG=348360

Committed: https://crrev.com/dffd7c39dddd7ee52ffde3b877a237b5b877a252
Cr-Commit-Position: refs/heads/master@{#439251}
==========

[commit-bot: I haz the power, 2016-12-17 00:28:44]

Patchset 8 (id:??) landed as
https://crrev.com/dffd7c39dddd7ee52ffde3b877a237b5b877a252
Cr-Commit-Position: refs/heads/master@{#439251}

[Peter Kasting, 2016-12-17 03:16:01]

I'm a little frustrated that you didn't reply to any of my comments, didn't do
anything with most of them (and, with the remaining one, did what I suggested
would be the worse solution), and then landed.

It's good form to reply either individually (if needed) or as a group (if you
implemented everything) to give your reactions/followup to various comments. 
For example, if you decide not to implement a review comment, say why.

Generally an "LGTM" plus comments is intended to mean "LGTM if you do these",
but if you elect not to, or to do something else that disagrees with the spirit
of the comment, you should probably not land without reviewer OK.

This is not worth reverting the CL over, but please reply to the comments if you
think the best response is something other than "done", and open a new CL for
any followups that are necessary as a result.

[ltian, 2016-12-21 23:26:53]

On 2016/12/17 03:16:01, Peter Kasting wrote:
> I'm a little frustrated that you didn't reply to any of my comments, didn't do
> anything with most of them (and, with the remaining one, did what I suggested
> would be the worse solution), and then landed.
> 
> It's good form to reply either individually (if needed) or as a group (if you
> implemented everything) to give your reactions/followup to various comments. 
> For example, if you decide not to implement a review comment, say why.
> 
> Generally an "LGTM" plus comments is intended to mean "LGTM if you do these",
> but if you elect not to, or to do something else that disagrees with the
spirit
> of the comment, you should probably not land without reviewer OK.
> 
> This is not worth reverting the CL over, but please reply to the comments if
you
> think the best response is something other than "done", and open a new CL for
> any followups that are necessary as a result.

Sorry I didn't reply to your comments and didn't update for all your comments
before
landing it. There are mainly two issues I didn't update. 

The first one is adding comments to the functions added in
template_url_service_android.h.
Because I find that all the public functions in template_url_service_android.h
doesn't
have any comments and from their function names, it is straightforward to learn
what those
functions do, so I thought it was better to keep consistent with other functions
and
skipped adding comments on them. But if you think it is better for have comments
on them,
I could add some to them.

The second issue is leaving UpdateTemplateURLVisitTime() in
template_url_serivce.h private
and expose it via a friend declaration. I tried adding FRIEND_TEST_ALL_PREFIXES(
TemplateUrlServiceAndroid, UpdateLastVisitedForTesting) to
template_url_serivce.h but then
there is an error when compiling the code reporting UpdateTemplateURLVisitTime
is a private
member. Because the unnitest is in Java code, I don't know any way to expose a
friend
declaration for it so I keep it as before.

Again, sorry I didn't reply all your comments and get your further approve
before landing it.

[Peter Kasting, 2016-12-22 00:04:12]

On 2016/12/21 23:26:53, ltian wrote:
> On 2016/12/17 03:16:01, Peter Kasting wrote:
> > I'm a little frustrated that you didn't reply to any of my comments, didn't
do
> > anything with most of them (and, with the remaining one, did what I
suggested
> > would be the worse solution), and then landed.
> > 
> > It's good form to reply either individually (if needed) or as a group (if
you
> > implemented everything) to give your reactions/followup to various comments.

> > For example, if you decide not to implement a review comment, say why.
> > 
> > Generally an "LGTM" plus comments is intended to mean "LGTM if you do
these",
> > but if you elect not to, or to do something else that disagrees with the
> spirit
> > of the comment, you should probably not land without reviewer OK.
> > 
> > This is not worth reverting the CL over, but please reply to the comments if
> you
> > think the best response is something other than "done", and open a new CL
for
> > any followups that are necessary as a result.
> 
> Sorry I didn't reply to your comments and didn't update for all your comments
> before
> landing it. There are mainly two issues I didn't update. 
> 
> The first one is adding comments to the functions added in
> template_url_service_android.h.
> Because I find that all the public functions in template_url_service_android.h
> doesn't
> have any comments and from their function names, it is straightforward to
learn
> what those
> functions do, so I thought it was better to keep consistent with other
functions
> and
> skipped adding comments on them. But if you think it is better for have
comments
> on them,
> I could add some to them.

You don't have to add comments on functions whose behavior is completely
obvious, but you should have comments about anything that's not, or in any case
where functions have side effects, need parameter explanations, etc.

For more detail, read the "declarations" section of
http://google.github.io/styleguide/cppguide.html#Function_Comments .

> The second issue is leaving UpdateTemplateURLVisitTime() in
> template_url_serivce.h private
> and expose it via a friend declaration. I tried adding
FRIEND_TEST_ALL_PREFIXES(
> TemplateUrlServiceAndroid, UpdateLastVisitedForTesting) to
> template_url_serivce.h but then
> there is an error when compiling the code reporting UpdateTemplateURLVisitTime
> is a private
> member. Because the unnitest is in Java code, I don't know any way to expose a
> friend
> declaration for it so I keep it as before.

I would deal with this by marking TemplateUrlServiceAndroid as a friend and
exposing this publicly there.  Then at least we limit who can call this to
Android-specific code.  It's somewhat reasonable for TemplateUrlServiceAndroid
to be a friend, so this makes sense.