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

Issue 7669052: Added DataTypeController integration and UI surfacing for syncing Search Engines. (Closed)

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

Description

Added DataTypeController integration and UI surfacing for syncing Search Engines. Removed some sync fields from search engine proto. TEST=Ensure that you can enable Search Engine Sync through the about:flags page and sync setup UI. With both enabled search engines should start syncing. BUG=15548 Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=99114

Patch Set 1 : Initial upload. #

Total comments: 12

Patch Set 2 : Addressed comments. Removed live test changes. #

Patch Set 3 : Added comments to search engine data proto file. #

Patch Set 4 : TODO comment in ProfileImpl #

Total comments: 6

Patch Set 5 : Changed TODO owner #

Patch Set 6 : Unsynced id and logo_id #

Total comments: 1

Patch Set 7 : Merge to tip of tree #

Patch Set 8 : Fixed notification registration. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+317 lines, -32 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 2 chunks +9 lines, -0 lines 0 comments Download
M chrome/browser/about_flags.cc View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/profiles/profile_impl.cc View 1 2 3 4 5 6 2 chunks +5 lines, -3 lines 0 comments Download
M chrome/browser/resources/sync_setup_overlay.html View 1 2 3 4 5 6 1 chunk +8 lines, -0 lines 0 comments Download
M chrome/browser/resources/sync_setup_overlay.js View 1 2 3 4 5 6 2 chunks +9 lines, -2 lines 0 comments Download
M chrome/browser/search_engines/template_url_service.cc View 1 2 3 4 5 6 3 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/search_engines/template_url_service_sync_unittest.cc View 1 2 3 4 5 6 1 chunk +0 lines, -1 line 0 comments Download
A chrome/browser/sync/glue/search_engine_data_type_controller.h View 1 chunk +53 lines, -0 lines 0 comments Download
A chrome/browser/sync/glue/search_engine_data_type_controller.cc View 1 2 3 4 5 6 7 1 chunk +93 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory.h View 1 2 3 4 5 6 1 chunk +7 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory_impl.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory_impl.cc View 1 2 3 4 5 6 6 chunks +33 lines, -4 lines 0 comments Download
M chrome/browser/sync/profile_sync_factory_mock.h View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/sync/profile_sync_service.cc View 1 2 3 4 5 6 2 chunks +5 lines, -0 lines 0 comments Download
M chrome/browser/sync/protocol/proto_value_conversions.cc View 1 2 3 4 5 6 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/browser/sync/protocol/search_engine_specifics.proto View 1 2 3 4 5 6 1 chunk +23 lines, -2 lines 0 comments Download
M chrome/browser/sync/resources/configure.html View 1 2 3 4 5 6 3 chunks +26 lines, -11 lines 0 comments Download
M chrome/browser/sync/sync_setup_flow.cc View 1 2 3 4 5 6 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/sync/sync_setup_wizard_unittest.cc View 1 2 3 4 5 6 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/webui/ntp/ntp_resource_cache.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/options/personal_options_handler.cc View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/browser/ui/webui/sync_setup_handler.cc View 1 2 3 4 5 6 2 chunks +7 lines, -0 lines 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M chrome/common/chrome_switches.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M chrome/common/chrome_switches.cc View 1 2 3 4 5 6 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/common/pref_names.h View 1 2 3 4 5 6 2 chunks +2 lines, -1 line 0 comments Download
M chrome/common/pref_names.cc View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
SteveT
Here's the integration patch for Search Engine Sync. Nick - Please have a look at ...
7 years, 7 months ago (2011-08-19 15:46:21 UTC) #1
Nicolas Zea
Couple nits. You should probably also add jhawkins for the chrome/browser/resources changes. I'm pretty sure ...
7 years, 7 months ago (2011-08-19 18:08:23 UTC) #2
Nicolas Zea
Forgot to publish comments. http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/sync_setup_overlay.html File chrome/browser/resources/sync_setup_overlay.html (right): http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/sync_setup_overlay.html#newcode312 chrome/browser/resources/sync_setup_overlay.html:312: <label for="search-engines-checkbox" i18n-content="searchEngines" 80 char ...
7 years, 7 months ago (2011-08-19 18:32:38 UTC) #3
Miranda Callahan
On 2011/08/19 18:32:38, nzea wrote: > Forgot to publish comments. > > http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/sync_setup_overlay.html > File ...
7 years, 7 months ago (2011-08-20 10:22:09 UTC) #4
SteveT
Removed my live test changes as they were causing the integration tests to fail for ...
7 years, 7 months ago (2011-08-22 18:33:42 UTC) #5
SteveT
R+jhawkins James - could you take a look at the JS/HTML changes? We're not sure ...
7 years, 7 months ago (2011-08-22 19:54:14 UTC) #6
Nicolas Zea
LGTM with Miranda's and Jame's go-aheads.
7 years, 7 months ago (2011-08-22 20:54:42 UTC) #7
Miranda Callahan
On 2011/08/22 18:33:42, SteveT wrote: > Removed my live test changes as they were causing ...
7 years, 6 months ago (2011-08-23 07:17:13 UTC) #8
Elliot Glaysher
On 2011/08/23 07:17:13, Miranda Callahan wrote: > On 2011/08/22 18:33:42, SteveT wrote: > > Part ...
7 years, 6 months ago (2011-08-23 17:50:08 UTC) #9
SteveT
Thanks for taking a look, Elliot. I've added the comment for now, and will chat ...
7 years, 6 months ago (2011-08-23 18:56:53 UTC) #10
Miranda Callahan
On 2011/08/23 17:50:08, Elliot Glaysher wrote: > On 2011/08/23 07:17:13, Miranda Callahan wrote: > > ...
7 years, 6 months ago (2011-08-23 19:20:46 UTC) #11
SteveT
Thanks for shepherding that, Miranda :)
7 years, 6 months ago (2011-08-24 12:59:36 UTC) #12
SteveT
Turns out James is away, so R+estade. Evan, can you look at the HTML/JS changes ...
7 years, 6 months ago (2011-08-24 18:03:53 UTC) #13
Sheridan Rawlins
Some drive by questions. http://codereview.chromium.org/7669052/diff/30001/chrome/browser/profiles/profile_impl.cc File chrome/browser/profiles/profile_impl.cc (right): http://codereview.chromium.org/7669052/diff/30001/chrome/browser/profiles/profile_impl.cc#newcode675 chrome/browser/profiles/profile_impl.cc:675: // TODO(sync): Make ProfileSyncService into ...
7 years, 6 months ago (2011-08-25 08:19:16 UTC) #14
SteveT
Sheridan - Responded to your comments. Still waiting on Evan's review. http://codereview.chromium.org/7669052/diff/30001/chrome/browser/profiles/profile_impl.cc File chrome/browser/profiles/profile_impl.cc (right): ...
7 years, 6 months ago (2011-08-25 14:50:55 UTC) #15
Sheridan Rawlins
Ok, now for my WebUI testing comment (you knew it was coming) :) I'm not ...
7 years, 6 months ago (2011-08-25 16:16:57 UTC) #16
SteveT
R-estade R+arv Replacing Evan with arv@, as per Evan's request. arv - Could you take ...
7 years, 6 months ago (2011-08-25 21:02:53 UTC) #17
Sheridan Rawlins
On 2011/08/25 21:02:53, SteveT wrote: > scr - Can I owe you that test in ...
7 years, 6 months ago (2011-08-26 07:05:41 UTC) #18
arv (Not doing code reviews)
7 years, 6 months ago (2011-08-26 17:52:55 UTC) #19
HTML/JS LGTM with style issues

http://codereview.chromium.org/7669052/diff/42012/chrome/browser/resources/sy...
File chrome/browser/resources/sync_setup_overlay.js (right):

http://codereview.chromium.org/7669052/diff/42012/chrome/browser/resources/sy...
chrome/browser/resources/sync_setup_overlay.js:355:
$('search-engines-item').className = "sync-item-show";
sigh... these should be single quotes but it seems this whole file is
inconsistent.

Powered by Google App Engine
This is Rietveld 408576698