[Android] Allow setting recently visited search engines as default search engine

chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java

Index: chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
diff --git a/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java b/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
index 855a003fcd29d839e79f3df8b66b50c6f071660d..8bc307e198ed386ae2a8c10c058d31de5402f52a 100644
--- a/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
+++ b/chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java
@@ -25,7 +25,9 @@ import android.widget.TextView;
 
 import org.chromium.base.ApiCompatibilityUtils;
 import org.chromium.base.ContextUtils;
+import org.chromium.base.metrics.RecordUserAction;
 import org.chromium.chrome.R;
+import org.chromium.chrome.browser.locale.LocaleManager;
 import org.chromium.chrome.browser.omnibox.geo.GeolocationHeader;
 import org.chromium.chrome.browser.preferences.website.ContentSetting;
 import org.chromium.chrome.browser.preferences.website.GeolocationInfo;
@@ -38,23 +40,17 @@ import org.chromium.components.location.LocationUtils;
 import org.chromium.ui.text.SpanApplier;
 import org.chromium.ui.text.SpanApplier.SpanInfo;
 
+import java.util.ArrayList;
 import java.util.List;
 
 /**
 * A custom adapter for listing search engines.
 */
 public class SearchEngineAdapter extends BaseAdapter implements LoadListener, OnClickListener {
-    /**
-     * A callback for reporting progress to the owner.
-     */
-    public interface SelectSearchEngineCallback {
-        /**
-         * Called when the search engine data has loaded and we've determined the currently active
-         * one.
-         * @param name Provides the name of it (with a simplified URL in parenthesis).
-         */
-        void currentSearchEngineDetermined(int selectedIndex);
-    }
+    private static final int VIEW_TYPE_ITEM = 0;
+    private static final int VIEW_TYPE_DIVIDER = 1;
+

[gone, 2016/11/28 21:37:02]

Remove newline above

[ltian, 2016/11/29 02:44:02]

Done.

+    private static final int VIEW_TYPE_COUNT = 2;
 
     // The current context.

[gone, 2016/11/28 21:37:02]

Switch these to /** javadoc style */ comments so they pop up in IDEs when you
hover over them.

[ltian, 2016/11/29 02:44:02]

Done.

     private Context mContext;
@@ -62,13 +58,15 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
     // The layout inflater to use for the custom views.
     private LayoutInflater mLayoutInflater;
 
-    // The callback to use for notifying caller of progress.
-    private SelectSearchEngineCallback mCallback;
+    // The list of prepopluated and default search engines.
+    private List<TemplateUrl> mPrepopulatedSearchEngines = new ArrayList<TemplateUrl>();

[gone, 2016/11/28 21:37:02]

= new ArrayList<>();

[ltian, 2016/11/29 02:44:02]

Done.

+
+    // The list of recently visited search engines.
+    private List<TemplateUrl> mRecentSearchEngines = new ArrayList<TemplateUrl>();
 
-    // The list of available search engines.
-    private List<TemplateUrl> mSearchEngines;
-    // The position (index into mSearchEngines) of the currently selected search engine. Can be -1
-    // if current search engine is managed and set to something other than the pre-populated values.
+    // The position (index into mPrepopulatedSearchEngines) of the currently selected search engine.
+    // Can be -1 if current search engine is managed and set to something other than the
+    // pre-populated values.
     private int mSelectedSearchEnginePosition = -1;
 
     // The position of the default search engine before user's action.
@@ -77,32 +75,27 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
     /**
      * Construct a SearchEngineAdapter.
      * @param context The current context.
-     * @param callback The callback to use to communicate back.
      */
-    public SearchEngineAdapter(Context context, SelectSearchEngineCallback callback) {
+    public SearchEngineAdapter(Context context) {
         mContext = context;
         mLayoutInflater = (LayoutInflater) mContext.getSystemService(
                 Context.LAYOUT_INFLATER_SERVICE);
-        mCallback = callback;
 
         initEntries();
     }
 
-    /**
-     * @return The index of the selected engine before user's action.
-     */
-    public int getInitialSearchEnginePosition() {
-        return mInitialEnginePosition;
-    }
-
     // Used for testing.
 
     String getValueForTesting() {

[gone, 2016/11/28 21:37:02]

Adding @VisibleForTesting or naming the function SomethingForTesting() usually
implies that without the comment.

[ltian, 2016/11/29 02:44:02]

Done.

         return Integer.toString(mSelectedSearchEnginePosition);
     }
 
-    void setValueForTesting(String value) {
-        searchEngineSelected(Integer.parseInt(value));
+    String setValueForTesting(String value) {
+        return searchEngineSelected(Integer.parseInt(value));
+    }
+
+    String getKeywordForTesting(int index) {
+        return toKeyword(index);
     }
 
     /**
@@ -116,37 +109,68 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
             return;  // Flow continues in onTemplateUrlServiceLoaded below.
         }
 
-        // Fetch all the search engine info and the currently active one.
-        mSearchEngines = templateUrlService.getLocalizedSearchEngines();
-        int searchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
-        // Convert the TemplateUrl index into an index into mSearchEngines.
+        int defaultSearchEngineIndex = templateUrlService.getDefaultSearchEngineIndex();
+        for (TemplateUrl templateUrl : templateUrlService.getSearchEngines()) {
+            if (templateUrl.getType() == TemplateUrlService.TYPE_PREPOPULATED
+                    || templateUrl.getType() == TemplateUrlService.TYPE_DEFAULT) {
+                mPrepopulatedSearchEngines.add(templateUrl);
+            } else {
+                mRecentSearchEngines.add(templateUrl);
+            }
+        }
+
+        // Convert the TemplateUrl index into an index of mSearchEngines.
         mSelectedSearchEnginePosition = -1;
-        for (int i = 0; i < mSearchEngines.size(); ++i) {
-            if (mSearchEngines.get(i).getIndex() == searchEngineIndex) {
+        for (int i = 0; i < mPrepopulatedSearchEngines.size(); ++i) {
+            if (mPrepopulatedSearchEngines.get(i).getIndex() == defaultSearchEngineIndex) {
                 mSelectedSearchEnginePosition = i;
             }
         }
+
+        for (int i = 0; i < mRecentSearchEngines.size(); ++i) {
+            if (mRecentSearchEngines.get(i).getIndex() == defaultSearchEngineIndex) {
+                // Add one to offset the title for recent search engine list

[gone, 2016/11/28 21:37:02]

1) Add period to end of the comment
2) the title for the recent search engine list

[ltian, 2016/11/29 02:44:02]

Done.

+                mSelectedSearchEnginePosition = i + mPrepopulatedSearchEngines.size() + 1;
+            }
+        }
+
         mInitialEnginePosition = mSelectedSearchEnginePosition;
 
-        // Report back what is selected.
-        mCallback.currentSearchEngineDetermined(toIndex(mSelectedSearchEnginePosition));
+        TemplateUrlService.getInstance().setSearchEngine(toKeyword(mSelectedSearchEnginePosition));
     }
 
-    private int toIndex(int position) {
-        return mSearchEngines.get(position).getIndex();
+    private String toKeyword(int position) {
+        if (position < mPrepopulatedSearchEngines.size()) {
+            return mPrepopulatedSearchEngines.get(position).getKeyword();
+        } else {
+            position -= mPrepopulatedSearchEngines.size() + 1;

[gone, 2016/11/28 21:37:02]

You use "mPrepopulatedSearchEngines.size() + 1" in a lot of places; you should
pull it out so that it's easy to fix if the list changes again, and so you can
explain why you need to +1 it in exactly one place.  I can't think of anything
better than "computeStartIndexForRecentSearchEngines" unfortunately :/

[ltian, 2016/11/29 02:44:02]

Done.

+            return mRecentSearchEngines.get(position).getKeyword();
+        }
     }
 
     // BaseAdapter:
 
     @Override
     public int getCount() {
-        return mSearchEngines == null ? 0 : mSearchEngines.size();
+        return mPrepopulatedSearchEngines == null
+                ? 0
+                : mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size() + 1;
+    }
+
+    @Override
+    public int getViewTypeCount() {
+        return VIEW_TYPE_COUNT;
     }
 
     @Override
     public Object getItem(int pos) {
-        TemplateUrl templateUrl = mSearchEngines.get(pos);
-        return templateUrl.getShortName();
+        if (pos < mPrepopulatedSearchEngines.size()) {
+            return mPrepopulatedSearchEngines.get(pos);
+        } else if (pos > mPrepopulatedSearchEngines.size()) {
+            pos -= mPrepopulatedSearchEngines.size() + 1;
+            return mRecentSearchEngines.get(pos);
+        }
+        return null;
     }
 
     @Override
@@ -155,10 +179,28 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
     }
 
     @Override
+    public int getItemViewType(int position) {
+        if (position == mPrepopulatedSearchEngines.size()) {
+            return VIEW_TYPE_DIVIDER;
+        } else {
+            return VIEW_TYPE_ITEM;
+        }
+    }
+
+    @Override
     public View getView(int position, View convertView, ViewGroup parent) {
         View view = convertView;
+        TemplateUrl templateUrl = (TemplateUrl) getItem(position);
+        int itemViewType = getItemViewType(position);
         if (convertView == null) {
-            view = mLayoutInflater.inflate(R.layout.search_engine, null);
+            if (itemViewType == VIEW_TYPE_DIVIDER) {
+                view = mLayoutInflater.inflate(R.layout.search_engine_recent_title, null);
+            } else {
+                view = mLayoutInflater.inflate(R.layout.search_engine, null);
+            }

[gone, 2016/11/28 21:37:02]

view = mLayoutInflater.inflate(itemViewType == VIEW_TYPE_DIVIDER ?
R.layout.search_engine_recent_title : R.layout.search_engine, null);

[ltian, 2016/11/29 02:44:02]

Done.

+        }
+        if (itemViewType == VIEW_TYPE_DIVIDER) {
+            return view;
         }
 
         view.setOnClickListener(this);
@@ -178,11 +220,18 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
         }
         radioButton.setChecked(selected);
 
-        TextView description = (TextView) view.findViewById(R.id.description);
-        TemplateUrl templateUrl = mSearchEngines.get(position);
+        TextView description = (TextView) view.findViewById(R.id.name);
         Resources resources = mContext.getResources();
         description.setText(templateUrl.getShortName());
 
+        TextView url = (TextView) view.findViewById(R.id.url);
+        url.setText(templateUrl.getUrl());
+        if (templateUrl.getType() == TemplateUrlService.TYPE_PREPOPULATED
+                || templateUrl.getType() == TemplateUrlService.TYPE_DEFAULT
+                || templateUrl.getUrl().length() == 0) {
+            url.setVisibility(View.GONE);
+        }
+
         // To improve the explore-by-touch experience, the radio button is hidden from accessibility
         // and instead, "checked" or "not checked" is read along with the search engine's name, e.g.
         // "google.com checked" or "google.com not checked".
@@ -202,7 +251,7 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
             }
         });
 
-        TextView link = (TextView) view.findViewById(R.id.link);
+        TextView link = (TextView) view.findViewById(R.id.location_permission);
         link.setVisibility(selected ? View.VISIBLE : View.GONE);
         if (selected) {
             if (getLocationPermissionType(position, true) == ContentSetting.ASK) {
@@ -251,7 +300,7 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
         }
     }
 
-    private void searchEngineSelected(int position) {
+    private String searchEngineSelected(int position) {
         // First clean up any automatically added permissions (if any) for the previously selected
         // search engine.
         SharedPreferences sharedPreferences =
@@ -259,7 +308,7 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
         if (sharedPreferences.getBoolean(PrefServiceBridge.LOCATION_AUTO_ALLOWED, false)) {
             if (locationEnabled(mSelectedSearchEnginePosition, false)) {
                 String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
-                        toIndex(mSelectedSearchEnginePosition));
+                        toKeyword(mSelectedSearchEnginePosition));
                 WebsitePreferenceBridge.nativeSetGeolocationSettingForOrigin(
                         url, url, ContentSetting.DEFAULT.toInt(), false);
             }
@@ -269,10 +318,17 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
         // Record the change in search engine.
         mSelectedSearchEnginePosition = position;
 
-        // Report the change back.
-        mCallback.currentSearchEngineDetermined(toIndex(mSelectedSearchEnginePosition));
+        String keyword = toKeyword(mSelectedSearchEnginePosition);
+        TemplateUrlService.getInstance().setSearchEngine(keyword);
 
+        // If the user has manually set the default search engine, disable auto switching.
+        boolean manualSwitch = mSelectedSearchEnginePosition != mInitialEnginePosition;
+        if (manualSwitch) {
+            RecordUserAction.record("SearchEngine_ManualChange");
+            LocaleManager.getInstance().setSearchEngineAutoSwitch(false);
+        }
         notifyDataSetChanged();
+        return keyword;
     }
 
     private void onLocationLinkClicked() {
@@ -282,7 +338,7 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
             Intent settingsIntent = PreferencesLauncher.createIntentForSettingsPage(
                     mContext, SingleWebsitePreferences.class.getName());
             String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
-                    toIndex(mSelectedSearchEnginePosition));
+                    toKeyword(mSelectedSearchEnginePosition));
             Bundle fragmentArgs = SingleWebsitePreferences.createFragmentArgsForSite(url);
             fragmentArgs.putBoolean(SingleWebsitePreferences.EXTRA_LOCATION,
                     locationEnabled(mSelectedSearchEnginePosition, true));
@@ -297,7 +353,7 @@ public class SearchEngineAdapter extends BaseAdapter implements LoadListener, On
         }
 
         String url = TemplateUrlService.getInstance().getSearchEngineUrlFromTemplateUrl(
-                toIndex(position));
+                toKeyword(position));
         GeolocationInfo locationSettings = new GeolocationInfo(url, null, false);
         ContentSetting locationPermission = locationSettings.getContentSetting();
         if (locationPermission == ContentSetting.ASK) {