[Android] Fix a custom search engine bounces when it selected and unselected as default engine

[Android] Fix a custom search engine bounces when it selected and unselected as default engine

When there are mutliple custom search engines displayed on setting page,
selecting and unselecting all except the top one will make them bounce
between default list and custom engine list. The right behaivor should
be the selected custom engine should remain in the same place and be
updated next time loading the setting page.

It is caused using their indexes to decide whether two engines are the
same. However, selecting/unselecting a custom engine will make Chrome
reorder the engine list which changes the selected/unselected one's
index and forces Chrome to refresh the list on Android.

To fix it, index of a search engine should not be used to decide whether
two engines are the same.

BUG=692343
Review-Url: https://codereview.chromium.org/2691033007
Cr-Commit-Position: refs/heads/master@{#451890}
Committed: https://chromium.googlesource.com/chromium/src/+/06aa457920d30616cc4c47c23057714694e3b3a7

[ltian, 2017-02-15 03:00:56]

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

[commit-bot: I haz the power, 2017-02-15 03:03:13]

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, 2017-02-15 04:27:35]

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

[commit-bot: I haz the power, 2017-02-15 04:27:35]

Dry run: This issue passed the CQ dry run.

[ltian, 2017-02-15 05:39:30]

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

[ltian, 2017-02-15 05:39:30]

tedchoc@chromium.org: can you take a look of my change in this CL?

Thanks!

[Ted C, 2017-02-15 17:08:37]

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:170:
int defaultSearchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
why do you need to make this move?  It seems like if anything it is doing
unnecessary work, but "shouldn't" be wrong.

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:124:
return mIndex == otherTemplateUrl.mIndex
as we make this less and less .equals and more specific to the one UI, I wonder
if we should move write our own comparison function in the other file to make
this more of a general .equals.

I'm worried that someone might come in and change this behavior and break search
engines.

[ltian, 2017-02-15 22:22:27]

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:170:
int defaultSearchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> why do you need to make this move?  It seems like if anything it is doing
> unnecessary work, but "shouldn't" be wrong.

The problem is if we leave these code here, when users select another custom
search engine, the sequence of engine list and the index of default search
engine will be changed, so the |defaultSearchEngineIndex| is changed. However,
if we don't check the index in compare function, |searchEnginesChanged| is false
and we don't update the engine list, then it will be inconsistent with
|defaultSearchEngineIndex| and cause UI to display wrong default search engine.

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:124:
return mIndex == otherTemplateUrl.mIndex
On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> as we make this less and less .equals and more specific to the one UI, I
wonder
> if we should move write our own comparison function in the other file to make
> this more of a general .equals.
> 
> I'm worried that someone might come in and change this behavior and break
search
> engines.

So is it a good idea to create a class named CompareUtil and have a public
static function like isEqualTemplateUrl(TemplateUrl lurl, TemplateUrl rurl)?

[Ted C, 2017-02-17 05:16:23]

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:170:
int defaultSearchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
On 2017/02/15 22:22:27, ltian wrote:
> On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> > why do you need to make this move?  It seems like if anything it is doing
> > unnecessary work, but "shouldn't" be wrong.
> 
> The problem is if we leave these code here, when users select another custom
> search engine, the sequence of engine list and the index of default search
> engine will be changed, so the |defaultSearchEngineIndex| is changed. However,
> if we don't check the index in compare function, |searchEnginesChanged| is
false
> and we don't update the engine list, then it will be inconsistent with
> |defaultSearchEngineIndex| and cause UI to display wrong default search
engine.

Ahh, because defaultSearchEngineIndex wouldn't match .getIndex() since the list
isn't up to date.  Got it.

Then I suggest we restructure this flow a bit.  I think we should pull out a
separate function that determines if the search engines changed (the code above
here), then I would move the contents of initializeSearchEngineGroups here. 
Since that does more than initialize groups at this point and really is the one
refreshing the data, I think it makes more since for that all to live in this
function now.

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:124:
return mIndex == otherTemplateUrl.mIndex
On 2017/02/15 22:22:27, ltian wrote:
> On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> > as we make this less and less .equals and more specific to the one UI, I
> wonder
> > if we should move write our own comparison function in the other file to
make
> > this more of a general .equals.
> > 
> > I'm worried that someone might come in and change this behavior and break
> search
> > engines.
> 
> So is it a good idea to create a class named CompareUtil and have a public
> static function like isEqualTemplateUrl(TemplateUrl lurl, TemplateUrl rurl)?

I don't think I'd create a new class.  I would probably just add a static method
in the search engine pref class like containsTemplateUrl(List<TemplateUrl>
templateUrlList, TemplateUrl templateUrl).

I broke the concept of the .equals with my last change here, which I think we
should undo, then move the pref specific logic to that file.

[ltian, 2017-02-17 19:34:59]

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:124:
return mIndex == otherTemplateUrl.mIndex
On 2017/02/17 05:16:22, Ted C wrote:
> On 2017/02/15 22:22:27, ltian wrote:
> > On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> > > as we make this less and less .equals and more specific to the one UI, I
> > wonder
> > > if we should move write our own comparison function in the other file to
> make
> > > this more of a general .equals.
> > > 
> > > I'm worried that someone might come in and change this behavior and break
> > search
> > > engines.
> > 
> > So is it a good idea to create a class named CompareUtil and have a public
> > static function like isEqualTemplateUrl(TemplateUrl lurl, TemplateUrl rurl)?
> 
> I don't think I'd create a new class.  I would probably just add a static
method
> in the search engine pref class like containsTemplateUrl(List<TemplateUrl>
> templateUrlList, TemplateUrl templateUrl).
> 
> I broke the concept of the .equals with my last change here, which I think we
> should undo, then move the pref specific logic to that file.

How about have a non-static containsTemplateUrl(List<TemplateUrl>
templateUrlList, TemplateUrl templateUrl) in SearchEngineAdapter class because
SearchEnginePreference extends PreferenceFragment and only deals with loading
adapter.

And how about we just remove this overrided equals fucntion. Because leaving it
as before that only check index and shortname is still confusing. Also I am not
sure why my original code checks the index for equal, but considering we don't
want UI changing when the default engine is changed, index should not be check
for equal.

[Ted C, 2017-02-17 21:34:39]

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:124:
return mIndex == otherTemplateUrl.mIndex
On 2017/02/17 19:34:59, ltian wrote:
> On 2017/02/17 05:16:22, Ted C wrote:
> > On 2017/02/15 22:22:27, ltian wrote:
> > > On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> > > > as we make this less and less .equals and more specific to the one UI, I
> > > wonder
> > > > if we should move write our own comparison function in the other file to
> > make
> > > > this more of a general .equals.
> > > > 
> > > > I'm worried that someone might come in and change this behavior and
break
> > > search
> > > > engines.
> > > 
> > > So is it a good idea to create a class named CompareUtil and have a public
> > > static function like isEqualTemplateUrl(TemplateUrl lurl, TemplateUrl
rurl)?
> > 
> > I don't think I'd create a new class.  I would probably just add a static
> method
> > in the search engine pref class like containsTemplateUrl(List<TemplateUrl>
> > templateUrlList, TemplateUrl templateUrl).
> > 
> > I broke the concept of the .equals with my last change here, which I think
we
> > should undo, then move the pref specific logic to that file.
> 
> How about have a non-static containsTemplateUrl(List<TemplateUrl>
> templateUrlList, TemplateUrl templateUrl) in SearchEngineAdapter class because
> SearchEnginePreference extends PreferenceFragment and only deals with loading
> adapter.

Ah, I meant a private static function in SearchEngineAdapter (I don't think you
need it to be non-static).  I forgot that the pref wasn't the same thing.

> 
> And how about we just remove this overrided equals fucntion. Because leaving
it
> as before that only check index and shortname is still confusing. Also I am
not
> sure why my original code checks the index for equal, but considering we don't
> want UI changing when the default engine is changed, index should not be check
> for equal.

ToolbarManager uses the .equals for TemplateUrl, so  I think we should leave it.
 But we should add mIndex and mTemplateUrlType to the check here.  We should
just make this a proper java .equals and check all fields.
I would add back

[ltian, 2017-02-17 23:47:40]

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

[commit-bot: I haz the power, 2017-02-17 23:49:18]

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, 2017-02-18 01:54:01]

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

[commit-bot: I haz the power, 2017-02-18 01:54:03]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[ltian, 2017-02-21 07:53:52]

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:170:
int defaultSearchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
On 2017/02/17 05:16:22, Ted C wrote:
> On 2017/02/15 22:22:27, ltian wrote:
> > On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> > > why do you need to make this move?  It seems like if anything it is doing
> > > unnecessary work, but "shouldn't" be wrong.
> > 
> > The problem is if we leave these code here, when users select another custom
> > search engine, the sequence of engine list and the index of default search
> > engine will be changed, so the |defaultSearchEngineIndex| is changed.
However,
> > if we don't check the index in compare function, |searchEnginesChanged| is
> false
> > and we don't update the engine list, then it will be inconsistent with
> > |defaultSearchEngineIndex| and cause UI to display wrong default search
> engine.
> 
> Ahh, because defaultSearchEngineIndex wouldn't match .getIndex() since the
list
> isn't up to date.  Got it.
> 
> Then I suggest we restructure this flow a bit.  I think we should pull out a
> separate function that determines if the search engines changed (the code
above
> here), then I would move the contents of initializeSearchEngineGroups here. 
> Since that does more than initialize groups at this point and really is the
one
> refreshing the data, I think it makes more since for that all to live in this
> function now.

Done.

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java
(left):

https://codereview.chromium.org/2691033007/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:124:
return mIndex == otherTemplateUrl.mIndex
On 2017/02/17 21:34:39, Ted C wrote:
> On 2017/02/17 19:34:59, ltian wrote:
> > On 2017/02/17 05:16:22, Ted C wrote:
> > > On 2017/02/15 22:22:27, ltian wrote:
> > > > On 2017/02/15 17:08:37, Ted C (OOO till 2.17) wrote:
> > > > > as we make this less and less .equals and more specific to the one UI,
I
> > > > wonder
> > > > > if we should move write our own comparison function in the other file
to
> > > make
> > > > > this more of a general .equals.
> > > > > 
> > > > > I'm worried that someone might come in and change this behavior and
> break
> > > > search
> > > > > engines.
> > > > 
> > > > So is it a good idea to create a class named CompareUtil and have a
public
> > > > static function like isEqualTemplateUrl(TemplateUrl lurl, TemplateUrl
> rurl)?
> > > 
> > > I don't think I'd create a new class.  I would probably just add a static
> > method
> > > in the search engine pref class like containsTemplateUrl(List<TemplateUrl>
> > > templateUrlList, TemplateUrl templateUrl).
> > > 
> > > I broke the concept of the .equals with my last change here, which I think
> we
> > > should undo, then move the pref specific logic to that file.
> > 
> > How about have a non-static containsTemplateUrl(List<TemplateUrl>
> > templateUrlList, TemplateUrl templateUrl) in SearchEngineAdapter class
because
> > SearchEnginePreference extends PreferenceFragment and only deals with
loading
> > adapter.
> 
> Ah, I meant a private static function in SearchEngineAdapter (I don't think
you
> need it to be non-static).  I forgot that the pref wasn't the same thing.
> 
> > 
> > And how about we just remove this overrided equals fucntion. Because leaving
> it
> > as before that only check index and shortname is still confusing. Also I am
> not
> > sure why my original code checks the index for equal, but considering we
don't
> > want UI changing when the default engine is changed, index should not be
check
> > for equal.
> 
> ToolbarManager uses the .equals for TemplateUrl, so  I think we should leave
it.
>  But we should add mIndex and mTemplateUrlType to the check here.  We should
> just make this a proper java .equals and check all fields.
> I would add back 

Done.

[Ted C, 2017-02-21 16:57:37]

some style nits, but overall this looks right to me

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:142:
if (isSearchEngineChaned(templateUrls)) {
I would early return here so you can reduce the indent

if (!isSearchEnginChanged(templateUrls)) return;

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:196:
isContain = true;
you can just return true here.

Then you don't need isContain at all and you return false at the bottom

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:203:
private boolean isSearchEngineChaned(List<TemplateUrl> templateUrls) {
I would call this:
didSearchEnginesChange

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:209:
if (!SearchEngineAdapter.containsTemplateUrl(
since you're in the same file, you don't need to fully qualify the static (i.e.
you can drop SearchEngineAdapter.)

[ltian, 2017-02-21 19:10:04]

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:142:
if (isSearchEngineChaned(templateUrls)) {
On 2017/02/21 16:57:37, Ted C wrote:
> I would early return here so you can reduce the indent
> 
> if (!isSearchEnginChanged(templateUrls)) return;

Done.

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:196:
isContain = true;
On 2017/02/21 16:57:37, Ted C wrote:
> you can just return true here.
> 
> Then you don't need isContain at all and you return false at the bottom

Done.

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:203:
private boolean isSearchEngineChaned(List<TemplateUrl> templateUrls) {
On 2017/02/21 16:57:37, Ted C wrote:
> I would call this:
> didSearchEnginesChange

Done.

https://codereview.chromium.org/2691033007/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:209:
if (!SearchEngineAdapter.containsTemplateUrl(
On 2017/02/21 16:57:37, Ted C wrote:
> since you're in the same file, you don't need to fully qualify the static
(i.e.
> you can drop SearchEngineAdapter.)

Done.

[Ted C, 2017-02-21 19:48:33]

lgtm w/ last nits

https://codereview.chromium.org/2691033007/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2691033007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:142:
if (!didSearchEnginesChange(templateUrls)) {
in java, we allow the conditional and statement to go on a single line w/o
braces if they all fit (and is readable).

In this case, I think it would be.

https://codereview.chromium.org/2691033007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:205:
if (!searchEnginesChanged) {
I think we can simplify this even more.

if (templateUrls.size() !=
        mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size()) {
    return true;
}

Then we can do the same thing above.  Just return true from within the loop and
return false at the end.  No need for a variable at all.

[ltian, 2017-02-21 20:00:56]

https://codereview.chromium.org/2691033007/diff/40001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
(right):

https://codereview.chromium.org/2691033007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:142:
if (!didSearchEnginesChange(templateUrls)) {
On 2017/02/21 19:48:33, Ted C wrote:
> in java, we allow the conditional and statement to go on a single line w/o
> braces if they all fit (and is readable).
> 
> In this case, I think it would be.

Done.

https://codereview.chromium.org/2691033007/diff/40001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:205:
if (!searchEnginesChanged) {
On 2017/02/21 19:48:33, Ted C wrote:
> I think we can simplify this even more.
> 
> if (templateUrls.size() !=
>         mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size()) {
>     return true;
> }
> 
> Then we can do the same thing above.  Just return true from within the loop
and
> return false at the end.  No need for a variable at all.

Done.

[ltian, 2017-02-21 20:12:07]

The CQ bit was checked by ltian@chromium.org

[ltian, 2017-02-21 20:12:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tedchoc@chromium.org
Link to the patchset: https://codereview.chromium.org/2691033007/#ps60001
(title: "Update based on Ted's nits.")

[commit-bot: I haz the power, 2017-02-21 20:13:15]

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, 2017-02-21 22:16:11]

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

[commit-bot: I haz the power, 2017-02-21 22:16:12]

Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)

[ltian, 2017-02-22 05:08:25]

The CQ bit was checked by ltian@chromium.org

[commit-bot: I haz the power, 2017-02-22 05:09:40]

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, 2017-02-22 05:46:43]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487740105218420,
"parent_rev": "fc37a436628053178de0179e8d4566d3e1ea9800", "commit_rev":
"06aa457920d30616cc4c47c23057714694e3b3a7"}

[commit-bot: I haz the power, 2017-02-22 05:47:44]

Description was changed from

==========
[Android] Fix a custom search engine bounces when it selected and unselected as
default engine

When there are mutliple custom search engines displayed on setting page,
selecting and unselecting all except the top one will make them bounce
between default list and custom engine list. The right behaivor should
be the selected custom engine should remain in the same place and be
updated next time loading the setting page.

It is caused using their indexes to decide whether two engines are the
same. However, selecting/unselecting a custom engine will make Chrome
reorder the engine list which changes the selected/unselected one's
index and forces Chrome to refresh the list on Android.

To fix it, index of a search engine should not be used to decide whether
two engines are the same.

BUG=692343
==========

to

==========
[Android] Fix a custom search engine bounces when it selected and unselected as
default engine

When there are mutliple custom search engines displayed on setting page,
selecting and unselecting all except the top one will make them bounce
between default list and custom engine list. The right behaivor should
be the selected custom engine should remain in the same place and be
updated next time loading the setting page.

It is caused using their indexes to decide whether two engines are the
same. However, selecting/unselecting a custom engine will make Chrome
reorder the engine list which changes the selected/unselected one's
index and forces Chrome to refresh the list on Android.

To fix it, index of a search engine should not be used to decide whether
two engines are the same.

BUG=692343

Review-Url: https://codereview.chromium.org/2691033007
Cr-Commit-Position: refs/heads/master@{#451890}
Committed:
https://chromium.googlesource.com/chromium/src/+/06aa457920d30616cc4c47c23057...
==========

[commit-bot: I haz the power, 2017-02-22 05:47:55]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/06aa457920d30616cc4c47c23057...