|
|
Created:
9 years, 4 months ago by SteveT Modified:
9 years, 4 months ago CC:
chromium-reviews, Paweł Hajdan Jr., tim (not reviewing), Raghu Simha, ncarter (slow), idana Base URL:
svn://svn.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionRecommitting what was r96969. Fixed the memory leak issues.
Implement SyncableServices in TemplateURLService. Add related unittests.
TEST=Ensure all TemplateURLService related unit tests pass. Ensure the HeapBots are happy!
BUG=15548
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=97187
Patch Set 1 : Initial upload #
Total comments: 19
Patch Set 2 : '' #
Total comments: 4
Patch Set 3 : Shifted helpers to private after Peter's initial comments #Patch Set 4 : Addressed Peter's final comment nits. #
Total comments: 45
Patch Set 5 : Addressed zea's TemplateURLService issues. #Patch Set 6 : Addressed zea's unittest issues. #
Total comments: 8
Patch Set 7 : Addressed zea's additional issues. #Patch Set 8 : Recommitting patch. Repaired memory leaks and adjusted unittests to reflect changes. #
Total comments: 10
Messages
Total messages: 19 (0 generated)
Hey guys, Here's my initial patch for syncing TemplateURLService (aka Syncing Custom Search Engines). Please note that this just includes the SyncableService implementation for TemplateURLService + unittests, but does NOT include the DataController integration that actually allows the service to start syncing. That will come in a later patch + UI changes to expose the feature + full integration tests. A note on this: When we originally added sync_guid as a field to TemplateURL, we were planning on only setting it when it gets synced. It turns out that we actually need to assign a GUID to all TemplateURLs - including unsynced ones - otherwise we have a hard time identifying the set of synced vs unsynced TemplateURLs the first time we sync. Nick - Please have a look at... everything. Peter - Please have a look at changes to TemplateURL/TemplateURLService, sans the SyncableService implementation bits. Thanks for taking over for Scott! If you have additional question, please let me know. Thanks, all! Steve
Hm, just noticed that the helpers in TemplateURLService can probably be all made private and friended with the unit test class. Will do that along the first round of review changes.
Make sure the following cases both work correctly: * Any URL that is managed by the "prepopulate" code continues to be so managed (on all synced machines) after syncing occurs * We don't create multiple copies of keywords when syncing two accounts that each separately have the same keyword to begin with (by "same" I mean "we consider the underlying URLs to be equivalent using the equivalence comparison used when deciding whether to update a TURL based on new content from a web page" -- in other words, something broader than a strict URL check, but narrower than a simple keyword check) http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:45: using syncable::SEARCH_ENGINES; Nit: We normally try to avoid using declarations (I'd personally kill base::Time here as well), would putting the extra qualifier in the code below really make things much worse? http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:412: ResetTemplateURL(url, title, keyword, search_url, ""); Nit: Use std::string() instead of "" http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:636: const { Nit: I think our pattern has been to wrap after "(" when "const" doesn't fit on the same line as the rest of the declaration. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:984: existing_turl, Nit: No need for one-arg-per-line or putting even the first arg on a new line like we do for function declarations; you're free to collapse args into fewer lines when making calls. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:42: typedef std::map<std::string, SyncData> SyncDataMap; Nit: Should this typedef be inside the class? Seems kinda bad to make it global. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:193: void ResetTemplateURL(const TemplateURL* url, By adding this, we've effectively created a "default argument" for the last arg. This is banned by the style guide. Can we remove the version above and have everyone only call this one? Should we keep these both but name them differently? Should we have a separate function to reset the GUID? http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:299: // |turl|. Returns true if successful, false otherwise. Nit: You can probably just say "Returns whether creation succeeded", but you might also want to note what can cause failure if that wouldn't be obvious to all callers. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:306: static TemplateURL* ReadSearchEngineSyncData(const SyncData& sync_data); Nit: These two functions are paired, but one returns a bool and takes an outparam, while the other returns a (heap-allocated) pointer directly. Also (less importantly), their names aren't strict mirrors, the way they would be with e.g. "CreateSyncDataFromTURL()" and "CreateTURLFromSyncData()" or something. Should these be made more parallel in structure? http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:560: // Set when start syncing, reset in StopSyncing. While this is not set, we Nit: "Set when start syncing" isn't grammatical (maybe "Set in MergeDataAndStartSyncing()"?)
I believe I've addressed everything inline. I will upload a separate patch where I make the helpers private.. so you can see the changes made before everything moves. On 2011/08/09 21:31:04, Peter Kasting wrote: > Make sure the following cases both work correctly: > > * Any URL that is managed by the "prepopulate" code continues to be so managed > (on all synced machines) after syncing occurs Since the Sync impl never touches the prepopulate IDs, this should be OK. However, I will note that when we write integration tests, we need to have a test case to ensure that prepopulated URLs can be synced and merged without any issues. > > * We don't create multiple copies of keywords when syncing two accounts that > each separately have the same keyword to begin with (by "same" I mean "we > consider the underlying URLs to be equivalent using the equivalence comparison > used when deciding whether to update a TURL based on new content from a web > page" -- in other words, something broader than a strict URL check, but narrower > than a simple keyword check) We already go through the trouble of ensuring that we never duplicate keywords on the same local machine (by generating a unique one if needed using ResolveSyncKeywordConflict). We also have some code (MergeSyncAndLocalURLDuplicates) to merge cases where we think we have duplicates of the same keyword from the same site (so keyword+URL), but perhaps the equivalence check is too strict. So I believe you're referring to the code in TemplateURLFetcher (I think) that rejects new TURLs that have already been added from a site. I'll see what they check in addition to the OSD URLs when trying to determine that and see if we need to modify how we handle dupes. Do correct me if I misunderstood something here. It's important that we get this de-duping right since it can cause some nasty inconsistencies or corrupted TURLServices if we're not careful. > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > File chrome/browser/search_engines/template_url_service.cc (right): > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.cc:45: using > syncable::SEARCH_ENGINES; > Nit: We normally try to avoid using declarations (I'd personally kill base::Time > here as well), would putting the extra qualifier in the code below really make > things much worse? > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.cc:412: ResetTemplateURL(url, > title, keyword, search_url, ""); > Nit: Use std::string() instead of "" > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.cc:636: const { > Nit: I think our pattern has been to wrap after "(" when "const" doesn't fit on > the same line as the rest of the declaration. > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.cc:984: existing_turl, > Nit: No need for one-arg-per-line or putting even the first arg on a new line > like we do for function declarations; you're free to collapse args into fewer > lines when making calls. > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > File chrome/browser/search_engines/template_url_service.h (right): > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.h:42: typedef > std::map<std::string, SyncData> SyncDataMap; > Nit: Should this typedef be inside the class? Seems kinda bad to make it > global. > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.h:193: void > ResetTemplateURL(const TemplateURL* url, > By adding this, we've effectively created a "default argument" for the last arg. > This is banned by the style guide. Can we remove the version above and have > everyone only call this one? Should we keep these both but name them > differently? Should we have a separate function to reset the GUID? > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.h:299: // |turl|. Returns > true if successful, false otherwise. > Nit: You can probably just say "Returns whether creation succeeded", but you > might also want to note what can cause failure if that wouldn't be obvious to > all callers. > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.h:306: static TemplateURL* > ReadSearchEngineSyncData(const SyncData& sync_data); > Nit: These two functions are paired, but one returns a bool and takes an > outparam, while the other returns a (heap-allocated) pointer directly. Also > (less importantly), their names aren't strict mirrors, the way they would be > with e.g. "CreateSyncDataFromTURL()" and "CreateTURLFromSyncData()" or > something. Should these be made more parallel in structure? > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.h:560: // Set when start > syncing, reset in StopSyncing. While this is not set, we > Nit: "Set when start syncing" isn't grammatical (maybe "Set in > MergeDataAndStartSyncing()"?)
Oops, forgot the comments. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:45: using syncable::SEARCH_ENGINES; Agreed. Cleaned up both using declarations. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:412: ResetTemplateURL(url, title, keyword, search_url, ""); On 2011/08/09 21:31:04, Peter Kasting wrote: > Nit: Use std::string() instead of "" Done. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:636: const { On 2011/08/09 21:31:04, Peter Kasting wrote: > Nit: I think our pattern has been to wrap after "(" when "const" doesn't fit on > the same line as the rest of the declaration. Done. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:984: existing_turl, Cool. Hope this works for you. It was mostly the last big argument that forced the line breaks. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:42: typedef std::map<std::string, SyncData> SyncDataMap; On 2011/08/09 21:31:04, Peter Kasting wrote: > Nit: Should this typedef be inside the class? Seems kinda bad to make it > global. Done. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:193: void ResetTemplateURL(const TemplateURL* url, You're right - the correct thing to do here is to introduced a separate method that resets just the GUID and persists the change to the DB (rather than add more crap to this method). Done. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:299: // |turl|. Returns true if successful, false otherwise. Changed the wording. Actually this currently always returns true because we're not doing anything complicated enough to fail. I'm mostly taking the format from Pref's CreatePrefSyncData method, which can possibly fail. I can just make this void for now if that makes more sense. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:306: static TemplateURL* ReadSearchEngineSyncData(const SyncData& sync_data); Actually, since our version of CreateSyncData (above) can't really fail, I'll have it return a SyncData so the structure is a bit more similar. I'll also change the wording. I think copying Pref's structure around these two helpers wasn't the best idea. http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:560: // Set when start syncing, reset in StopSyncing. While this is not set, we On 2011/08/09 21:31:04, Peter Kasting wrote: > Nit: "Set when start syncing" isn't grammatical (maybe "Set in > MergeDataAndStartSyncing()"?) Done.
On 2011/08/10 15:14:48, SteveT wrote: > On 2011/08/09 21:31:04, Peter Kasting wrote: > > Make sure the following cases both work correctly: > > > > * Any URL that is managed by the "prepopulate" code continues to be so managed > > (on all synced machines) after syncing occurs > > Since the Sync impl never touches the prepopulate IDs, this should be OK. I think the prepopulate code also checks whether the URLs have been edited, or something, when determining whether it's safe to auto-update. I don't recall details, but I'm worried that just not changing these IDs might not be sufficient to prevent problems. > So I believe you're referring to the code in TemplateURLFetcher (I think) that > rejects new TURLs that have already been added from a site. I'll see what they > check in addition to the OSD URLs when trying to determine that and see if we > need to modify how we handle dupes. The picture I have in my head is that there are cases where a keyword's URL can be changed if the code thinks it's safe to replace the old URL with the new one. I think this is used by the prepopulate stuff I'm talking about above, too? Like, maybe if the old URL hasn't been user-modified, we'll happily replace it, or something. I just want to make sure that if there is indeed such code, and one computer updates a keyword using it while another sits idle, then when we sync the two together we get one keyword with the new URL as a result. I'm sorry I'm being so vague. Scott's always been the one to work on this code so I'm not terribly familiar with it :( http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:299: // |turl|. Returns true if successful, false otherwise. On 2011/08/10 15:15:12, SteveT wrote: > Changed the wording. Actually this currently always returns true because we're > not doing anything complicated enough to fail. I'm mostly taking the format from > Pref's CreatePrefSyncData method, which can possibly fail. I can just make this > void for now if that makes more sense. Yeah, I'd do that; it's easy to change the signature later as needed.
Other than the broad vague comments I've just made, the changes LGTM http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:184: // TemplateURL. The TemplateURL is marked as not replaceable. If |guid| is Nit: This comment is out of date. http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:191: // Resets the sync GUID of the specified TemplateURL and persist the change to Nit: persist -> persists
On 2011/08/10 17:39:11, Peter Kasting wrote: > On 2011/08/10 15:14:48, SteveT wrote: > > On 2011/08/09 21:31:04, Peter Kasting wrote: > > > Make sure the following cases both work correctly: > > > > > > * Any URL that is managed by the "prepopulate" code continues to be so > managed > > > (on all synced machines) after syncing occurs > > > > Since the Sync impl never touches the prepopulate IDs, this should be OK. > > I think the prepopulate code also checks whether the URLs have been edited, or > something, when determining whether it's safe to auto-update. I don't recall > details, but I'm worried that just not changing these IDs might not be > sufficient to prevent problems. I see. Okay. I will have another look through all the prepopulate TURL management code to see if I missed anything. If I can write any unit tests to check this, I'll do that. Otherwise we'll verify in integration testing. > > > So I believe you're referring to the code in TemplateURLFetcher (I think) that > > rejects new TURLs that have already been added from a site. I'll see what they > > check in addition to the OSD URLs when trying to determine that and see if we > > need to modify how we handle dupes. > > The picture I have in my head is that there are cases where a keyword's URL can > be changed if the code thinks it's safe to replace the old URL with the new one. > I think this is used by the prepopulate stuff I'm talking about above, too? > Like, maybe if the old URL hasn't been user-modified, we'll happily replace it, > or something. I just want to make sure that if there is indeed such code, and > one computer updates a keyword using it while another sits idle, then when we > sync the two together we get one keyword with the new URL as a result. Ah okay. Those sound like scenarios where "safe_for_autoreplace" TURLs are replaced by a detected OSD or something. I'll look through the code to see where that flag is checked/set. As far as I know, these replacement scenarios involve calls to the normal TURLService API (Add/Remove/ResetTemplateURL) which have all been modified in this patch to transmit changes to Sync. I'll include an integration test to ensure that these autoreplace scenarios result in consistent clients. > > I'm sorry I'm being so vague. Scott's always been the one to work on this code > so I'm not terribly familiar with it :( No prob. Scott said he'd have a look through this when he gets back from vacation, but the extra eyes are helpful. > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > File chrome/browser/search_engines/template_url_service.h (right): > > http://codereview.chromium.org/7566036/diff/18002/chrome/browser/search_engin... > chrome/browser/search_engines/template_url_service.h:299: // |turl|. Returns > true if successful, false otherwise. > On 2011/08/10 15:15:12, SteveT wrote: > > Changed the wording. Actually this currently always returns true because we're > > not doing anything complicated enough to fail. I'm mostly taking the format > from > > Pref's CreatePrefSyncData method, which can possibly fail. I can just make > this > > void for now if that makes more sense. > > Yeah, I'd do that; it's easy to change the signature later as needed.
Fixed last couple nits and privatized helpers. Waiting on Nick's review now. http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:184: // TemplateURL. The TemplateURL is marked as not replaceable. If |guid| is On 2011/08/10 17:42:50, Peter Kasting wrote: > Nit: This comment is out of date. Done. http://codereview.chromium.org/7566036/diff/21001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:191: // Resets the sync GUID of the specified TemplateURL and persist the change to On 2011/08/10 17:42:50, Peter Kasting wrote: > Nit: persist -> persists Done.
Initial comments, still need to go through unit tests. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:634: current_data.push_back(data); May as well combine these into a single line (...push_back(CreateSyncdata...) if you're not doing any checks. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:659: LOG(ERROR) << "Failed to read search engine."; NOTREACHED() << "Failed..." http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:688: LOG(ERROR) << "Unexpected sync change state."; NOTREACHED() << "..." I would also return a sync_error in this case with info about the actions requested. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:695: if (sync_error.IsSet()) You can just return sync_error here without the conditional http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:722: const TemplateURL* sync_turl = CreateTemplateURLFromSyncData(iter->second); Maybe remove const so you don't need to const_cast below? (And use a separate const var for the duplicate turl) http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:730: if (sync_turl->last_modified() > local_turl->last_modified()) { Can this logic be replaced by ResolveSyncKeywordConflict or MergeSyncAndLocalUrlDuplicates? Seems there's logic being duplicated here. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:811: AutoReset<bool> processing_changes(&processing_syncer_changes_, true); This shouldn't be necessary. If the sync processor calls ProcessSyncChanges on you, you set the bool there. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:880: se_specifics->keyword(), fix indents http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:20: #include "chrome/browser/sync/api/sync_change.h" Do you have to include sync_change? http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:22: #include "chrome/browser/sync/protocol/search_engine_specifics.pb.h" Same, I don't see the specifics being used anywhere in the header http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:478: // conflicting local keyword (whichever is older). If the cloud TURL is Just a thought, are you sure the oldest one is the one you want to modify? Seems to me you may want to preserve the oldest and change the newer, since the older is more likely to have been in use (in my view). I'm imagining the case where the newest was added on a machine that had fallen behind (in sync terms). Your call.
http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:17: static std::string GetGUID(const SyncData& sync_data) { I think anonymous namespaces are preferred over static functions. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:52: return SyncError(); Would be good to also test that your code handles SyncErrors from ProcessSyncChanges gracefully. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:170: EXPECT_EQ(GetKeyword(iter1->second), GetKeyword(iter2->second)); Shouldn't these be asserts? http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:193: CreateTestTemplateURL("key1", "http://key1.com", "key1", 90); This should be a scoped_ptr I think. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:316: EXPECT_EQ(0, static_cast<int>(changes.size())); nit: use 0U instead of static casting (and in other places) http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:327: changes.erase(changes.begin()); changes.clear(); http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:377: model()->FindDuplicateOfSyncTemplateURL(*sync_turl); ASSERT_NE(NULL, dupe_turl) http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:534: EXPECT_TRUE(processor()->ContainsGUID("xyz")); ASSERT_TRUE http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:544: model()->Add(CreateTestTemplateURL( I think it's better to make changes to copies of the initial sync data. That way you don't have to manually ensure the url's/keyword literals match those in CreateInitialSyncData if the test gets modified. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:565: EXPECT_TRUE(processor()->ContainsGUID("key1")); ASSERT here and elsewhere you rely on getting the the change from the processor http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:895: model()->ProcessSyncChanges(FROM_HERE, changes); check that it returned an error.
Here's a patch addressing your first set of comments. Almost everything is dealt with, save a few additional questions/explanations. Let me know if any more work needs to be done here. Also in this patch, I addressed the unit test crash in NotificationService that we discussed offline. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:634: current_data.push_back(data); On 2011/08/15 17:01:02, nzea wrote: > May as well combine these into a single line (...push_back(CreateSyncdata...) if > you're not doing any checks. Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:659: LOG(ERROR) << "Failed to read search engine."; On 2011/08/15 17:01:02, nzea wrote: > NOTREACHED() << "Failed..." Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:688: LOG(ERROR) << "Unexpected sync change state."; Done. Added SyncChange::ChangeTypeToString to convert the enum into a string name. I'm not sure what the normal convention is for such a function, so let me know if you prefer it written another way. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:695: if (sync_error.IsSet()) On 2011/08/15 17:01:02, nzea wrote: > You can just return sync_error here without the conditional Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:722: const TemplateURL* sync_turl = CreateTemplateURLFromSyncData(iter->second); On 2011/08/15 17:01:02, nzea wrote: > Maybe remove const so you don't need to const_cast below? (And use a separate > const var for the duplicate turl) Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:730: if (sync_turl->last_modified() > local_turl->last_modified()) { No, this bit of code actually does something fairly different from resolving keywords or dupe resolution. Here we are dealing with already synced TURLs and no real conflicts. We compare their timestamps to see if there are some fields that have changed since the last update. The structure is similar to those two methods you mentioned, but one key thing to notice is that in the ==s case, we leave the local and sync data alone, as nothing has been changed at all since the last sync. In the other two methods, we have to handle this case since we're actually resolving some conflict. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:811: AutoReset<bool> processing_changes(&processing_syncer_changes_, true); On 2011/08/15 17:01:02, nzea wrote: > This shouldn't be necessary. If the sync processor calls ProcessSyncChanges on > you, you set the bool there. Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:880: se_specifics->keyword(), On 2011/08/15 17:01:02, nzea wrote: > fix indents Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:20: #include "chrome/browser/sync/api/sync_change.h" So the three dependencies I had were: - SyncChange (can forward declare this) - SyncChangeList (can re-typedef this) - SyncChange::SyncChangeType (an enum) Not sure how I can get rid of the dependency on SyncChangeType. The only thing I can think of is redefining it in a separate header with just the enum... but it's probably not worth it. Can you suggest something better? http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:22: #include "chrome/browser/sync/protocol/search_engine_specifics.pb.h" Oops, yeah. I moved all the Specifics dependencies to the cc some time ago. Moved this header over, as well. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:478: // conflicting local keyword (whichever is older). If the cloud TURL is Yeah, so that is a good point, and I haven been thinking about this conflict resolution case for some time. I decided that the most recently modified should be the keeper, as the timestamp only changes if the user explicitly goes to the Search Engines settings and changes some field. This would imply to me that whatever keyword was set at that time would be the one they knew they wanted for that particular search engine. Once everything is synced, I would expect that the user wants to use that newest keyword on every client that has his search engines synced. Anyways, it's not a hard thing to change, so I would stick with this and change it later if during manual testing it really doesn't make sense.
I think that's everything for this file... over to you. PTAL. Thanks! http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:17: static std::string GetGUID(const SyncData& sync_data) { On 2011/08/15 19:33:33, nzea wrote: > I think anonymous namespaces are preferred over static functions. Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:52: return SyncError(); Added a couple SyncError tests below to test this. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:170: EXPECT_EQ(GetKeyword(iter1->second), GetKeyword(iter2->second)); On 2011/08/15 19:33:33, nzea wrote: > Shouldn't these be asserts? Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:193: CreateTestTemplateURL("key1", "http://key1.com", "key1", 90); On 2011/08/15 19:33:33, nzea wrote: > This should be a scoped_ptr I think. Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:316: EXPECT_EQ(0, static_cast<int>(changes.size())); On 2011/08/15 19:33:33, nzea wrote: > nit: use 0U instead of static casting (and in other places) Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:327: changes.erase(changes.begin()); On 2011/08/15 19:33:33, nzea wrote: > changes.clear(); Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:377: model()->FindDuplicateOfSyncTemplateURL(*sync_turl); Done, but used ASSERT_TRUE() instead, since the compiler doesn't seem to like comparing NULL to TemplateURL pointers. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:534: EXPECT_TRUE(processor()->ContainsGUID("xyz")); On 2011/08/15 19:33:33, nzea wrote: > ASSERT_TRUE Done. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:544: model()->Add(CreateTestTemplateURL( Done here and below. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:565: EXPECT_TRUE(processor()->ContainsGUID("key1")); Done everywhere. http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:895: model()->ProcessSyncChanges(FROM_HERE, changes); On 2011/08/15 19:33:33, nzea wrote: > check that it returned an error. Done.
http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/27001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:20: #include "chrome/browser/sync/api/sync_change.h" On 2011/08/15 21:13:58, SteveT wrote: > So the three dependencies I had were: > - SyncChange (can forward declare this) > - SyncChangeList (can re-typedef this) > - SyncChange::SyncChangeType (an enum) > > Not sure how I can get rid of the dependency on SyncChangeType. The only thing I > can think of is redefining it in a separate header with just the enum... but > it's probably not worth it. Can you suggest something better? Fair enough. http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:13: #include "chrome/test/base/testing_browser_process_test.h" last include is out of abc order http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:17: namespace { nit: blank line after (and before close brace) http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:35: } nit: extend the namespace to be the entire file and add a "// namespace" after close brace. http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:944: processor()->set_erroneous(false); This is a bit confusing as to why it would trigger an error (presumably it results in updating sync as opposed to the local model). I'd prefer calling model->Add(...) or something instead, which will then try to update sync. Here and below
Back to you, with perhaps one more thing to look at. PTAL. http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:13: #include "chrome/test/base/testing_browser_process_test.h" On 2011/08/15 23:40:00, nzea wrote: > last include is out of abc order Done. http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:17: namespace { On 2011/08/15 23:40:00, nzea wrote: > nit: blank line after (and before close brace) Done. http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:35: } On 2011/08/15 23:40:00, nzea wrote: > nit: extend the namespace to be the entire file and add a "// namespace" after > close brace. Done. http://codereview.chromium.org/7566036/diff/37001/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service_sync_unittest.cc:944: processor()->set_erroneous(false); Ah, sorry. I should have put a comment here to explain what was going on. In this case, it's not the SyncChanges that cause the second error.. the second error is expected because the model was never associated due to an error in MergeDataAndStartSyncing. I added a comment to that effect... let me know if it's clear now or if you'd rather I did something else.
LGTM
Hey Nick, Could you take a quick look? The original commit was reverted due to some heapcheck mem leaks, which I've fixed here in Patch 8. Thanks, Steve
LGTM
I took at look and have a handful of comments. -Scott http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:500: GetSearchProvidersUsingKeywordResult(*result, This may end up modifying the set of TemplateURLs (at least the prepopulate ones). You'll need to deal with that in some way. In fact it's worth a test case to make sure migration gets handled correctly. http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:691: return error; Are you sure we want to return here and not continue on? Say you have two machines, A and B. A is connected, B is offline. If you delete the same TURL on both machines, then B becomes connected, might you hit this branch? Maybe MergeData happens first so this isn't an issue? http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:701: SyncError TemplateURLService::MergeDataAndStartSyncing( Is it guaranteed these methods are only invoked after loading has completed? Could you add a DCHECK to that effect? http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:739: sync_turl->url() ? sync_turl->url()->url() : std::string()); I believe you'll also want safe_for_autoreplace copied over too, otherwise any synced TURL will end up with safe_for_autoreplace false. http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:874: se_specifics->set_id(turl.id()); Is there any protection to make sure we don't end up with duplicate ids? In fact do we even need to sync the id? http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.cc:1679: string16 new_keyword; nit: pull into if as you don't use it outside the if. http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:134: // The caller should not try to delete the returned pointer; the data store nit: Returns the TemplateURL with the specified guid, or NULL if not found. http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:475: // Given a TemplateURL from Sync, resolves any keyword conflicts by checking nit: add (cloud) after Sync. http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:484: SyncChangeList& change_list); Because this may modify change_list, it should be a pointer. http://codereview.chromium.org/7566036/diff/51007/chrome/browser/search_engin... chrome/browser/search_engines/template_url_service.h:500: SyncChangeList& change_list); change_list should be a pointer here too. |