Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2367373003:
[Android] Allow setting recently visited search engines as default search engine (Closed)
Description[Android] Allow setting recently visited search engines as default search engine
1. Change setting page layout to display recently visited search engines.
2. Change search engine layout to display url for recently visited
engines.
3. Implement the logic to change the location of custom search engine
based on whether it is selected as default or not.
BUG=348360
Committed: https://crrev.com/3b5e1a345a490034e4588ce4e512bfa10de413f3
Cr-Commit-Position: refs/heads/master@{#435383}
Patch Set 1 #
Total comments: 26
Patch Set 2 : Update based on Ian's comments. #Patch Set 3 : remove SAVE and CANCEL buttons, change set default engine by keyword instead of index, change will … #Patch Set 4 : remove unnecessary changes. #
Total comments: 20
Patch Set 5 : update based on ian's comments. #
Total comments: 24
Patch Set 6 : Update based on Ian's comments. #Patch Set 7 : Remove unnecessary comment. #Patch Set 8 : Update the test code to adapt new change. #Patch Set 9 : Adapt to the changes form gclient sync #
Total comments: 16
Patch Set 10 : Update based on Ian's comments and fix bugs caused by test cases. #
Total comments: 23
Patch Set 11 : Update based on Ian and Peter's new comments. #Patch Set 12 : Update based on Dan's comments. #
Total comments: 25
Patch Set 13 : Update based on Dan's new comments. #
Total comments: 23
Patch Set 14 : Update based on the CL dealing with deletion of show_in_default_list. #Patch Set 15 : Reorganize template_url_serivce_android code. #
Total comments: 44
Patch Set 16 : Update based on Dan and Peter's comments. #
Total comments: 31
Patch Set 17 : Update based on Peter and Dan's comments. #
Total comments: 1
Depends on Patchset: Messages
Total messages: 82 (44 generated)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
ltian@chromium.org changed reviewers: + ianwen@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
The CQ bit was unchecked by ltian@chromium.org
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
The CQ bit was unchecked by ltian@chromium.org
https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... File chrome/android/java/res/layout/search_engine.xml (right): https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/res/layout/search_engine.xml:31: android:textColor="@color/fre_title_color" /> If you want to reuse this color, you should change the name of it. An alternative is to simply put color constant here, such as #565656. https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/res/layout/search_engine.xml:36: android:textSize="15sp" /> Preset it to visibility gone? https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... File chrome/android/java/res/layout/search_engine_divider.xml (right): https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/res/layout/search_engine_divider.xml:6: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" I wouldn't call this file "divider". Instead, how about search_engine_recent_title https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/res/layout/search_engine_divider.xml:17: android:layout_width="match_parent" Indentation not correct. You should install android plugin for your eclipse to solve this issue. https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:113: int searchEngineIndex = templateUrlService.getDefaultSearchEngineIndex(); Rename this to be defaultEngineIndex. https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:118: TemplateUrl divider = new TemplateUrl(-1, "", "", false); Oops this is wrong. Don't do it. What will happen if the user clicks on the title? Let's not add fake template urls to the model. I believe what you should do is simply in getCount(), return mSearchEngines.size() + 1. If you are not sure, check out BookmarkDrawerListViewAdapter. https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:135: private void setSearchEngineType(TemplateUrl templateUrl, int searchEngineIndex) { The adapter shouldn't be responsible for setting the type of the template url. This value should already have been set up during construction. https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:49: public enum TemplateUrlType { Let's use ints instead of enums. https://developer.android.com/training/articles/memory.html#Overhead https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:59: DIVIDER, Remove DIVIDER https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:60: CUSTOM I wouldn't use the term "custom" here, as the list you are showing is not customizable. You might want to rephrase it to "recent" https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:104: public boolean getIsPrepopulated() { Instead of creating this method, you could create a method called getType, and return an integer. Later users of this class will know if it is a recent, or prepopulated, or default. https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:134: public int compareTo(TemplateUrl templateUrl) { Q: why adding this? https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... File chrome/android/java/strings/android_chrome_strings.grd (right): https://chromiumcodereview.appspot.com/2367373003/diff/1/chrome/android/java/... chrome/android/java/strings/android_chrome_strings.grd:260: <message name="IDS_RECENTLY_VISITED" desc="The title of recently visited search engine list view."> IDS_SEARCH_ENGINE_RECENTLY_VISITED
https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/search_engine.xml (right): https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/search_engine.xml:31: android:textColor="@color/fre_title_color" /> On 2016/09/27 04:42:00, Ian Wen wrote: > If you want to reuse this color, you should change the name of it. An > alternative is to simply put color constant here, such as #565656. Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/search_engine.xml:36: android:textSize="15sp" /> On 2016/09/27 04:42:00, Ian Wen wrote: > Preset it to visibility gone? I make it visible as the default setting. Then in SearchEngineAdapater, it will be set as gone if the search engine is prepopulated. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/res/lay... File chrome/android/java/res/layout/search_engine_divider.xml (right): https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/search_engine_divider.xml:6: <LinearLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2016/09/27 04:42:00, Ian Wen wrote: > I wouldn't call this file "divider". Instead, how about > search_engine_recent_title Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/res/lay... chrome/android/java/res/layout/search_engine_divider.xml:17: android:layout_width="match_parent" On 2016/09/27 04:42:00, Ian Wen wrote: > Indentation not correct. You should install android plugin for your eclipse to > solve this issue. Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:113: int searchEngineIndex = templateUrlService.getDefaultSearchEngineIndex(); On 2016/09/27 04:42:00, Ian Wen wrote: > Rename this to be defaultEngineIndex. Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:118: TemplateUrl divider = new TemplateUrl(-1, "", "", false); On 2016/09/27 04:42:00, Ian Wen wrote: > Oops this is wrong. Don't do it. What will happen if the user clicks on the > title? Let's not add fake template urls to the model. > > I believe what you should do is simply in getCount(), return > mSearchEngines.size() + 1. > > If you are not sure, check out BookmarkDrawerListViewAdapter. Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:135: private void setSearchEngineType(TemplateUrl templateUrl, int searchEngineIndex) { On 2016/09/27 04:42:00, Ian Wen wrote: > The adapter shouldn't be responsible for setting the type of the template url. > This value should already have been set up during construction. Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:49: public enum TemplateUrlType { On 2016/09/27 04:42:00, Ian Wen wrote: > Let's use ints instead of enums. > https://developer.android.com/training/articles/memory.html#Overhead Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:59: DIVIDER, On 2016/09/27 04:42:00, Ian Wen wrote: > Remove DIVIDER Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:60: CUSTOM On 2016/09/27 04:42:00, Ian Wen wrote: > I wouldn't use the term "custom" here, as the list you are showing is not > customizable. You might want to rephrase it to "recent" Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:104: public boolean getIsPrepopulated() { On 2016/09/27 04:42:00, Ian Wen wrote: > Instead of creating this method, you could create a method called getType, and > return an integer. Later users of this class will know if it is a recent, or > prepopulated, or default. Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/src/org... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:134: public int compareTo(TemplateUrl templateUrl) { On 2016/09/27 04:42:00, Ian Wen wrote: > Q: why adding this? Done. https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/strings... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2367373003/diff/1/chrome/android/java/strings... chrome/android/java/strings/android_chrome_strings.grd:260: <message name="IDS_RECENTLY_VISITED" desc="The title of recently visited search engine list view."> On 2016/09/27 04:42:00, Ian Wen wrote: > IDS_SEARCH_ENGINE_RECENTLY_VISITED Done.
I will create a CL to include urls in the view so that it can be cherry-picked. https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/search_engine_recent_title.xml:16: android:text="@string/search_engine_recently_visited" 1. You shouldn't use LinearLayout. Linearlayout should only be used when you need to layout children in linear fashion. 2. In this case you should not use a wrapper layout at all. Instead I would set paddings. https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:83: static final int VIEW_TYPE_ITEM = 0; 1. If this variable is not used by other classes in this package, you should mark it as private. 2. Static final < static < private final < private. Let's make the order correct. https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:110: String getKeywordFromIndex(int index) { Rename it to getKeywordForTesting() ? https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:128: * add into different lists. Inner comments should start with // https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:139: // Convert the TemplateUrl index into an index into mSearchEngines. an index *of* mSearchEngines. https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:83: index = template_url_list.size() - 1; Why is index always size() - 1? Shouldn't it be 1? Also you should create a helper static method that returns whether a template url is in the default list or not, so that you don't need to check isPrepopulated and created_by_policy all the time. https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:90: // If default search engine is recently visited one, force it Nit: force it *to* append https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:144: TemplateURL* template_url = template_url_list[index]; Since this class does not own the template_urls that it stores, you should add null check here and return null if a template url is somehow deleted in the service. https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.h:97: std::vector<TemplateURL*> template_url_list; In C++, class member variable should end with "_". https://codereview.chromium.org/2367373003/diff/60001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/60001/components/search_engin... components/search_engines/template_url_service.h:391: TemplateURL* FindNonExtensionTemplateURLForKeyword( It looks like GetTemplateURLForKeyword is already a public method in this class and I think we should directly use that instead. There is no extension on Android.
https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/res... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/res... chrome/android/java/res/layout/search_engine_recent_title.xml:16: android:text="@string/search_engine_recently_visited" On 2016/10/17 23:32:21, Ian Wen wrote: > 1. You shouldn't use LinearLayout. Linearlayout should only be used when you > need to layout children in linear fashion. > 2. In this case you should not use a wrapper layout at all. Instead I would set > paddings. Done. https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:83: static final int VIEW_TYPE_ITEM = 0; On 2016/10/17 23:32:21, Ian Wen wrote: > 1. If this variable is not used by other classes in this package, you should > mark it as private. > > 2. Static final < static < private final < private. Let's make the order > correct. Done. https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:110: String getKeywordFromIndex(int index) { On 2016/10/17 23:32:21, Ian Wen wrote: > Rename it to getKeywordForTesting() ? Done. https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:128: * add into different lists. On 2016/10/17 23:32:21, Ian Wen wrote: > Inner comments should start with // Done. https://codereview.chromium.org/2367373003/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:139: // Convert the TemplateUrl index into an index into mSearchEngines. On 2016/10/17 23:32:21, Ian Wen wrote: > an index *of* mSearchEngines. Done. https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:83: index = template_url_list.size() - 1; On 2016/10/17 23:32:21, Ian Wen wrote: > Why is index always size() - 1? Shouldn't it be 1? > > Also you should create a helper static method that returns whether a template > url is in the default list or not, so that you don't need to check > isPrepopulated and created_by_policy all the time. Our plan is if the default search engine is a recently visited engine, we manually append it to the bottom of top list, otherwise keep the order as it is provided. So it is not always 1. https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:90: // If default search engine is recently visited one, force it On 2016/10/17 23:32:21, Ian Wen wrote: > Nit: force it *to* append Done. https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:144: TemplateURL* template_url = template_url_list[index]; On 2016/10/17 23:32:21, Ian Wen wrote: > Since this class does not own the template_urls that it stores, you should add > null check here and return null if a template url is somehow deleted in the > service. Done. https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/60001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.h:97: std::vector<TemplateURL*> template_url_list; On 2016/10/17 23:32:21, Ian Wen wrote: > In C++, class member variable should end with "_". Done. https://codereview.chromium.org/2367373003/diff/60001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/60001/components/search_engin... components/search_engines/template_url_service.h:391: TemplateURL* FindNonExtensionTemplateURLForKeyword( On 2016/10/17 23:32:21, Ian Wen wrote: > It looks like GetTemplateURLForKeyword is already a public method in this class > and I think we should directly use that instead. > > There is no extension on Android. Done.
https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:57: * @param name Provides the name of it (with a simplified URL in parenthesis). You should update the javadoc for this function. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:74: private List<TemplateUrl> mRecentVisitedSearchEngines = new ArrayList<TemplateUrl>(); How about renaming it to mRecentSearchEngines? https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:77: // if current search engine is managed and set to something other than the pre-populated values. Please reformat the comments. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:84: private static final int VIEW_TYPE_DIVIDER = 1; Like what I said in last patchset, public members should be listed above protected and private members; static members should show above nonstatic members; final member should show above non final members. you should move #83-#84 to #61. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:127: // add into different lists. I would simply remove the comments between 126 and 127. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:128: for (TemplateUrl templateUrl : templateUrlService.getLocalizedSearchEngines()) { I would rename getLocalizedSearchEngines() to getSearchEngines(), as it is no longer just localized any more. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:147: mSelectedSearchEnginePosition = i + mPrepopulatedSearchEngines.size() + 1; Please explain why you need to add one here. "// Add one to offset the title for recent search engine list" https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:205: if (itemViewType == VIEW_TYPE_DIVIDER) { Oops this is a mistake. The reason why you need to have convertView is that you could recycle the item view that is scrolled off the screen. In your current approach you are creating new Views, even if convertView is already inflated. Check this out: http://lucasr.org/2012/04/05/performance-tips-for-androids-listview/. Also you should understand the code in bookmarks: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:61: public void currentSearchEngineDetermined(String selectedKeyword) { I think this method should be removed, and the interface should be removed as well. In the adapter we could call setSearchEngine() ourselves. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:49: public static final int TYPE_PREPOPULATED = 0; Move #49 down with #54. https://codereview.chromium.org/2367373003/diff/80001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:88: } else if (default_search_provider == template_urls[i]) { I don't understand why you need to continue if dsp is current template url. https://codereview.chromium.org/2367373003/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:94: // If default search engine is recently visited one, force it to Did you mean if index is not set, then dsp is in recent list? If so, you should guard the following following code by checking if index == -1.
https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:57: * @param name Provides the name of it (with a simplified URL in parenthesis). On 2016/10/19 00:03:31, Ian Wen wrote: > You should update the javadoc for this function. Interface will be removed and this class will directly update the default search provider. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:74: private List<TemplateUrl> mRecentVisitedSearchEngines = new ArrayList<TemplateUrl>(); On 2016/10/19 00:03:30, Ian Wen wrote: > How about renaming it to mRecentSearchEngines? Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:77: // if current search engine is managed and set to something other than the pre-populated values. On 2016/10/19 00:03:31, Ian Wen wrote: > Please reformat the comments. Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:84: private static final int VIEW_TYPE_DIVIDER = 1; On 2016/10/19 00:03:31, Ian Wen wrote: > Like what I said in last patchset, public members should be listed above > protected and private members; static members should show above nonstatic > members; final member should show above non final members. > > you should move #83-#84 to #61. Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:127: // add into different lists. On 2016/10/19 00:03:31, Ian Wen wrote: > I would simply remove the comments between 126 and 127. Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:128: for (TemplateUrl templateUrl : templateUrlService.getLocalizedSearchEngines()) { On 2016/10/19 00:03:31, Ian Wen wrote: > I would rename getLocalizedSearchEngines() to getSearchEngines(), as it is no > longer just localized any more. Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:147: mSelectedSearchEnginePosition = i + mPrepopulatedSearchEngines.size() + 1; On 2016/10/19 00:03:31, Ian Wen wrote: > Please explain why you need to add one here. > > "// Add one to offset the title for recent search engine list" Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:205: if (itemViewType == VIEW_TYPE_DIVIDER) { On 2016/10/19 00:03:31, Ian Wen wrote: > Oops this is a mistake. > > The reason why you need to have convertView is that you could recycle the item > view that is scrolled off the screen. In your current approach you are creating > new Views, even if convertView is already inflated. > > Check this out: > http://lucasr.org/2012/04/05/performance-tips-for-androids-listview/. > > Also you should understand the code in bookmarks: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:61: public void currentSearchEngineDetermined(String selectedKeyword) { On 2016/10/19 00:03:31, Ian Wen wrote: > I think this method should be removed, and the interface should be removed as > well. In the adapter we could call setSearchEngine() ourselves. Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:49: public static final int TYPE_PREPOPULATED = 0; On 2016/10/19 00:03:31, Ian Wen wrote: > Move #49 down with #54. Done. https://codereview.chromium.org/2367373003/diff/80001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:88: } else if (default_search_provider == template_urls[i]) { On 2016/10/19 00:03:31, Ian Wen wrote: > I don't understand why you need to continue if dsp is current template url. This part checks if the default search engine is a recently visited one. If so, we want put it after all the prepopulated ones. For example Airbnb could be put in front of all the prepopulated ones if set as default. So we manually change the order of the list. https://codereview.chromium.org/2367373003/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_android.cc:94: // If default search engine is recently visited one, force it to On 2016/10/19 00:03:31, Ian Wen wrote: > Did you mean if index is not set, then dsp is in recent list? > > If so, you should guard the following following code by checking if index == -1. No, the index is set but not guaranteed to be after all prepopulated ones. So we reorganize the order.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Almost there! :) https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_layout.xml (right): https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_layout.xml:17: android:layout_height="match_parent"/> Since this layout is so simple now, it makes me think if it is necessary. If you call super in SearchEnginePreference#onCreateView() to inflate the default layout, and then find the list using the same id, I think it should just work. Another example is that SiteSettingsPreferences is a list and it does not need to override onCreateView(). https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:135: // Report back what is selected. Nit: remove #135 as it is outdated https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:135: // Report back what is selected. I think this comment is not needed. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:195: view.setOnClickListener(null); I think you want to replace #195 and #196 to view.setClickable(false). https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:312: // Report the change back. Remove #312. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:51: * if a custom search engine is set as default, it will move to the default list. s/default list/prepopulated list https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:54: public static final int TYPE_PREPOPULATED = 0; Please order them in ascending order. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:70: @TemplateUrlType IIRC you should append @TemplateUrlType between private and int.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_layout.xml (right): https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_layout.xml:17: android:layout_height="match_parent"/> On 2016/10/26 18:56:14, Ian Wen wrote: > Since this layout is so simple now, it makes me think if it is necessary. > > If you call super in SearchEnginePreference#onCreateView() to inflate the > default layout, and then find the list using the same id, I think it should just > work. > > Another example is that SiteSettingsPreferences is a list and it does not need > to override onCreateView(). Done. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:135: // Report back what is selected. On 2016/10/26 18:56:14, Ian Wen wrote: > Nit: remove #135 as it is outdated Done. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:135: // Report back what is selected. On 2016/10/26 18:56:14, Ian Wen wrote: > I think this comment is not needed. Done. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:195: view.setOnClickListener(null); On 2016/10/26 18:56:14, Ian Wen wrote: > I think you want to replace #195 and #196 to view.setClickable(false). Done. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:312: // Report the change back. On 2016/10/26 18:56:14, Ian Wen wrote: > Remove #312. Done. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:51: * if a custom search engine is set as default, it will move to the default list. On 2016/10/26 18:56:14, Ian Wen wrote: > s/default list/prepopulated list Done. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:54: public static final int TYPE_PREPOPULATED = 0; On 2016/10/26 18:56:15, Ian Wen wrote: > Please order them in ascending order. Done. https://codereview.chromium.org/2367373003/diff/160001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:70: @TemplateUrlType On 2016/10/26 18:56:15, Ian Wen wrote: > IIRC you should append @TemplateUrlType between private and int. It fires an error "'@TemplateUrlType' annotation modifier does not precede non-annotation modifiers." if putting @TemplateUrlType between private and int.
Looks good in general. I think you should pass it to owners after addressing my comments. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:14: android:paddingBottom="16dp" Shouldn't you set clickable to be false here? https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:194: view.setClickable(false); BTW this can be done in xml, so you don't need to do it in java. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:51: int paddingTop = (int) (density * PADDING_TOP); Don't do this... Add the padding into dimens.xml and get that dimension from resource. Finding density and doing it your self is not android typical style. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:54: layoutParams.setMargins(0, paddingTop, 0, 0); Also if this is used as margin, the variable name should be marginTop, not paddingTop.
ltian@chromium.org changed reviewers: + dfalcantara@chromium.org, pkasting@chromium.org
dfalcantara@chromium.org: Please review changes in chrome/android/. pkasting@chromium.org: Please review changes in chrome/browser/ and components/. Thanks!
https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:69: return IsPrepopulatedTemplate(template_url) || Nit: Seems like the body of this method can now be inlined here. At that point this seems closely related to TemplateURLService::CanReplace(), except for the safe_for_autoreplace() check. Can/should we move this function to that class and build CanReplace() off it? https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:82: std::vector<TemplateURL*> recent_visit_list; It seems surprising that this method updates |template_url_list_|, even though from the name it sounds like a simple getter. I think it would be clearer if this became more of a simple getter, and one of the following things happened: * We auto-updated this list appropriately every time something changed in the underlying model that would affect it. * The methods that currently access |template_url_list_| (including this one) instead called some kind of getter that supplied them with the appropriate list, with whatever necessary smarts to construct/cache it as needed. https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:83: for (size_t i = 0; i < template_urls.size(); ++i) { I'm slightly confused by what this function is doing. I think I mostly follow it, but can you break into more blocks and give high-level comments on the blocks about what we're trying to do? It may also help to give more function-level comments in the header. I may have more comments on this after you do that. https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:149: return NULL; Nit: nullptr https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:99: std::vector<TemplateURL*> template_url_list_; Nit: Goes above DISALLOW_COPY_AND_ASSIGN. Avoid using "list" in the name of a variable that isn't a list. How about |template_urls_|?
https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:14: android:paddingBottom="16dp" On 2016/10/28 22:17:39, Ian Wen wrote: > Shouldn't you set clickable to be false here? Done. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:194: view.setClickable(false); On 2016/10/28 22:17:39, Ian Wen wrote: > BTW this can be done in xml, so you don't need to do it in java. Done. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:51: int paddingTop = (int) (density * PADDING_TOP); On 2016/10/28 22:17:39, Ian Wen wrote: > Don't do this... Add the padding into dimens.xml and get that dimension from > resource. > > Finding density and doing it your self is not android typical style. Done. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:54: layoutParams.setMargins(0, paddingTop, 0, 0); On 2016/10/28 22:17:39, Ian Wen wrote: > Also if this is used as margin, the variable name should be marginTop, not > paddingTop. Done. https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:69: return IsPrepopulatedTemplate(template_url) || On 2016/10/28 23:05:15, Peter Kasting wrote: > Nit: Seems like the body of this method can now be inlined here. > > At that point this seems closely related to TemplateURLService::CanReplace(), > except for the safe_for_autoreplace() check. Can/should we move this function > to that class and build CanReplace() off it? Done. https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:83: for (size_t i = 0; i < template_urls.size(); ++i) { On 2016/10/28 23:05:15, Peter Kasting wrote: > I'm slightly confused by what this function is doing. I think I mostly follow > it, but can you break into more blocks and give high-level comments on the > blocks about what we're trying to do? It may also help to give more > function-level comments in the header. > > I may have more comments on this after you do that. Done. https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:149: return NULL; On 2016/10/28 23:05:14, Peter Kasting wrote: > Nit: nullptr Done. https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:99: std::vector<TemplateURL*> template_url_list_; On 2016/10/28 23:05:15, Peter Kasting wrote: > Nit: Goes above DISALLOW_COPY_AND_ASSIGN. > > Avoid using "list" in the name of a variable that isn't a list. How about > |template_urls_|? Done.
Er, guess you uploaded a new patch. I'll send these out and review the new one. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine.xml (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:31: android:textColor="#565656" /> The mock looks like it's using #000000 for the site title, #646464 for the URL, and whatever active_state_color is for the third line: https://storage.googleapis.com/monorail-prod.appspot.com/16/attachments/82f35... Do you have a screenshot of what this CL looks like? https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:12: android:textSize="15sp" Where'd you get the 15sp number from? 16sp is far more common in the code. Same for your other use of 15sp. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:60: private List<TemplateUrl> mPrepopulatedSearchEngines = new ArrayList<TemplateUrl>(); Add newlines between the previous variable and the comments so that this is easier to parse.
https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine.xml (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:31: android:textColor="#565656" /> On 2016/10/31 22:31:28, dfalcantara (slow 10.24) wrote: > The mock looks like it's using #000000 for the site title, #646464 for the URL, > and whatever active_state_color is for the third line: > > https://storage.googleapis.com/monorail-prod.appspot.com/16/attachments/82f35... > > Do you have a screenshot of what this CL looks like? Here is the screenshot of current setting: https://bugs.chromium.org/p/chromium/issues/detail?id=649443&can=1&q=owner%3A... But I think your suggestion gives a better look for the UI. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:12: android:textSize="15sp" On 2016/10/31 22:31:28, dfalcantara (slow 10.24) wrote: > Where'd you get the 15sp number from? 16sp is far more common in the code. > Same for your other use of 15sp. I might referred to a file that is not a common case. Change it to 16sp. https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/180001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:60: private List<TemplateUrl> mPrepopulatedSearchEngines = new ArrayList<TemplateUrl>(); On 2016/10/31 22:31:28, dfalcantara (slow 10.24) wrote: > Add newlines between the previous variable and the comments so that this is > easier to parse. Done.
https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine.xml (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:37: android:textColor="#646464" /> @color/descriptive_text_color https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:39: android:id="@+id/link" These TextViews should be renamed: the "description" is actually the name of the search engine, and the "link" is more or less clarification text. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:47: android:paddingTop="12dp" /> This should have the color set, right? android:textColor="@color/light_active_color" https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:11: android:textStyle="bold" I think you're actually trying to find Roboto Medium. Apply this style instead of bolding the text manually so that you get something that looks about right on all Android versions: style="@style/RobotoMediumStyle" https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:189: if (itemViewType == VIEW_TYPE_DIVIDER) { I'm not sure this'll work: you're inflating two different layouts but only checking if the convertView is non-null. If you get into a situation where the view is supposed to be re-used but has the wrong layout, it'll crash because it won't have the correct fields in it. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:19: private static final int PADDING_TOP = 6; nit: Remove. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:35: String getKeywordFromIndexTesting(int index) { ForTesting https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:50: float pxToDp = 1.0f / getActivity().getResources().getDisplayMetrics().density; Instead of manually calculating this you should be setting values in dimens.xml to get them automatically. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:57: @IntDef({TYPE_PREPOPULATED, TYPE_DEFAULT, TYPE_RECENT}) Reorder these so that they match the value ordering above. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:71: private int mTemplateUrlType; maybe @TemplateUrlType private int mTemplateUrlType? https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:84: Matcher m = pattern.matcher(url); mUrl = m.find() ? m.group(0) : ""; https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:260: <message name="IDS_SEARCH_ENGINE_RECENTLY_VISITED" desc="The title of recently visited search engine list view."> Title indicating that everything following represents recently visited pages that may support search queries?
https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine.xml (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:37: android:textColor="#646464" /> On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > @color/descriptive_text_color Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:39: android:id="@+id/link" On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > These TextViews should be renamed: the "description" is actually the name of the > search engine, and the "link" is more or less clarification text. Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:47: android:paddingTop="12dp" /> On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > This should have the color set, right? > > android:textColor="@color/light_active_color" The color setting is coded in SearchEngineAdapter.java by "ForegroundColorSpan linkSpan = new ForegroundColorSpan( ApiCompatibilityUtils.getColor(resources, R.color.pref_accent_color));" and R.color.pref_accent_color = @color/light_active_color. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:11: android:textStyle="bold" On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > I think you're actually trying to find Roboto Medium. Apply this style instead > of bolding the text manually so that you get something that looks about right on > all Android versions: > > style="@style/RobotoMediumStyle" Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:189: if (itemViewType == VIEW_TYPE_DIVIDER) { On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > I'm not sure this'll work: you're inflating two different layouts but only > checking if the convertView is non-null. If you get into a situation where the > view is supposed to be re-used but has the wrong layout, it'll crash because it > won't have the correct fields in it. The data is only populated in the constructor and the order will not be changed after users' settings. So the convertView should always be matched to the same data and layout. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:19: private static final int PADDING_TOP = 6; On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > nit: Remove. Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:35: String getKeywordFromIndexTesting(int index) { On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > ForTesting Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEnginePreference.java:50: float pxToDp = 1.0f / getActivity().getResources().getDisplayMetrics().density; On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > Instead of manually calculating this you should be setting values in dimens.xml > to get them automatically. Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:57: @IntDef({TYPE_PREPOPULATED, TYPE_DEFAULT, TYPE_RECENT}) On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > Reorder these so that they match the value ordering above. Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:71: private int mTemplateUrlType; On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > maybe > > @TemplateUrlType private int mTemplateUrlType? Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:84: Matcher m = pattern.matcher(url); On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > mUrl = m.find() ? m.group(0) : ""; Done. https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:260: <message name="IDS_SEARCH_ENGINE_RECENTLY_VISITED" desc="The title of recently visited search engine list view."> On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > Title indicating that everything following represents recently visited pages > that may support search queries? Done.
https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/220001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:189: if (itemViewType == VIEW_TYPE_DIVIDER) { On 2016/11/01 22:59:36, ltian wrote: > On 2016/11/01 21:30:29, dfalcantara (slow 10.24) wrote: > > I'm not sure this'll work: you're inflating two different layouts but only > > checking if the convertView is non-null. If you get into a situation where > the > > view is supposed to be re-used but has the wrong layout, it'll crash because > it > > won't have the correct fields in it. > > The data is only populated in the constructor and the order will not be changed > after users' settings. So the convertView should always be matched to the same > data and layout. Doesn't matter if the order changes -- if the user scrolls far enough down on the list, Android will recycle what Views are being shown to show new items. If you add enough items here, eventually the Recently visited View will be recycled for a regular radio button search engine layout and then cause a crash.
https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:70: const JavaParamRef<jobject>& obj) { It seems like it might be simpler to implement the majority this function as a simple sort() call, using a comparator that always sorted prepopulated engines before others, and otherwise sorted the current default above others. Then we wouldn't have to make multiple passes through multiple lists to try to construct things. This would not address my comment below about a recency check. https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:78: // Add only IsInDefaultList() providers to the template_urls_. Nit: Blank line separating a comment + block of code from the unrelated block above. (Several places) https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:88: recent_visit_list.push_back(template_urls[i]); Doesn't this need some kind of recency check? Otherwise it's not really a "recent_visit_list", it's more like an "everything else" list. https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:283: base::android::ConvertJavaStringToUTF16(env, jkeyword)); Nit: Prefer assignment to constructor-style init for strings like this; see http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variabl... . https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:31: // Update template_urls and return the index of default search provider in the Nit: Function comments should be descriptive ("Updates") rather than imperative ("Update"); see http://google.github.io/styleguide/cppguide.html#Function_Comments . I would place a blank line above this comment and below the function, to separate it from the other functions this comment is not talking about. Bonus points if you add similar descriptive comments to any other functions which wouldn't be instantly clear to readers, or have surprising side effects. This comment refers to "template_urls", which isn't a parameter. Did you mean |template_urls_|? It says "updates"; updates how? In a way different from the last sentence of the comment (which is what "Note" kind of implies")? Or does that cover this? If so make this clearer, e.g.: Returns the index of the default search provider within |template_urls_|. If this is a custom search engine (as opposed to a prepopulated one), the list will be reordered so it comes before all other custom engines. However, I still think it's confusing and error-prone that this getter has obviously-non-const behavior on the class, and this reordering should be something you do elsewhere (see comment on previous patch set, why you didn't reply to). A caller that did this seemingly-benign pseudocode would go badly wrong: for (i = 0; i < GetTemplateUrlCount(); ++i) AddItemToMenu(GetTemplateUrlAt(i)); SelectMenuItem(GetDefaultSearchProvider()); In fact it's even worse than it looks, because before calling GetDefaultSearchProvider() the first time, |template_urls_| is totally empty; this function doesn't just reorder the list, it populates it. I really think you need to change this design. You can already observe when things in the base TemplateURLService change, so you can keep a parallel list in sync. https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:34: // of Nit: Rewrap https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:36: jint GetDefaultSearchProvider( Nit: GetDefaultSearchProviderIndex()? https://codereview.chromium.org/2367373003/diff/240001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/240001/components/search_engi... components/search_engines/template_url_service.h:154: // Returns true if the TempalteURL is prepopulated or created by policy. Nit: TemplateURL https://codereview.chromium.org/2367373003/diff/240001/components/search_engi... components/search_engines/template_url_service.h:155: bool IsInDefaultList(const TemplateURL* url); It seems like this is really a const member method on TemplateURL, no? It doesn't have anything to do with the TemplateURLService. When moving it there, you're going to run up against ShowInDefaultList() and show_in_default_list(). I don't think we should have all three of these functions. We should probably have just one. I propose that Android's definition of the list here -- the prepopulated/policy URLs, the current default, and a recently-used engine or two -- be what we use on desktop, and we use that for both the dropdown in the main settings on desktop and the top section of the search engine management dialog. Basically, I think we should retire the existing show_in_default_list bit. We should probably also enforce (in all these places) that the TemplateURL supports replacement, as ShowInDefaultList() does.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:70: const JavaParamRef<jobject>& obj) { On 2016/11/02 00:34:03, Peter Kasting wrote: > It seems like it might be simpler to implement the majority this function as a > simple sort() call, using a comparator that always sorted prepopulated engines > before others, and otherwise sorted the current default above others. > > Then we wouldn't have to make multiple passes through multiple lists to try to > construct things. > > This would not address my comment below about a recency check. Done. https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:78: // Add only IsInDefaultList() providers to the template_urls_. On 2016/11/02 00:34:02, Peter Kasting wrote: > Nit: Blank line separating a comment + block of code from the unrelated block > above. (Several places) Done. https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:88: recent_visit_list.push_back(template_urls[i]); On 2016/11/02 00:34:02, Peter Kasting wrote: > Doesn't this need some kind of recency check? Otherwise it's not really a > "recent_visit_list", it's more like an "everything else" list. Will write a sort function to simplify these operation. "recent_visit_list" is not needed anymore. https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:283: base::android::ConvertJavaStringToUTF16(env, jkeyword)); On 2016/11/02 00:34:02, Peter Kasting wrote: > Nit: Prefer assignment to constructor-style init for strings like this; see > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variabl... > . Sorry I am a little confused. Is the constructor-style init the second option shown in your reference? I think this keyword is assigned in that way? https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:31: // Update template_urls and return the index of default search provider in the On 2016/11/02 00:34:03, Peter Kasting wrote: > Nit: Function comments should be descriptive ("Updates") rather than imperative > ("Update"); see > http://google.github.io/styleguide/cppguide.html#Function_Comments . > > I would place a blank line above this comment and below the function, to > separate it from the other functions this comment is not talking about. Bonus > points if you add similar descriptive comments to any other functions which > wouldn't be instantly clear to readers, or have surprising side effects. > > This comment refers to "template_urls", which isn't a parameter. Did you mean > |template_urls_|? It says "updates"; updates how? In a way different from the > last sentence of the comment (which is what "Note" kind of implies")? Or does > that cover this? If so make this clearer, e.g.: > > Returns the index of the default search provider within |template_urls_|. If > this is a custom search engine (as opposed to a prepopulated one), the list will > be reordered so it comes before all other custom engines. > > However, I still think it's confusing and error-prone that this getter has > obviously-non-const behavior on the class, and this reordering should be > something you do elsewhere (see comment on previous patch set, why you didn't > reply to). A caller that did this seemingly-benign pseudocode would go badly > wrong: > > for (i = 0; i < GetTemplateUrlCount(); ++i) > AddItemToMenu(GetTemplateUrlAt(i)); > SelectMenuItem(GetDefaultSearchProvider()); > > In fact it's even worse than it looks, because before calling > GetDefaultSearchProvider() the first time, |template_urls_| is totally empty; > this function doesn't just reorder the list, it populates it. > > I really think you need to change this design. You can already observe when > things in the base TemplateURLService change, so you can keep a parallel list in > sync. I will wrap the operations about populating and sorting |template_urls_| in another function and in this function, it first calls that function to reload all data and then returns the index of default provider. Does this sound a better design? https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:34: // of On 2016/11/02 00:34:03, Peter Kasting wrote: > Nit: Rewrap Done. https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:36: jint GetDefaultSearchProvider( On 2016/11/02 00:34:03, Peter Kasting wrote: > Nit: GetDefaultSearchProviderIndex()? Done. https://codereview.chromium.org/2367373003/diff/240001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/240001/components/search_engi... components/search_engines/template_url_service.h:154: // Returns true if the TempalteURL is prepopulated or created by policy. On 2016/11/02 00:34:03, Peter Kasting wrote: > Nit: TemplateURL Done. https://codereview.chromium.org/2367373003/diff/240001/components/search_engi... components/search_engines/template_url_service.h:155: bool IsInDefaultList(const TemplateURL* url); On 2016/11/02 00:34:03, Peter Kasting wrote: > It seems like this is really a const member method on TemplateURL, no? It > doesn't have anything to do with the TemplateURLService. > > When moving it there, you're going to run up against ShowInDefaultList() and > show_in_default_list(). I don't think we should have all three of these > functions. We should probably have just one. > > I propose that Android's definition of the list here -- the prepopulated/policy > URLs, the current default, and a recently-used engine or two -- be what we use > on desktop, and we use that for both the dropdown in the main settings on > desktop and the top section of the search engine management dialog. Basically, > I think we should retire the existing show_in_default_list bit. > > We should probably also enforce (in all these places) that the TemplateURL > supports replacement, as ShowInDefaultList() does. Not exist any more.
https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:283: base::android::ConvertJavaStringToUTF16(env, jkeyword)); On 2016/11/22 21:51:41, ltian wrote: > On 2016/11/02 00:34:02, Peter Kasting wrote: > > Nit: Prefer assignment to constructor-style init for strings like this; see > > > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variabl... > > . > > Sorry I am a little confused. Is the constructor-style init the second option > shown in your reference? I think this keyword is assigned in that way? Yes, and as the reference notes, you should prefer assignment-style for things that are, effectively, a simple move of some data into the object; a string is given as an explicit sample in the first option ("std::string s = "Hello";"). https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:31: // Update template_urls and return the index of default search provider in the On 2016/11/22 21:51:41, ltian wrote: > On 2016/11/02 00:34:03, Peter Kasting wrote: > > I still think it's confusing and error-prone that this getter has > > obviously-non-const behavior on the class, and this reordering should be > > something you do elsewhere (see comment on previous patch set, why you didn't > > reply to). A caller that did this seemingly-benign pseudocode would go badly > > wrong: > > > > for (i = 0; i < GetTemplateUrlCount(); ++i) > > AddItemToMenu(GetTemplateUrlAt(i)); > > SelectMenuItem(GetDefaultSearchProvider()); > > > > In fact it's even worse than it looks, because before calling > > GetDefaultSearchProvider() the first time, |template_urls_| is totally empty; > > this function doesn't just reorder the list, it populates it. > > > > I really think you need to change this design. You can already observe when > > things in the base TemplateURLService change, so you can keep a parallel list > in > > sync. > > I will wrap the operations about populating and sorting |template_urls_| in > another function and in this function, it first calls that function to reload > all data and then returns the index of default provider. Does this sound a > better design? Are you planning to call these methods from every function that can directly or indirectly access this data? For example, GetTemplateUrlCount() needs this too. Then will you make the relevant methods const and |template_urls_| mutable, so it's clear that, from an API perspective, calling this does not change observable state? If so, then what you're describing is a pull-based system that uses |template_urls_| as a cache which is updated on each attempted access. One of my core questions is whether that's a better design than a push-based system which updates this on every change to the underlying URLs.
https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:283: base::android::ConvertJavaStringToUTF16(env, jkeyword)); On 2016/11/22 21:57:10, Peter Kasting wrote: > On 2016/11/22 21:51:41, ltian wrote: > > On 2016/11/02 00:34:02, Peter Kasting wrote: > > > Nit: Prefer assignment to constructor-style init for strings like this; see > > > > > > http://dev.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Variabl... > > > . > > > > Sorry I am a little confused. Is the constructor-style init the second option > > shown in your reference? I think this keyword is assigned in that way? > > Yes, and as the reference notes, you should prefer assignment-style for things > that are, effectively, a simple move of some data into the object; a string is > given as an explicit sample in the first option ("std::string s = "Hello";"). Got it, thx! https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:31: // Update template_urls and return the index of default search provider in the On 2016/11/22 21:57:10, Peter Kasting wrote: > On 2016/11/22 21:51:41, ltian wrote: > > On 2016/11/02 00:34:03, Peter Kasting wrote: > > > I still think it's confusing and error-prone that this getter has > > > obviously-non-const behavior on the class, and this reordering should be > > > something you do elsewhere (see comment on previous patch set, why you > didn't > > > reply to). A caller that did this seemingly-benign pseudocode would go > badly > > > wrong: > > > > > > for (i = 0; i < GetTemplateUrlCount(); ++i) > > > AddItemToMenu(GetTemplateUrlAt(i)); > > > SelectMenuItem(GetDefaultSearchProvider()); > > > > > > In fact it's even worse than it looks, because before calling > > > GetDefaultSearchProvider() the first time, |template_urls_| is totally > empty; > > > this function doesn't just reorder the list, it populates it. > > > > > > I really think you need to change this design. You can already observe when > > > things in the base TemplateURLService change, so you can keep a parallel > list > > in > > > sync. > > > > I will wrap the operations about populating and sorting |template_urls_| in > > another function and in this function, it first calls that function to reload > > all data and then returns the index of default provider. Does this sound a > > better design? > > Are you planning to call these methods from every function that can directly or > indirectly access this data? For example, GetTemplateUrlCount() needs this too. > Then will you make the relevant methods const and |template_urls_| mutable, so > it's clear that, from an API perspective, calling this does not change > observable state? > > If so, then what you're describing is a pull-based system that uses > |template_urls_| as a cache which is updated on each attempted access. One of > my core questions is whether that's a better design than a push-based system > which updates this on every change to the underlying URLs. Yes, it is a pull-based system and I think it is fine because the Android side is also a pull-based system. The Android UI will not be updated once it is initiated(By initEntries() in SearchEngineAdapter.java). And I think LoadTemplateURLs() is not needed to be called from every function accessing this data. It only need to be called by GetDefaultSearchProviderIndex() because that is the very first function to get data from TemplateURLService once the service is loaded. So basically the workflow is when Android side tries to populate UI, it calls the initEntries() in SearchEngineAdapter.java, the initEntries() first calls GetDefaultSearchProviderIndex() and GetDefaultSearchProviderIndex() loads |template_urls_| and |default_search_provider_index_|. Then later every Android side call will directly retrieves data from those two fields. The two field will only be reloaded once Android side loads UI again.
https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/240001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:31: // Update template_urls and return the index of default search provider in the On 2016/11/22 23:25:10, ltian wrote: > On 2016/11/22 21:57:10, Peter Kasting wrote: > > On 2016/11/22 21:51:41, ltian wrote: > > > On 2016/11/02 00:34:03, Peter Kasting wrote: > > > > I still think it's confusing and error-prone that this getter has > > > > obviously-non-const behavior on the class, and this reordering should be > > > > something you do elsewhere (see comment on previous patch set, why you > > didn't > > > > reply to). A caller that did this seemingly-benign pseudocode would go > > badly > > > > wrong: > > > > > > > > for (i = 0; i < GetTemplateUrlCount(); ++i) > > > > AddItemToMenu(GetTemplateUrlAt(i)); > > > > SelectMenuItem(GetDefaultSearchProvider()); > > > > > > > > In fact it's even worse than it looks, because before calling > > > > GetDefaultSearchProvider() the first time, |template_urls_| is totally > > empty; > > > > this function doesn't just reorder the list, it populates it. > > > > > > > > I really think you need to change this design. You can already observe > when > > > > things in the base TemplateURLService change, so you can keep a parallel > > list > > > in > > > > sync. > > > > > > I will wrap the operations about populating and sorting |template_urls_| in > > > another function and in this function, it first calls that function to > reload > > > all data and then returns the index of default provider. Does this sound a > > > better design? > > > > Are you planning to call these methods from every function that can directly > or > > indirectly access this data? For example, GetTemplateUrlCount() needs this > too. > > Then will you make the relevant methods const and |template_urls_| mutable, > so > > it's clear that, from an API perspective, calling this does not change > > observable state? > > > > If so, then what you're describing is a pull-based system that uses > > |template_urls_| as a cache which is updated on each attempted access. One of > > my core questions is whether that's a better design than a push-based system > > which updates this on every change to the underlying URLs. > > Yes, it is a pull-based system and I think it is fine because the Android side > is also a pull-based system. The Android UI will not be updated once it is > initiated(By initEntries() in SearchEngineAdapter.java). Then why not force the caller to use that access pattern? Eliminate most of the functions here and replace them with some kind of thing that just returns the full list of relevant TemplateURLs, in order, plus the index of the default search URL in that list. Then there's no need to cache anything; you just compute this each time. The API becomes simpler and safer. And the caller can cache this list or do whatever they want with it. > And I think LoadTemplateURLs() is not needed to be called from every function > accessing this data. It only need to be called by > GetDefaultSearchProviderIndex() because that is the very first function to get > data from TemplateURLService once the service is loaded. But can you guarantee that won't change in the future? What if someone adds a call to one of these other functions from elsewhere later, since the API looks like that's a reasonable thing to do? > So basically the workflow is when Android side tries to populate UI, it calls > the initEntries() in SearchEngineAdapter.java, the initEntries() first calls > GetDefaultSearchProviderIndex() and GetDefaultSearchProviderIndex() loads > |template_urls_| and |default_search_provider_index_|. Then later every Android > side call will directly retrieves data from those two fields. The two field will > only be reloaded once Android side loads UI again. Right, the problem is that this code cannot guarantee that's how all callers for all time will use it, and the API does not make it hard or impossible to use it in some other way.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Did not do a full review https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:54: } Nit: Shorter: return !template_url_service->ShowInPrepopulatedList(rhs) || (lhs->prepopulate_id() < rhs->prepopulate_id()); (Also avoids else after return) https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:55: } else if (lhs == default_search_provider && Nit: Shorter: } return (lhs == default_search_provider) && !template_url_service->ShowInPrepopulatedList(rhs); (Also avoids else after return) After making both the changes above, this function is probably short enough to declare as a lambda, in the target function where you'll be using it. This reduces the boilerplate pretty significantly. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:109: LoadTemplateURLs(); It seems like we shouldn't put this here. If the TemplateURLService is possibly already loaded by the time this class is constructed, then this needs to happen in the constructor, not here. Then this method should be made const. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:152: if (!template_url) When can this be true? It seems like the list of TemplateURLs should never contain a null pointer. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:189: } Nit: More efficient, and allows removing the init of |default_search_provider_index_| above (so it's shorter on net): auto it = std::find(template_urls_.begin(), template_urls_.end(), default_search_provider); default_search_provider_index_ = (it == template_urls_.end()) ? -1 : (it - template_urls_.begin()); Note that we could just do this in GetDefaultSearchProviderIndex() and avoid the member variable entirely, as this variable is not accessed outside that function, so there's no risk of anything getting "out of sync". https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:34: // within |it. This comment lies, since it doesn't update this variable. In general, I'd avoid talking about |template_urls_| anyway in the public API comments, since that's an implementation detail. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:92: void LoadTemplateURLs(); Nit: Add comments about what this does. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:101: std::vector<TemplateURL*> template_urls_; Nit: Add comments about why we need to cache this. https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... components/search_engines/template_url_service.cc:405: t_url->SupportsReplacement(search_terms_data())) || I don't think we should explicitly check SupportsReplacement() here, since if something is already the default search provider, it presumably supports replacement, but even if somehow it doesn't, it surely should be shown in the default list. https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... components/search_engines/template_url_service.h:137: // prepopulated by Chrome browser. Nit: Typo + grammar Because you are splitting the existing functionality of ShowInDefaultList() into pieces, you should aim to split its comment into pieces as well. All that stuff about "most likely choices of default engine" probably belongs over here, and the two function comments should be written to make it clear how one is a superset of the other.
Didn't look at the native stuff too closely, yet. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine.xml (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:31: android:textColor="@color/fre_text_color" /> I'd just use default_text_color and then tell Rebecca. It'd be more consistent with the rest of the UI. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:8: android:layout_width="match_parent" nit: Group the layout_* params at the top. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:52: Remove newline above https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:55: // The current context. Switch these to /** javadoc style */ comments so they pop up in IDEs when you hover over them. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:62: private List<TemplateUrl> mPrepopulatedSearchEngines = new ArrayList<TemplateUrl>(); = new ArrayList<>(); https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:89: String getValueForTesting() { Adding @VisibleForTesting or naming the function SomethingForTesting() usually implies that without the comment. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:132: // Add one to offset the title for recent search engine list 1) Add period to end of the comment 2) the title for the recent search engine list https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:146: position -= mPrepopulatedSearchEngines.size() + 1; 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 :/ https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:200: } view = mLayoutInflater.inflate(itemViewType == VIEW_TYPE_DIVIDER ? R.layout.search_engine_recent_title : R.layout.search_engine, null); https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:59: public @interface TemplateUrlType {} I'd stick the public static final int TYPE_DEFAULT, etc fields underneath, like how ChromeTabbedActivity and SearchEnginePromoDialog do it. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:71: private int mTemplateUrlType; Maybe just inline it to make this look a little cleaner? @TemplateUrlType private int mTemplateUrlType https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:260: <message name="IDS_SEARCH_ENGINE_RECENTLY_VISITED" desc="Divider between prepopulated and recently visited search engines."> Maybe desc -> "Header for the list of recently visited search engines"? Divider doesn't say much.
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine.xml (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/search_engine.xml:31: android:textColor="@color/fre_text_color" /> On 2016/11/28 21:37:01, dfalcantara (check my queue) wrote: > I'd just use default_text_color and then tell Rebecca. It'd be more consistent > with the rest of the UI. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... File chrome/android/java/res/layout/search_engine_recent_title.xml (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/re... chrome/android/java/res/layout/search_engine_recent_title.xml:8: android:layout_width="match_parent" On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > nit: Group the layout_* params at the top. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:52: On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > Remove newline above Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:55: // The current context. On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > Switch these to /** javadoc style */ comments so they pop up in IDEs when you > hover over them. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:62: private List<TemplateUrl> mPrepopulatedSearchEngines = new ArrayList<TemplateUrl>(); On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > = new ArrayList<>(); Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:89: String getValueForTesting() { On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > Adding @VisibleForTesting or naming the function SomethingForTesting() usually > implies that without the comment. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:132: // Add one to offset the title for recent search engine list On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > 1) Add period to end of the comment > 2) the title for the recent search engine list Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:146: position -= mPrepopulatedSearchEngines.size() + 1; On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > 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 :/ Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:200: } On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > view = mLayoutInflater.inflate(itemViewType == VIEW_TYPE_DIVIDER ? > R.layout.search_engine_recent_title : R.layout.search_engine, null); Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:59: public @interface TemplateUrlType {} On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > I'd stick the public static final int TYPE_DEFAULT, etc fields underneath, like > how ChromeTabbedActivity and SearchEnginePromoDialog do it. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/search_engines/TemplateUrlService.java:71: private int mTemplateUrlType; On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > Maybe just inline it to make this look a little cleaner? > > @TemplateUrlType private int mTemplateUrlType Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/st... File chrome/android/java/strings/android_chrome_strings.grd (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/android/java/st... chrome/android/java/strings/android_chrome_strings.grd:260: <message name="IDS_SEARCH_ENGINE_RECENTLY_VISITED" desc="Divider between prepopulated and recently visited search engines."> On 2016/11/28 21:37:02, dfalcantara (check my queue) wrote: > Maybe desc -> "Header for the list of recently visited search engines"? Divider > doesn't say much. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:54: } On 2016/11/23 21:30:27, Peter Kasting wrote: > Nit: Shorter: > > return !template_url_service->ShowInPrepopulatedList(rhs) || > (lhs->prepopulate_id() < rhs->prepopulate_id()); > > (Also avoids else after return) Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:55: } else if (lhs == default_search_provider && On 2016/11/23 21:30:27, Peter Kasting wrote: > Nit: Shorter: > > } > return (lhs == default_search_provider) && > !template_url_service->ShowInPrepopulatedList(rhs); > > (Also avoids else after return) > > After making both the changes above, this function is probably short enough to > declare as a lambda, in the target function where you'll be using it. This > reduces the boilerplate pretty significantly. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:109: LoadTemplateURLs(); On 2016/11/23 21:30:27, Peter Kasting wrote: > It seems like we shouldn't put this here. If the TemplateURLService is possibly > already loaded by the time this class is constructed, then this needs to happen > in the constructor, not here. > > Then this method should be made const. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:152: if (!template_url) On 2016/11/23 21:30:27, Peter Kasting wrote: > When can this be true? It seems like the list of TemplateURLs should never > contain a null pointer. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:189: } On 2016/11/23 21:30:27, Peter Kasting wrote: > Nit: More efficient, and allows removing the init of > |default_search_provider_index_| above (so it's shorter on net): > > auto it = std::find(template_urls_.begin(), template_urls_.end(), > default_search_provider); > default_search_provider_index_ = (it == template_urls_.end()) > ? -1 > : (it - template_urls_.begin()); > > Note that we could just do this in GetDefaultSearchProviderIndex() and avoid the > member variable entirely, as this variable is not accessed outside that > function, so there's no risk of anything getting "out of sync". That is really a great way to do this. Thanks for pointing this out! https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:34: // within |it. On 2016/11/23 21:30:27, Peter Kasting wrote: > This comment lies, since it doesn't update this variable. > > In general, I'd avoid talking about |template_urls_| anyway in the public API > comments, since that's an implementation detail. Sorry I forgot to update the comment. I think for current design, the function name is pretty clear, so there is no need for any comment. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:92: void LoadTemplateURLs(); On 2016/11/23 21:30:27, Peter Kasting wrote: > Nit: Add comments about what this does. Done. https://codereview.chromium.org/2367373003/diff/280001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:101: std::vector<TemplateURL*> template_urls_; On 2016/11/23 21:30:27, Peter Kasting wrote: > Nit: Add comments about why we need to cache this. Done. https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... components/search_engines/template_url_service.cc:405: t_url->SupportsReplacement(search_terms_data())) || On 2016/11/23 21:30:27, Peter Kasting wrote: > I don't think we should explicitly check SupportsReplacement() here, since if > something is already the default search provider, it presumably supports > replacement, but even if somehow it doesn't, it surely should be shown in the > default list. Done. https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/280001/components/search_engi... components/search_engines/template_url_service.h:137: // prepopulated by Chrome browser. On 2016/11/23 21:30:27, Peter Kasting wrote: > Nit: Typo + grammar > > Because you are splitting the existing functionality of ShowInDefaultList() into > pieces, you should aim to split its comment into pieces as well. All that stuff > about "most likely choices of default engine" probably belongs over here, and > the two function comments should be written to make it clear how one is a > superset of the other. Done.
https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:45: if (template_url_service_->loaded() && template_urls_.size() == 0) { Nit: Prefer .empty() to .size() == 0. {} not necessary https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:79: : (it - template_urls_.begin()); Nit: Indenting looks a bit odd. Is this what git cl format did? https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:147: template_urls_.clear(); This call is unnecessary, since you reset template_urls_ just below. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:153: auto typeCompare = [&] (const TemplateURL* lhs, const TemplateURL* rhs) Nit: Use normal variable capitalization scheme. I don't really know what "type compare" as a name means, since you're not comparing the types of anything, in the C++ sense of the word type. I might use "comp", which matches the name used in the standard for this argument to sort(). https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:154: -> bool { Nit: Is the return type necessary for compilation? If not, I'd probably omit it for brevity, as I'm not sure it adds much. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:156: return !template_url_service_->ShowInPrepopulatedList(rhs) || Nit: This call is verbose and you make it regardless of the value of the conditional; maybe store in a temp like |rhs_prepopulated|? https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:162: Nit: I'd avoid this blank line, since the lambda just above it is declared solely for the call below. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:91: // default search provider with it. This comment is incorrect, since the function does not calculate a default search provider index. There have been several cases in this CL where comments haven't been updated correctly from one patch set to another. Please be careful to keep your comments up to date. Reviewers now and readers later will treat your comments as more important than the code itself. I don't know whether you should clarify the "sorts" bit at all. Perhaps a comment like this: Updates |template_urls_| to contain everything for which TemplateURLService::ShowInDefaultList() returns true. Sorts this list with prepopulated engines first, then any default non-prepopulated engine, then other non-prepopulated engines. https://codereview.chromium.org/2367373003/diff/300001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/300001/components/search_engi... components/search_engines/template_url_service.h:136: // Retruns whether |template_url| should be shown in the list of engines Nit: Typo https://codereview.chromium.org/2367373003/diff/300001/components/search_engi... components/search_engines/template_url_service.h:144: // from a full list of all TemplateURLs (which might be very long). These comments are still confusingly overlapping. What about this for the function comment and name for the new function you're adding? With this, I think the ShowInDefaultList() comment doesn't need a change at all. // Returns whether the engine is a "pre-existing" engine, either from the // prepopulate list or created by policy. bool IsPrepopulatedOrCreatedByPolicy(...);
Java bits lgtm https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:56: * The current context. nit: You can save some room using the inline syntax: /** The current context. */ https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:99: // Used for testing. You can toss the comment now. https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:171: : mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size() + 1; nit: I'd just put "? 0" and ": everything else" on the same line if they fit. https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:384: public int computeStartIndexForRecentSearchEngines() { Make it private or add a javadoc
https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:171: : mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size() + 1; On 2016/11/29 18:43:10, dfalcantara (check my queue) wrote: > nit: I'd just put "? 0" and ": everything else" on the same line if they fit. (Note that clang-format for C++ code will force these to be on different lines unless everything fits on the same line as the condition. I assume it does something similar for Java.)
https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:171: : mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size() + 1; On 2016/11/29 20:47:44, Peter Kasting wrote: > On 2016/11/29 18:43:10, dfalcantara (check my queue) wrote: > > nit: I'd just put "? 0" and ": everything else" on the same line if they fit. > > (Note that clang-format for C++ code will force these to be on different lines > unless everything fits on the same line as the condition. I assume it does > something similar for Java.) clang-format makes a couple of strange decisions for indentation of Java code in several situations (e.g. indenting by 16 when it should indent by 8); I'm honestly not sure what it would do here, though I hope it'd be reasonable... Lei: If you run "git cl format" does this bit change any?
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg 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) chromeos_x86-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_chromeos_compile_dbg_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_clobber_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by ltian@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:56: * The current context. On 2016/11/29 18:43:11, dfalcantara (check my queue) wrote: > nit: You can save some room using the inline syntax: > > /** The current context. */ Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:99: // Used for testing. On 2016/11/29 18:43:11, dfalcantara (check my queue) wrote: > You can toss the comment now. Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:171: : mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size() + 1; On 2016/11/29 18:43:10, dfalcantara (check my queue) wrote: > nit: I'd just put "? 0" and ": everything else" on the same line if they fit. Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:171: : mPrepopulatedSearchEngines.size() + mRecentSearchEngines.size() + 1; On 2016/11/29 21:19:47, dfalcantara (check my queue) wrote: > On 2016/11/29 20:47:44, Peter Kasting wrote: > > On 2016/11/29 18:43:10, dfalcantara (check my queue) wrote: > > > nit: I'd just put "? 0" and ": everything else" on the same line if they > fit. > > > > (Note that clang-format for C++ code will force these to be on different lines > > unless everything fits on the same line as the condition. I assume it does > > something similar for Java.) > > clang-format makes a couple of strange decisions for indentation of Java code in > several situations (e.g. indenting by 16 when it should indent by 8); I'm > honestly not sure what it would do here, though I hope it'd be reasonable... > > Lei: If you run "git cl format" does this bit change any? Yeah, git cl format puts "? 0" in a separate line as it is shown before. https://codereview.chromium.org/2367373003/diff/300001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/preferences/SearchEngineAdapter.java:384: public int computeStartIndexForRecentSearchEngines() { On 2016/11/29 18:43:11, dfalcantara (check my queue) wrote: > Make it private or add a javadoc Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.cc (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:45: if (template_url_service_->loaded() && template_urls_.size() == 0) { On 2016/11/29 03:32:18, Peter Kasting wrote: > Nit: Prefer .empty() to .size() == 0. {} not necessary Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:79: : (it - template_urls_.begin()); On 2016/11/29 03:32:18, Peter Kasting wrote: > Nit: Indenting looks a bit odd. Is this what git cl format did? Sorry the Eclipse setting and git cl format do in different ways. Change it to use git cl format. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:147: template_urls_.clear(); On 2016/11/29 03:32:18, Peter Kasting wrote: > This call is unnecessary, since you reset template_urls_ just below. Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:153: auto typeCompare = [&] (const TemplateURL* lhs, const TemplateURL* rhs) On 2016/11/29 03:32:18, Peter Kasting wrote: > Nit: Use normal variable capitalization scheme. > > I don't really know what "type compare" as a name means, since you're not > comparing the types of anything, in the C++ sense of the word type. I might use > "comp", which matches the name used in the standard for this argument to sort(). Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:154: -> bool { On 2016/11/29 03:32:18, Peter Kasting wrote: > Nit: Is the return type necessary for compilation? If not, I'd probably omit it > for brevity, as I'm not sure it adds much. Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:156: return !template_url_service_->ShowInPrepopulatedList(rhs) || On 2016/11/29 03:32:18, Peter Kasting wrote: > Nit: This call is verbose and you make it regardless of the value of the > conditional; maybe store in a temp like |rhs_prepopulated|? Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.cc:162: On 2016/11/29 03:32:18, Peter Kasting wrote: > Nit: I'd avoid this blank line, since the lambda just above it is declared > solely for the call below. Done. https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_android.h (right): https://codereview.chromium.org/2367373003/diff/300001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_android.h:91: // default search provider with it. On 2016/11/29 03:32:18, Peter Kasting wrote: > This comment is incorrect, since the function does not calculate a default > search provider index. > > There have been several cases in this CL where comments haven't been updated > correctly from one patch set to another. Please be careful to keep your > comments up to date. Reviewers now and readers later will treat your comments > as more important than the code itself. > > I don't know whether you should clarify the "sorts" bit at all. Perhaps a > comment like this: > > Updates |template_urls_| to contain everything for which > TemplateURLService::ShowInDefaultList() returns true. Sorts this list with > prepopulated engines first, then any default non-prepopulated engine, then other > non-prepopulated engines. Sorry for forgetting update the comment up to date. And the |template_urls_| actually not only holds urls for which TemplateURLService::ShowInDefaultList() returns true, but also all the other Template URLs. https://codereview.chromium.org/2367373003/diff/300001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/300001/components/search_engi... components/search_engines/template_url_service.h:136: // Retruns whether |template_url| should be shown in the list of engines On 2016/11/29 03:32:19, Peter Kasting wrote: > Nit: Typo Done. https://codereview.chromium.org/2367373003/diff/300001/components/search_engi... components/search_engines/template_url_service.h:144: // from a full list of all TemplateURLs (which might be very long). On 2016/11/29 03:32:19, Peter Kasting wrote: > These comments are still confusingly overlapping. > > What about this for the function comment and name for the new function you're > adding? With this, I think the ShowInDefaultList() comment doesn't need a > change at all. > > // Returns whether the engine is a "pre-existing" engine, either from the > // prepopulate list or created by policy. > bool IsPrepopulatedOrCreatedByPolicy(...); > Done.
LGTM https://codereview.chromium.org/2367373003/diff/320001/components/search_engi... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2367373003/diff/320001/components/search_engi... components/search_engines/template_url_service.h:143: // separately from a full list of all TemplateURLs (which might be very long). Nit: I still would revert this comment to the original one on this function, which seems clearer.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by ltian@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dfalcantara@chromium.org Link to the patchset: https://codereview.chromium.org/2367373003/#ps320001 (title: "Update based on Peter and Dan's comments.")
The CQ bit was unchecked by ltian@chromium.org
The CQ bit was checked by ltian@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 320001, "attempt_start_ts": 1480534948632550,
"parent_rev": "808a7dab6cc44bd4971e55e9a147d0f5acdaecd5", "commit_rev":
"097c6a8c900ce172ca18e75e1949b5cfaa5628e6"}
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== [Android] Allow setting recently visited search engines as default search engine 1. Change setting page layout to display recently visited search engines. 2. Change search engine layout to display url for recently visited engines. 3. Implement the logic to change the location of custom search engine based on whether it is selected as default or not. BUG=348360 ========== to ========== [Android] Allow setting recently visited search engines as default search engine 1. Change setting page layout to display recently visited search engines. 2. Change search engine layout to display url for recently visited engines. 3. Implement the logic to change the location of custom search engine based on whether it is selected as default or not. BUG=348360 Committed: https://crrev.com/3b5e1a345a490034e4588ce4e512bfa10de413f3 Cr-Commit-Position: refs/heads/master@{#435383} ==========
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/3b5e1a345a490034e4588ce4e512bfa10de413f3 Cr-Commit-Position: refs/heads/master@{#435383} |
