Added DataTypeController integration and UI surfacing for syncing Search Engines.

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

[SteveT, 2011-08-19 15:46:21]

Here's the integration patch for Search Engine Sync.

Nick - Please have a look at everything. FYI I omitted my addition of a
search_engine_data_type_controller_unittest for now... I'm working on that in a
separate patch as I have to leave soon and wanted to push these changes to
review. Also wondering if I should ommit my changes to live_sync_test.cc for
now...

Miranda - Hey could you have a look at profile_impl.cc and make sure I'm not
doing anything insane?

The Sync Service is supposed to be deleted before it's services are destroyed
(see comment), but the DestroyProfileServices call happens to destroy one of
it's services first (TemplateURLService, which it is now syncing).

Let me know if you have any questions. Thanks!


Steve

[Nicolas Zea, 2011-08-19 18:08:23]

Couple nits. 

You should probably also add jhawkins for the chrome/browser/resources changes.
I'm pretty sure chrome/browser/sync/resources/configure.html isn't used anymore.
James? Can we get rid of it?

The live_sync_test.cc change is fine. 

Mind also adding the comments to search_engine_specifics.proto? Once that's in
we can just use that one server side.

[Nicolas Zea, 2011-08-19 18:32:38]

Forgot to publish comments.

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
File chrome/browser/resources/sync_setup_overlay.html (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.html:312: <label
for="search-engines-checkbox" i18n-content="searchEngines"
80 char limit for these two lines

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
File chrome/browser/sync/glue/search_engine_data_type_controller.cc (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
chrome/browser/sync/glue/search_engine_data_type_controller.cc:48: // We want to
start the bookmark model before we begin associating.
Rewrite comment

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
chrome/browser/sync/glue/search_engine_data_type_controller.cc:57: // Add an
observer and continue when the bookmarks model is loaded.
here too

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
chrome/browser/sync/glue/search_engine_data_type_controller.cc:79:
UMA_HISTOGRAM_COUNTS("Sync.SearchEngineRunFailures", 1);
note: you're going to have to add these to histograms.xml in tools/histograms,
but that can be a separate cl

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_factory_impl.cc:349:
browser_sync::SyncableServiceAdapter* sync_service_adapter =
may as well add SyncableServiceAdapter to the "using" list at the top of the
file (and remove the namespace from the preferences call)

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
File chrome/browser/sync/resources/configure.html (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
chrome/browser/sync/resources/configure.html:365:
document.getElementById("searchEnginesCheckbox").checked =
args.syncSearchEngines;
line length

[Miranda Callahan, 2011-08-20 10:22:09]

On 2011/08/19 18:32:38, nzea wrote:
> Forgot to publish comments.
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
> File chrome/browser/resources/sync_setup_overlay.html (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
> chrome/browser/resources/sync_setup_overlay.html:312: <label
> for="search-engines-checkbox" i18n-content="searchEngines"
> 80 char limit for these two lines
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> File chrome/browser/sync/glue/search_engine_data_type_controller.cc (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> chrome/browser/sync/glue/search_engine_data_type_controller.cc:48: // We want
to
> start the bookmark model before we begin associating.
> Rewrite comment
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> chrome/browser/sync/glue/search_engine_data_type_controller.cc:57: // Add an
> observer and continue when the bookmarks model is loaded.
> here too
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> chrome/browser/sync/glue/search_engine_data_type_controller.cc:79:
> UMA_HISTOGRAM_COUNTS("Sync.SearchEngineRunFailures", 1);
> note: you're going to have to add these to histograms.xml in tools/histograms,
> but that can be a separate cl
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
> File chrome/browser/sync/profile_sync_factory_impl.cc (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
> chrome/browser/sync/profile_sync_factory_impl.cc:349:
> browser_sync::SyncableServiceAdapter* sync_service_adapter =
> may as well add SyncableServiceAdapter to the "using" list at the top of the
> file (and remove the namespace from the preferences call)
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
> File chrome/browser/sync/resources/configure.html (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
> chrome/browser/sync/resources/configure.html:365:
> document.getElementById("searchEnginesCheckbox").checked =
> args.syncSearchEngines;
> line length

Hi, Steve -- hmm, can you explain why you changed the shutdown order in the
ProfileImpl dtor?  This destruction order is pretty touchy, and I'm afraid of
breakage. Also -- what do you think about the sync integration test breakage on
the bots?

[SteveT, 2011-08-22 18:33:42]

Removed my live test changes as they were causing the integration tests to fail
for some reason. We haven't added actual SE Sync integration tests yet, so I
think it's cool to leave this out 'til we do.

Nick - Everything addressed.

Miranda - Yeah I figured it was touchy, so Nick suggested I check with you about
it :)

So in the call to
ProfileDependencyManager::GetInstance()->DestroyProfileServices(this), we
destroy all of the Profile services, including the TemplateURLServices, which
manages the search engine data.

Part of what sync_service_.reset() does is that it tries to call a shutdown
method on each of the synced services... one of which is the TemplateURLService
we already destroyed above. So this is a call to an invalid pointer, which
causes a crash. We need this call to occur while the service is still around, so
I moved the sync_service_.reset() call up before the service is destroyed.

As for the tests, I've removed a change here that seemed to make them all
unhappy. I'll make sure they're green before committing, but it doesn't look
like the ProfileImpl change was what caused it.

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
File chrome/browser/resources/sync_setup_overlay.html (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
chrome/browser/resources/sync_setup_overlay.html:312: <label
for="search-engines-checkbox" i18n-content="searchEngines"
On 2011/08/19 18:32:39, nzea wrote:
> 80 char limit for these two lines

Done.

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
File chrome/browser/sync/glue/search_engine_data_type_controller.cc (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
chrome/browser/sync/glue/search_engine_data_type_controller.cc:48: // We want to
start the bookmark model before we begin associating.
On 2011/08/19 18:32:39, nzea wrote:
> Rewrite comment

Done.

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
chrome/browser/sync/glue/search_engine_data_type_controller.cc:57: // Add an
observer and continue when the bookmarks model is loaded.
On 2011/08/19 18:32:39, nzea wrote:
> here too

Done.

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
chrome/browser/sync/glue/search_engine_data_type_controller.cc:79:
UMA_HISTOGRAM_COUNTS("Sync.SearchEngineRunFailures", 1);
Noted.

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
File chrome/browser/sync/profile_sync_factory_impl.cc (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
chrome/browser/sync/profile_sync_factory_impl.cc:349:
browser_sync::SyncableServiceAdapter* sync_service_adapter =
On 2011/08/19 18:32:39, nzea wrote:
> may as well add SyncableServiceAdapter to the "using" list at the top of the
> file (and remove the namespace from the preferences call)

Done.

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
File chrome/browser/sync/resources/configure.html (right):

http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
chrome/browser/sync/resources/configure.html:365:
document.getElementById("searchEnginesCheckbox").checked =
args.syncSearchEngines;
On 2011/08/19 18:32:39, nzea wrote:
> line length

Done.

[SteveT, 2011-08-22 19:54:14]

R+jhawkins

James - could you take a look at the JS/HTML changes? We're not sure if
configure.html is still necessary.

Thanks!

[Nicolas Zea, 2011-08-22 20:54:42]

LGTM with Miranda's and Jame's go-aheads.

[Miranda Callahan, 2011-08-23 07:17:13]

On 2011/08/22 18:33:42, SteveT wrote:
> Removed my live test changes as they were causing the integration tests to
fail
> for some reason. We haven't added actual SE Sync integration tests yet, so I
> think it's cool to leave this out 'til we do.
> 
> Nick - Everything addressed.
> 
> Miranda - Yeah I figured it was touchy, so Nick suggested I check with you
about
> it :)
> 
> So in the call to
> ProfileDependencyManager::GetInstance()->DestroyProfileServices(this), we
> destroy all of the Profile services, including the TemplateURLServices, which
> manages the search engine data.
> 
> Part of what sync_service_.reset() does is that it tries to call a shutdown
> method on each of the synced services... one of which is the
TemplateURLService
> we already destroyed above. So this is a call to an invalid pointer, which
> causes a crash. We need this call to occur while the service is still around,
so
> I moved the sync_service_.reset() call up before the service is destroyed.
> 
> As for the tests, I've removed a change here that seemed to make them all
> unhappy. I'll make sure they're green before committing, but it doesn't look
> like the ProfileImpl change was what caused it.
> 

+erg

Thanks for the explanation! That sounds entirely reasonable -- I'd like Elliot
to just glance at this, though, as he created the PDM and I know he's done a lot
of thinking about the Profile shutdown order.

>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
> File chrome/browser/resources/sync_setup_overlay.html (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/resources/syn...
> chrome/browser/resources/sync_setup_overlay.html:312: <label
> for="search-engines-checkbox" i18n-content="searchEngines"
> On 2011/08/19 18:32:39, nzea wrote:
> > 80 char limit for these two lines
> 
> Done.
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> File chrome/browser/sync/glue/search_engine_data_type_controller.cc (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> chrome/browser/sync/glue/search_engine_data_type_controller.cc:48: // We want
to
> start the bookmark model before we begin associating.
> On 2011/08/19 18:32:39, nzea wrote:
> > Rewrite comment
> 
> Done.
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> chrome/browser/sync/glue/search_engine_data_type_controller.cc:57: // Add an
> observer and continue when the bookmarks model is loaded.
> On 2011/08/19 18:32:39, nzea wrote:
> > here too
> 
> Done.
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/glue/sea...
> chrome/browser/sync/glue/search_engine_data_type_controller.cc:79:
> UMA_HISTOGRAM_COUNTS("Sync.SearchEngineRunFailures", 1);
> Noted.
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
> File chrome/browser/sync/profile_sync_factory_impl.cc (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/profile_...
> chrome/browser/sync/profile_sync_factory_impl.cc:349:
> browser_sync::SyncableServiceAdapter* sync_service_adapter =
> On 2011/08/19 18:32:39, nzea wrote:
> > may as well add SyncableServiceAdapter to the "using" list at the top of the
> > file (and remove the namespace from the preferences call)
> 
> Done.
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
> File chrome/browser/sync/resources/configure.html (right):
> 
>
http://codereview.chromium.org/7669052/diff/8001/chrome/browser/sync/resource...
> chrome/browser/sync/resources/configure.html:365:
> document.getElementById("searchEnginesCheckbox").checked =
> args.syncSearchEngines;
> On 2011/08/19 18:32:39, nzea wrote:
> > line length
> 
> Done.

[Elliot Glaysher, 2011-08-23 17:50:08]

On 2011/08/23 07:17:13, Miranda Callahan wrote:
> On 2011/08/22 18:33:42, SteveT wrote:
> > Part of what sync_service_.reset() does is that it tries to call a shutdown
> > method on each of the synced services... one of which is the
> TemplateURLService
> > we already destroyed above. So this is a call to an invalid pointer, which
> > causes a crash. We need this call to occur while the service is still
around,
> so
> > I moved the sync_service_.reset() call up before the service is destroyed.

This is hacky and needs to be fixed. For now, LGTM if you put in a TODO for
moving the sync service to ProfileKeyedService and make that
DependsOn(TemplateURLServiceFactory::GetInstance()). That will keep track of
destruction order for you.

mirandac, did you ever file that bug for the sync system to be PKSFized?

[SteveT, 2011-08-23 18:56:53]

Thanks for taking a look, Elliot. I've added the comment for now, and will chat
with the sync team about it.

James - just waiting on your review now!

[Miranda Callahan, 2011-08-23 19:20:46]

On 2011/08/23 17:50:08, Elliot Glaysher wrote:
> On 2011/08/23 07:17:13, Miranda Callahan wrote:
> > On 2011/08/22 18:33:42, SteveT wrote:
> > > Part of what sync_service_.reset() does is that it tries to call a
shutdown
> > > method on each of the synced services... one of which is the
> > TemplateURLService
> > > we already destroyed above. So this is a call to an invalid pointer, which
> > > causes a crash. We need this call to occur while the service is still
> around,
> > so
> > > I moved the sync_service_.reset() call up before the service is destroyed.
> 
> This is hacky and needs to be fixed. For now, LGTM if you put in a TODO for
> moving the sync service to ProfileKeyedService and make that
> DependsOn(TemplateURLServiceFactory::GetInstance()). That will keep track of
> destruction order for you.
> 
> mirandac, did you ever file that bug for the sync system to be PKSFized?

Filed and cc-ed you and stevet; filed it against tmcoy as Sync team leader with
a description of what's to be done and why, and you saw the email I sent to
everyone concerned.

[SteveT, 2011-08-24 12:59:36]

Thanks for shepherding that, Miranda :)

[SteveT, 2011-08-24 18:03:53]

Turns out James is away, so R+estade. Evan, can you look at the HTML/JS changes
and let me know if configure.html is still relevant? Thanks!

Also cc+scr, as he wants to look out for potential WebUI testing opportunities.

[Sheridan Rawlins, 2011-08-25 08:19:16]

Some drive by questions.

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:675: // TODO(sync): Make
ProfileSyncService into a PKS and let the PDM take care of
Shouldn't this be TODO(someone's ldap) ?

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/sync/resourc...
File chrome/browser/sync/resources/configure.html (right):

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/sync/resourc...
chrome/browser/sync/resources/configure.html:471: });
This looks a lot like the sync_setup_overlay.js

Can you share this code by including in html rather than inlining?

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:304:
l10n_util::GetStringUTF16(IDS_SYNC_DATATYPE_SEARCH_ENGINES));
Does this change need to be made to NTP4 as well?

[SteveT, 2011-08-25 14:50:55]

Sheridan - Responded to your comments.

Still waiting on Evan's review.

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/profiles/pro...
File chrome/browser/profiles/profile_impl.cc (right):

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/profiles/pro...
chrome/browser/profiles/profile_impl.cc:675: // TODO(sync): Make
ProfileSyncService into a PKS and let the PDM take care of
The issue was assigned to the Sync PM, tmccoy, for triage, at the time this TODO
was authored. It looks like it's been reassigned to me now. I'll take ownership,
but keep in mind that this is a general sync change and not search engines
specific.

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/sync/resourc...
File chrome/browser/sync/resources/configure.html (right):

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/sync/resourc...
chrome/browser/sync/resources/configure.html:471: });
zea@ thinks configure.html might actually be deprecated now. I'll wait for
estade@ to confirm before I make changes here.

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/ui/webui/ntp...
File chrome/browser/ui/webui/ntp/ntp_resource_cache.cc (right):

http://codereview.chromium.org/7669052/diff/30001/chrome/browser/ui/webui/ntp...
chrome/browser/ui/webui/ntp/ntp_resource_cache.cc:304:
l10n_util::GetStringUTF16(IDS_SYNC_DATATYPE_SEARCH_ENGINES));
Is there a separate setup for NTP4? I didn't find anything else in my search for
where IDS_SYNC_DATATYPE_SESSIONS was being used.

[Sheridan Rawlins, 2011-08-25 16:16:57]

Ok, now for my WebUI testing comment (you knew it was coming) :)

I'm not sure exactly what page you're modifying, but can you add a minimal test
which browses to the page to verify no console errors (console errors are
checked by WebUIBrowserTest).

Here's an example of adding a minimal test (for ntp4):
http://codereview.chromium.org/7741010/

[SteveT, 2011-08-25 21:02:53]

R-estade
R+arv

Replacing Evan with arv@, as per Evan's request.

arv - Could you take a look at my JS/HTML changes? We were also wondering if
configure.html is now deprecated or not.

scr - Can I owe you that test in a separate CL?

Thanks!

[Sheridan Rawlins, 2011-08-26 07:05:41]

On 2011/08/25 21:02:53, SteveT wrote:
> scr - Can I owe you that test in a separate CL?

Yes you may, as we discussed on gchat.

[arv (Not doing code reviews), 2011-08-26 17:52:55]

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.