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

Issue 1135163002: Omnibox - Strip Extra Whitespace from Custom Search Engine Names (Closed)

Created:
5 years, 7 months ago by Mark P
Modified:
5 years, 7 months ago
Reviewers:
Peter Kasting
CC:
chromium-reviews, tim+watch_chromium.org, extensions-reviews_chromium.org, zea+watch_chromium.org, tfarina, maxbogue+watch_chromium.org, pvalenzuela+watch_chromium.org, plaree+watch_chromium.org, chromium-apps-reviews_chromium.org, maniscalco+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Omnibox - Strip Extra Whitespace from Custom Search Engine Names Whitespace in the form of tabs and line feeds and the like can screw things up. This fixes the fresh.amazon.com bug. (I tested it interatively.) By the way, there is no need to do a migration pass. The data will be correctly loaded when read from the database. (I checked.) While it's true the database will be out of date until written again, given that the actual data used is correct, I don't think that's an issue. TBR=rdevlin.cronin,isherman,lazyboy,pvalenzuela,kmadhusu,avi rdevlin.cronin - this trivial template URL data API change required some changes in chrome/browser/extensions isherman - this trivial template URL data API change required some changes in chrome/browser/importer lazyboy - this trivial template URL data API change required some changes in chrome/browser/renderer_context_menu pvalenzuela - this trivial template URL data API change required some changes in chrome/browser/sync/test/integration kmadhusu - this trivial template URL data API change required some changes in chrome/browser/ui/search/ avi - this trivial template URL data API change required some changes in chrome/browser/ui/cocoa/browser/ BUG=485357 Committed: https://crrev.com/3c6d7afbb243889bc593c69085302d460a706bdb Cr-Commit-Position: refs/heads/master@{#329742}

Patch Set 1 #

Total comments: 2

Patch Set 2 : change arg name #

Patch Set 3 : make tests compile and add unit test #

Patch Set 4 : fix compile error #

Patch Set 5 : fix more compile errors #

Patch Set 6 : fix mac compile error #

Patch Set 7 : add short name in new_tab_page_interceptor_browsertest.cc #

Patch Set 8 : fix instant unittest #

Patch Set 9 : add base::UTF8ToUTF16() call #

Patch Set 10 : fix various tests that never set short_name #

Patch Set 11 : fix more tests that don't set short_name #

Unified diffs Side-by-side diffs Delta from patch set Stats (+168 lines, -106 lines) Patch
M chrome/browser/autocomplete/autocomplete_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 10 6 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/autocomplete/history_quick_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/autocomplete/history_url_provider_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/autocomplete/search_provider_unittest.cc View 1 2 5 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/autocomplete/shortcuts_backend_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/autocomplete/zero_suggest_provider_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/chrome_content_browser_client_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/extensions/api/search_engines_private/search_engines_private_api.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/api/settings_overrides/settings_overrides_api.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/importer/in_process_importer_bridge.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_context_menu/render_view_context_menu.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/renderer_context_menu/render_view_context_menu_browsertest.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/search/instant_service_unittest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -0 lines 0 comments Download
M chrome/browser/search/instant_unittest_base.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/search/search_unittest.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/search_engines/default_search_pref_migration.cc View 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/search_engines/default_search_pref_migration_unittest.cc View 1 2 4 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/search_engines/search_provider_install_data_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/sync/test/integration/search_engines_helper.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/browser/edit_search_engine_cocoa_controller_unittest.mm View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/omnibox/omnibox_view_browsertest.cc View 1 2 3 4 4 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/ui/search/instant_search_prerenderer_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/search/instant_test_utils.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/search/new_tab_page_interceptor_browsertest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/search/search_ipc_router_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/ui/search_engines/keyword_editor_controller_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/search_engines/search_engine_tab_helper.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/search_engines/template_url_table_model.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/omnibox/autocomplete_result_unittest.cc View 1 2 2 chunks +2 lines, -2 lines 0 comments Download
M components/omnibox/keyword_provider_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M components/search_engines/default_search_manager.cc View 3 chunks +5 lines, -3 lines 0 comments Download
M components/search_engines/default_search_manager_unittest.cc View 1 2 5 chunks +6 lines, -6 lines 0 comments Download
M components/search_engines/keyword_table.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M components/search_engines/keyword_table_unittest.cc View 1 2 7 chunks +37 lines, -7 lines 0 comments Download
M components/search_engines/template_url.h View 1 chunk +1 line, -1 line 0 comments Download
M components/search_engines/template_url.cc View 2 chunks +2 lines, -2 lines 0 comments Download
M components/search_engines/template_url_data.h View 1 2 chunks +3 lines, -1 line 0 comments Download
M components/search_engines/template_url_data.cc View 2 chunks +9 lines, -0 lines 0 comments Download
M components/search_engines/template_url_fetcher_unittest.cc View 1 2 1 chunk +1 line, -1 line 0 comments Download
M components/search_engines/template_url_parser.cc View 2 chunks +4 lines, -3 lines 0 comments Download
M components/search_engines/template_url_prepopulate_data.cc View 1 chunk +1 line, -1 line 0 comments Download
M components/search_engines/template_url_prepopulate_data_unittest.cc View 1 2 5 chunks +6 lines, -6 lines 0 comments Download
M components/search_engines/template_url_service.cc View 1 2 3 4 5 6 7 8 6 chunks +7 lines, -6 lines 0 comments Download
M components/search_engines/template_url_service_sync_unittest.cc View 1 2 10 chunks +10 lines, -10 lines 0 comments Download
M components/search_engines/template_url_service_unittest.cc View 1 2 16 chunks +16 lines, -16 lines 0 comments Download
M components/search_engines/util.cc View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 26 (11 generated)
Mark P
This is a pretty trivial change to template_url_data.{h,cc}. It's an API change though so it ...
5 years, 7 months ago (2015-05-11 23:48:35 UTC) #2
Peter Kasting
LGTM https://codereview.chromium.org/1135163002/diff/1/components/search_engines/template_url_data.h File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/1135163002/diff/1/components/search_engines/template_url_data.h#newcode26 components/search_engines/template_url_data.h:26: void SetShortName(const base::string16& shortname); Nit: Call the arg ...
5 years, 7 months ago (2015-05-11 23:53:17 UTC) #3
tfarina
Could you remove "If this change looks good, I'll revise all the tests so they ...
5 years, 7 months ago (2015-05-12 00:00:37 UTC) #4
Mark P
Also revised changelist description. --mark https://codereview.chromium.org/1135163002/diff/1/components/search_engines/template_url_data.h File components/search_engines/template_url_data.h (right): https://codereview.chromium.org/1135163002/diff/1/components/search_engines/template_url_data.h#newcode26 components/search_engines/template_url_data.h:26: void SetShortName(const base::string16& shortname); ...
5 years, 7 months ago (2015-05-12 04:37:38 UTC) #5
Mark P
Added a unit test in components/omnibox/keyword_provider_unittest.cc. Made all other tests compile. Re-review if you feel ...
5 years, 7 months ago (2015-05-12 21:26:16 UTC) #6
Peter Kasting
LGTM
5 years, 7 months ago (2015-05-12 21:30:21 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135163002/40001
5 years, 7 months ago (2015-05-12 23:53:15 UTC) #9
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_gn_chromeos_rel/builds/36957)
5 years, 7 months ago (2015-05-13 00:14:04 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135163002/80001
5 years, 7 months ago (2015-05-13 00:43:07 UTC) #14
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/65457)
5 years, 7 months ago (2015-05-13 01:18:54 UTC) #16
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135163002/100001
5 years, 7 months ago (2015-05-13 03:54:25 UTC) #19
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/17298)
5 years, 7 months ago (2015-05-13 04:40:34 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1135163002/200001
5 years, 7 months ago (2015-05-13 22:47:40 UTC) #24
commit-bot: I haz the power
Committed patchset #11 (id:200001)
5 years, 7 months ago (2015-05-14 00:00:05 UTC) #25
commit-bot: I haz the power
5 years, 7 months ago (2015-05-14 00:01:03 UTC) #26
Message was sent while issue was closed.
Patchset 11 (id:??) landed as
https://crrev.com/3c6d7afbb243889bc593c69085302d460a706bdb
Cr-Commit-Position: refs/heads/master@{#329742}

Powered by Google App Engine
This is Rietveld 408576698