Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(27)

Issue 8334030: Merge search engines sync data type with Preferences. Sync the default search provider. Add some ... (Closed)

Created:
7 years, 4 months ago by SteveT
Modified:
7 years, 4 months ago
CC:
chromium-reviews, ncarter (slow), akalin, Raghu Simha, arv (Not doing code reviews), PaweĊ‚ Hajdan Jr., estade+watch_chromium.org, tim (not reviewing)
Visibility:
Public.

Description

Merge search engines sync data type with Preferences. Sync the default search provider. Add some defensive measures to prevent deletion of the default search engine or unnecessarily uniquifying keywords. TEST=Ensure that the default search provider syncs when the Preferences sync data type is enabled. Ensure that the normal search engine syncing changes (add, update, delete) all work. BUG=15548 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109882

Patch Set 1 : '' #

Total comments: 33

Patch Set 2 : '' #

Total comments: 6

Patch Set 3 : '' #

Total comments: 18

Patch Set 4 : '' #

Total comments: 4

Patch Set 5 : Merge to TOT. Changes to setup UI from rsimha's comments. #

Patch Set 6 : Merge to TOT and fixed additional conflicts from rsimha. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+541 lines, -130 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 1 chunk +0 lines, -7 lines 0 comments Download
M chrome/browser/prefs/pref_set_observer.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/resources/sync_setup_overlay.html View 1 2 3 4 5 1 chunk +0 lines, -8 lines 0 comments Download
M chrome/browser/resources/sync_setup_overlay.js View 1 2 3 4 5 2 chunks +0 lines, -7 lines 0 comments Download
M chrome/browser/search_engines/template_url_service.h View 1 2 3 4 5 3 chunks +15 lines, -2 lines 0 comments Download
M chrome/browser/search_engines/template_url_service.cc View 1 2 3 4 5 14 chunks +120 lines, -14 lines 0 comments Download
M chrome/browser/search_engines/template_url_service_factory.cc View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/search_engines/template_url_service_sync_unittest.cc View 1 2 3 4 5 5 chunks +215 lines, -1 line 0 comments Download
M chrome/browser/sync/profile_sync_factory_impl.cc View 1 2 3 4 5 2 chunks +7 lines, -7 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory_impl_unittest.cc View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/browser/sync/resources/configure.html View 1 2 3 4 5 3 chunks +11 lines, -26 lines 0 comments Download
M chrome/browser/sync/sync_prefs.cc View 1 2 3 4 5 3 chunks +21 lines, -4 lines 0 comments Download
M chrome/browser/sync/sync_prefs_unittest.cc View 1 2 3 4 5 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/sync/sync_setup_flow.cc View 1 2 3 4 5 2 chunks +0 lines, -4 lines 0 comments Download
M chrome/browser/sync/sync_setup_wizard_unittest.cc View 1 2 3 4 5 2 chunks +2 lines, -3 lines 0 comments Download
M chrome/browser/sync/test/integration/search_engines_helper.h View 1 2 3 4 5 2 chunks +11 lines, -1 line 0 comments Download
M chrome/browser/sync/test/integration/search_engines_helper.cc View 1 2 3 4 5 3 chunks +43 lines, -4 lines 0 comments Download
M chrome/browser/sync/test/integration/sync_test.h View 1 2 3 4 5 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/sync/test/integration/sync_test.cc View 1 2 3 4 5 2 chunks +9 lines, -4 lines 0 comments Download
M chrome/browser/sync/test/integration/two_client_search_engines_sync_test.cc View 1 2 3 4 5 3 chunks +43 lines, -2 lines 0 comments Download
M chrome/browser/sync/test/integration/two_client_sessions_sync_test.cc View 1 2 3 4 5 6 chunks +19 lines, -14 lines 0 comments Download
M chrome/browser/ui/webui/ntp/ntp_resource_cache.cc View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/options/personal_options_handler.cc View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/sync_setup_handler.cc View 1 2 3 4 5 4 chunks +0 lines, -11 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 2 chunks +1 line, -1 line 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 1 chunk +8 lines, -0 lines 0 comments Download

Messages

Total messages: 14 (0 generated)
SteveT
Nick: PTAL at everything except perhaps the SearchEngine integration tests. Raghu: PTAL at the integration ...
7 years, 4 months ago (2011-11-04 21:23:23 UTC) #1
SteveT
+jeanluc
7 years, 4 months ago (2011-11-04 21:26:52 UTC) #2
Nicolas Zea
http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engines/template_url_service.cc File chrome/browser/search_engines/template_url_service.cc (left): http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engines/template_url_service.cc#oldcode531 chrome/browser/search_engines/template_url_service.cc:531: #if defined(OS_WIN) Is this supposed to be part of ...
7 years, 4 months ago (2011-11-04 23:08:06 UTC) #3
SteveT
Thanks for the initial look. I've addressed *most* of your concerns. PTAL and let me ...
7 years, 4 months ago (2011-11-07 21:48:55 UTC) #4
Raghu Simha
Thanks for making these changes. Integration test changes LGTM pending green bots. http://codereview.chromium.org/8334030/diff/20003/chrome/browser/sync/test/integration/search_engines_helper.cc File chrome/browser/sync/test/integration/search_engines_helper.cc ...
7 years, 4 months ago (2011-11-07 22:18:12 UTC) #5
SteveT
Thanks for the look, Raghu. Made changes as you suggested and made some adjustments to ...
7 years, 4 months ago (2011-11-08 00:17:27 UTC) #6
jeanluc1
http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engines/template_url_service.cc File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/8334030/diff/21007/chrome/browser/search_engines/template_url_service.cc#newcode666 chrome/browser/search_engines/template_url_service.cc:666: prefs->GetString(prefs::kSyncedDefaultSearchProviderGUID)); Can GetString return NULL? What's the behavior of ...
7 years, 4 months ago (2011-11-08 19:49:41 UTC) #7
Nicolas Zea
http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engines/template_url_service.cc File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/8334030/diff/15003/chrome/browser/search_engines/template_url_service.cc#newcode585 chrome/browser/search_engines/template_url_service.cc:585: } On 2011/11/07 21:48:56, SteveT wrote: > I believe ...
7 years, 4 months ago (2011-11-08 19:57:26 UTC) #8
SteveT
Nick/Jean-Luc. I've responded to all your comments - back to you guys for another look. ...
7 years, 4 months ago (2011-11-08 21:40:03 UTC) #9
Nicolas Zea
LGTM, thanks!
7 years, 4 months ago (2011-11-08 21:47:34 UTC) #10
Raghu Simha
Looks like you'll need to make two more changes to code that was landed yesterday ...
7 years, 4 months ago (2011-11-09 00:22:53 UTC) #11
SteveT
Cool, thanks for pointing this out, Raghu! I've re-run associated tests and things look good ...
7 years, 4 months ago (2011-11-09 16:33:18 UTC) #12
SteveT
Ping - Hey Jean-Luc, could you take another look? Thanks.
7 years, 4 months ago (2011-11-10 14:45:52 UTC) #13
jeanluc1
7 years, 4 months ago (2011-11-12 22:58:33 UTC) #14
lgtm

Powered by Google App Engine
This is Rietveld 408576698