Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(1288)

Issue 2487633003: Change behaivor to decide whether a search engine should be shown in the default list (Closed)

Created:
4 years, 1 month ago by ltian
Modified:
4 years, 1 month ago
CC:
chromium-reviews, dbeam+watch-options_chromium.org, extensions-reviews_chromium.org, michaelpg+watch-options_chromium.org, rouslan+autofill_chromium.org, tfarina, sebsg+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, estade+watch_chromium.org, vabr+watchlistautofill_chromium.org, chromium-apps-reviews_chromium.org, sync-reviews_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change behaivor to decide whether a search engine should be shown in the default list 1. Delete show_in_default_list field in template_url_data. 2. New rule checks whether a search engine is prepopulated or created by policy or now selected as default to decide whether it should appears in the default search setting list. The search engine should also support search term replacement if it is in that list. 3. Search engines parsed from Firefox by default will not appears in the default list. BUG=348360 Committed: https://crrev.com/cf06dd01d2516e1509acd1e99b20ccff09b8c19a Cr-Commit-Position: refs/heads/master@{#433151}

Patch Set 1 #

Total comments: 37

Patch Set 2 : Update based on Ian and Peter's comments. #

Total comments: 16

Patch Set 3 : Update based on Peter and Nicolas's comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+283 lines, -351 lines) Patch
M chrome/browser/android/locale/special_locale_handler.cc View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc View 1 chunk +0 lines, -1 line 0 comments Download
M chrome/browser/importer/in_process_importer_bridge.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/importer/profile_writer.cc View 1 chunk +4 lines, -4 lines 0 comments Download
M chrome/browser/search_engines/template_url_parser_unittest.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/search_engines/template_url_service_sync_unittest.cc View 4 chunks +4 lines, -6 lines 0 comments Download
M chrome/browser/search_engines/template_url_service_unittest.cc View 1 2 22 chunks +28 lines, -25 lines 0 comments Download
M chrome/browser/ui/search_engines/template_url_table_model.cc View 1 1 chunk +5 lines, -8 lines 0 comments Download
M chrome/browser/ui/webui/options/browser_options_handler.cc View 1 1 chunk +4 lines, -4 lines 0 comments Download
M components/search_engines/default_search_manager.cc View 1 chunk +0 lines, -1 line 0 comments Download
M components/search_engines/default_search_manager_unittest.cc View 3 chunks +0 lines, -3 lines 0 comments Download
M components/search_engines/default_search_pref_migration.cc View 1 chunk +0 lines, -1 line 0 comments Download
M components/search_engines/keyword_table.h View 2 chunks +1 line, -1 line 0 comments Download
M components/search_engines/keyword_table.cc View 1 2 9 chunks +81 lines, -36 lines 0 comments Download
M components/search_engines/keyword_table_unittest.cc View 3 chunks +0 lines, -5 lines 0 comments Download
M components/search_engines/template_url.h View 1 chunk +0 lines, -5 lines 0 comments Download
M components/search_engines/template_url.cc View 2 chunks +0 lines, -7 lines 0 comments Download
M components/search_engines/template_url_data.h View 1 chunk +0 lines, -6 lines 0 comments Download
M components/search_engines/template_url_data.cc View 2 chunks +2 lines, -4 lines 0 comments Download
M components/search_engines/template_url_fetcher.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/search_engines/template_url_parser.h View 1 chunk +0 lines, -1 line 0 comments Download
M components/search_engines/template_url_parser.cc View 5 chunks +3 lines, -9 lines 0 comments Download
M components/search_engines/template_url_prepopulate_data.cc View 1 chunk +0 lines, -1 line 0 comments Download
M components/search_engines/template_url_service.h View 1 2 1 chunk +7 lines, -0 lines 0 comments Download
M components/search_engines/template_url_service.cc View 1 2 6 chunks +7 lines, -7 lines 0 comments Download
M components/search_engines/template_url_unittest.cc View 1 chunk +0 lines, -1 line 0 comments Download
M components/sync/protocol/proto_visitors.h View 1 2 35 chunks +91 lines, -202 lines 0 comments Download
M components/sync/protocol/search_engine_specifics.proto View 1 2 1 chunk +3 lines, -2 lines 0 comments Download
A + components/test/data/web_database/version_67.sql View 2 chunks +2 lines, -2 lines 0 comments Download
M components/webdata/common/BUILD.gn View 1 chunk +1 line, -0 lines 0 comments Download
M components/webdata/common/web_database.cc View 1 1 chunk +2 lines, -2 lines 0 comments Download
M components/webdata/common/web_database_migration_unittest.cc View 2 chunks +33 lines, -1 line 0 comments Download

Dependent Patchsets:

Messages

Total messages: 38 (21 generated)
ltian
pkasting@chromium.org: please review the changes related to search engines and web data. Thanks!
4 years, 1 month ago (2016-11-09 19:29:25 UTC) #6
ltian
4 years, 1 month ago (2016-11-09 19:30:27 UTC) #8
Ian Wen
As far as I can tell it's in good shape. Here are some nits that ...
4 years, 1 month ago (2016-11-09 19:58:53 UTC) #9
Peter Kasting
https://codereview.chromium.org/2487633003/diff/1/chrome/browser/search_engines/template_url_service_unittest.cc File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2487633003/diff/1/chrome/browser/search_engines/template_url_service_unittest.cc#newcode867 chrome/browser/search_engines/template_url_service_unittest.cc:867: data.prepopulate_id = 999999; Nit: Define a constant for this ...
4 years, 1 month ago (2016-11-10 06:41:07 UTC) #10
ltian
https://codereview.chromium.org/2487633003/diff/1/chrome/browser/search_engines/template_url_service_unittest.cc File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2487633003/diff/1/chrome/browser/search_engines/template_url_service_unittest.cc#newcode867 chrome/browser/search_engines/template_url_service_unittest.cc:867: data.prepopulate_id = 999999; On 2016/11/10 06:41:06, Peter Kasting wrote: ...
4 years, 1 month ago (2016-11-11 03:52:14 UTC) #17
ltian
zea@chromium.org: Please review changes in components/sync. Thanks!
4 years, 1 month ago (2016-11-11 05:59:34 UTC) #19
Peter Kasting
https://codereview.chromium.org/2487633003/diff/1/components/search_engines/keyword_table.cc File components/search_engines/keyword_table.cc (right): https://codereview.chromium.org/2487633003/diff/1/components/search_engines/keyword_table.cc#newcode344 components/search_engines/keyword_table.cc:344: return true; On 2016/11/11 03:52:13, ltian wrote: > On ...
4 years, 1 month ago (2016-11-11 06:13:08 UTC) #20
Peter Kasting
LGTM https://codereview.chromium.org/2487633003/diff/20001/chrome/browser/search_engines/template_url_service_unittest.cc File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2487633003/diff/20001/chrome/browser/search_engines/template_url_service_unittest.cc#newcode142 chrome/browser/search_engines/template_url_service_unittest.cc:142: const int kPrepopulatedId = 999999; Nit: constexpr https://codereview.chromium.org/2487633003/diff/20001/components/search_engines/keyword_table.cc ...
4 years, 1 month ago (2016-11-14 19:59:16 UTC) #21
Nicolas Zea
LGTM with a small style change. https://codereview.chromium.org/2487633003/diff/20001/components/sync/protocol/search_engine_specifics.proto File components/sync/protocol/search_engine_specifics.proto (left): https://codereview.chromium.org/2487633003/diff/20001/components/sync/protocol/search_engine_specifics.proto#oldcode37 components/sync/protocol/search_engine_specifics.proto:37: optional bool show_in_default_list ...
4 years, 1 month ago (2016-11-14 20:48:10 UTC) #22
ltian
https://codereview.chromium.org/2487633003/diff/20001/chrome/browser/search_engines/template_url_service_unittest.cc File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2487633003/diff/20001/chrome/browser/search_engines/template_url_service_unittest.cc#newcode142 chrome/browser/search_engines/template_url_service_unittest.cc:142: const int kPrepopulatedId = 999999; On 2016/11/14 19:59:16, Peter ...
4 years, 1 month ago (2016-11-15 19:43:42 UTC) #27
ltian
dbeam@chromium.org: Please review changes in chrome/browser/ui/webui. sky@chromium.org: Please review changes in chrome/browser/extensions and chrome/browser/importer. Thanks!
4 years, 1 month ago (2016-11-15 19:49:49 UTC) #29
Peter Kasting
https://codereview.chromium.org/2487633003/diff/20001/components/sync/protocol/search_engine_specifics.proto File components/sync/protocol/search_engine_specifics.proto (left): https://codereview.chromium.org/2487633003/diff/20001/components/sync/protocol/search_engine_specifics.proto#oldcode37 components/sync/protocol/search_engine_specifics.proto:37: optional bool show_in_default_list = 9; On 2016/11/15 19:43:41, ltian ...
4 years, 1 month ago (2016-11-15 19:53:14 UTC) #30
sky
LGTM
4 years, 1 month ago (2016-11-15 20:49:58 UTC) #31
Dan Beam
webui lgtm
4 years, 1 month ago (2016-11-15 22:14:38 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2487633003/40001
4 years, 1 month ago (2016-11-18 06:25:34 UTC) #35
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 1 month ago (2016-11-18 08:49:43 UTC) #36
commit-bot: I haz the power
4 years, 1 month ago (2016-11-18 08:51:21 UTC) #38
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/cf06dd01d2516e1509acd1e99b20ccff09b8c19a
Cr-Commit-Position: refs/heads/master@{#433151}

Powered by Google App Engine
This is Rietveld 408576698