Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10806065:
Rewrite TemplateURLService's SyncableService implmentation to avoid sending ACTION_DELETEs to Sync. (Closed)
|
Created:
8 years, 5 months ago by SteveT Modified:
8 years, 4 months ago Reviewers:
Nicolas Zea CC:
chromium-reviews, Nicolas Zea Base URL:
http://git.chromium.org/chromium/src.git@master Visibility:
Public. |
DescriptionRewrite TemplateURLService's SyncableService implmentation to avoid sending ACTION_DELETEs to Sync.
Update unit tests to reflect these changes.
BUG=140732
TEST=
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=151391
Patch Set 1 : Initial draft #
Total comments: 23
Patch Set 2 : Addressed Nick's first batch of comments #Patch Set 3 : Addressed Nick's second set of comments #Patch Set 4 : Set the SyncedDSP pref when there is no pending DSP #Patch Set 5 : Cleaned out old methods and corrected unittests #Patch Set 6 : Cleanup new code #
Total comments: 19
Patch Set 7 : Initial round of new tests #Patch Set 8 : merge to TOT #Patch Set 9 : Addressed zea's initial comments #
Total comments: 17
Patch Set 10 : Added MergeInSyncTemplateURL test #Patch Set 11 : addressed zea comments round 2 #
Total comments: 10
Patch Set 12 : additional zea changes #
Total comments: 2
Patch Set 13 : comment nit #Patch Set 14 : fix assert #
Messages
Total messages: 19 (0 generated)
Hey Nick - giving you a preview of my rewrite. Note that this is not yet done yet: There's still a lot of cleanup, documentation, unit test updating and possible edge cases that I've missed... but I think that the implementation covers what we've discussed and I wanted you to take a look and reach consensus before I spend too much time on unit tests. In the mean time I'll be manually testing this to make sure the changes make sense. Let me know what you think.
Some initial higher-level comments. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:1156: MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes, given that this method supports finding a conflicting local turl and taking it into account, do you need the dupe detection at this level? Aren't they both looking for a duplicate by keyword? https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2355: // Synced GUID pref with the new GUID? What about in Hmm, interesting. Preferences are associated before search engines, so at this point the pref GUID should match what's been synced. So, unless somewhere after pref association but before this is called we're attempting to overwrite the pref guid, it shouldn't be possible for the unsynced local turl to be the pref guid. If it's not possible for something to attempt to write the guid at startup, then I'd say we shouldn't be messing with the pref guid here, although some sort of histogram might be useful to double check. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2365: initial_data->erase(guid); does this need to be done for the case above as well? https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2382: // We might have to detect that case ourselves and explicitly set the pref? I'm not sure I follow; why would we want to update the pref at this point? This method should never update I thought, since it's updating the DSP based on the pref, right? https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2389: TemplateURL* local_turl, |sync_turl| and |local_turl| don't really make sense anymore since they're both known to sync? https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2526: if (IsFromSync(local_turl, sync_data)) { I don't think we want to delete the local turl if it's synced. Did you meant for this to be here?
This latest patch addresses the comments you had that required action. Let me know if there are any other high level concerns. I will be continuing with cleanups and unit tests shortly... https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:1156: MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes, So the above method (MergeSyncAndLocalURLDuplicates) finds true-dupes (that is, both the Keyword and URL are the same). I recall that we did discuss that maybe we don't care if the URL matches or not, and we should just take whichever entry is "better"? What do you think? Should we attempt to be extra smart about matching URLs, or treat them no differently than any other keyword conflict? https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2355: // Synced GUID pref with the new GUID? What about in Oh, so we have an ordering guarantee now that Preferences will be associated (and pref observers notified) before we get to this point? So local_turl at this point _could_ be the DSP, but it should change to point to a Synced entry sometime within MergeDataAndStartSyncing. If that is the case, then yeah, let's not mess with the SyncedDSPGUID pref. Removed the TODO. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2365: initial_data->erase(guid); Hm, yes, you're right. In either case, since local_turl is not known to sync, it will be completely overridden by the conflicting sync_turl. I'll pull the erase() out. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2382: // We might have to detect that case ourselves and explicitly set the pref? Ah right, I was thinking about just SetDefaultSearchProviderNoNotify as opposed to the extra logic layered in top by SetDefaultSearchProviderIfNewlySynced. Also, from your comment above, it sounds like the canonical synced DSP GUID pref should be synced before we even enter MergeDataAndStartSyncing, so calling just SetDefaultSearchProviderIfNewlySynced here is the right thing to do. Removed the TODO. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2389: TemplateURL* local_turl, This is true, but local_turl is already located on the local model when this method is asked to resolve conflicts... Either way, I'll rename |local_turl| to |conflicting_turl| to be a bit more descriptive... Suggestions welcome. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2526: if (IsFromSync(local_turl, sync_data)) { Hm yeah, this is actually wrong. What happens under our new no-deletes model in this case, when we have two _synced_ entries that have the same keyword and URL? I guess we want to uniquify one of them and keep both around. Maybe as I suggested above, we can stop treating this as a special case and just roll the logic into ResolveSyncKeywordConflicts2?
https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:1156: MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes, On 2012/07/23 21:07:00, SteveT wrote: > So the above method (MergeSyncAndLocalURLDuplicates) finds true-dupes (that is, > both the Keyword and URL are the same). > > I recall that we did discuss that maybe we don't care if the URL matches or not, > and we should just take whichever entry is "better"? > > What do you think? Should we attempt to be extra smart about matching URLs, or > treat them no differently than any other keyword conflict? How is the behavior between the two different? It seems like they both wind up looking at modification time anyways, while MergeInSyncTemplateURL also looks at created by policy and default? MergeInSync looks like it's basically doing the same work. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2355: // Synced GUID pref with the new GUID? What about in On 2012/07/23 21:07:00, SteveT wrote: > Oh, so we have an ordering guarantee now that Preferences will be associated > (and pref observers notified) before we get to this point? > > So local_turl at this point _could_ be the DSP, but it should change to point to > a Synced entry sometime within MergeDataAndStartSyncing. > > If that is the case, then yeah, let's not mess with the SyncedDSPGUID pref. > > Removed the TODO. Yeah...it's not a guarantee, given that preferences might hit an error or something. That said, the case we're concerned about here is an unsynced search engine being the default, and losing the dupe-resolution with a synced search engine. If this happens, then yes, I think we should set the pref GUID to the new one. If we didn't, then other clients are going to be stuck in a state of pending guid, since the unsynced local SE would never have been added. I suspect we can only get into this state if SE/preferences hit some error/crash in a weird way...but honestly, in sync, if it _CAN_ happen...it will. And several thousand times at that. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2389: TemplateURL* local_turl, On 2012/07/23 21:07:00, SteveT wrote: > This is true, but local_turl is already located on the local model when this > method is asked to resolve conflicts... Either way, I'll rename |local_turl| to > |conflicting_turl| to be a bit more descriptive... Suggestions welcome. maybe unapplied_sync_turl and applied_sync_turl? https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2526: if (IsFromSync(local_turl, sync_data)) { On 2012/07/23 21:07:00, SteveT wrote: > Hm yeah, this is actually wrong. > > What happens under our new no-deletes model in this case, when we have two > _synced_ entries that have the same keyword and URL? I guess we want to uniquify > one of them and keep both around. > > Maybe as I suggested above, we can stop treating this as a special case and just > roll the logic into ResolveSyncKeywordConflicts2? Yeah, I think that's the right approach.
More discussion inline. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:1156: MergeInSyncTemplateURL(sync_turl.get(), sync_data_map, &new_changes, Actually yeah, MergeInSyncTemplateURL is sort of a superset of MergeSyncAndLocalURLDuplicates. In fact, it's a little bit smarter in that it considers policy and DSP, like you said. I had thought that MergeSyncAndLocalURLDuplicates handled a case where the URLs are the same that might be missed by MergeIn, but it seems like MergeIn implicitly takes care of that. MergeIn also gracefully handles the case where |local_turl| is actually from Sync (where we have to uniquify). MergeSyncAndLocalURLDuplicates doesn't handle this case, but like I asserted in an earlier email, this is currently not possible due to the data we have in the server. So I will simplify this section to just called MergeInSyncTemplateURL. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2355: // Synced GUID pref with the new GUID? What about in So the problem here isn't the local_turl losing conflict res (in fact, it can't, since one requirement is to not be the DSP). The problem here is if an unsynced local_turl wins... in which case we exchange it's guid for sync_turl's guid (which is effectively deleting it). In this case, if local_turl is the DSP, we should probably set the SyncedDSPGUID pref with the new guid from sync. I wrote some code to do this. The one issue with this is when the new pref has already arrived before search engines sync, but we haven't yet received the expected default entry in our MergeDataAndStartSyncing loop. In this case, we will override the pref with the local default's GUID, which sort of breaks how Preferences sync works, right? (The Prefs sync model always prefers the value from Sync over the local value, but for this particular case and pref, we're breaking that rule) Does this seem OK, or do we need to avoid doing this? We _could_ hold off on setting the SyncedDSPGUID until the end of the MergeDataAndStartSyncing loop (wait for our expected synced default to come in). If it doesn't, then we could set the local DSP as the new default. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2389: TemplateURL* local_turl, Sounds good. Done. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2526: if (IsFromSync(local_turl, sync_data)) { Oops, when I said "roll the logic into ResolveSyncKeywordConflcits2" I actually meant "just use MergeInSyncTemplateURL. The special case I was referring to was the fact that we try to do something special for when the URL is the same (combine them into one rather than uniquifying).
Rest of logic LG. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2355: // Synced GUID pref with the new GUID? What about in On 2012/07/24 14:46:30, SteveT wrote: > So the problem here isn't the local_turl losing conflict res (in fact, it can't, > since one requirement is to not be the DSP). The problem here is if an unsynced > local_turl wins... in which case we exchange it's guid for sync_turl's guid > (which is effectively deleting it). > > In this case, if local_turl is the DSP, we should probably set the SyncedDSPGUID > pref with the new guid from sync. I wrote some code to do this. > > The one issue with this is when the new pref has already arrived before search > engines sync, but we haven't yet received the expected default entry in our > MergeDataAndStartSyncing loop. > > In this case, we will override the pref with the local default's GUID, which > sort of breaks how Preferences sync works, right? (The Prefs sync model always > prefers the value from Sync over the local value, but for this particular case > and pref, we're breaking that rule) > > Does this seem OK, or do we need to avoid doing this? We _could_ hold off on > setting the SyncedDSPGUID until the end of the MergeDataAndStartSyncing loop > (wait for our expected synced default to come in). If it doesn't, then we could > set the local DSP as the new default. Ah, I see. So we're in one of two states: 1) We locally changed the DSP, and there is no pending synced default change. 2) We locally changed the DSP while there was a pending synced default change. For 1, I think it's safe to say that we should update the pref guid. For 2, it's basically a valid race right? And we in fact do have a way to detect the the race, since we can check for pending DSP. So if we have a pending DSP, don't overwrite real DSP pref, but update the local DSP engine to use the new sync GUID. WDYT?
Response inline. Let me know if this latest change sounds good. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2355: // Synced GUID pref with the new GUID? What about in Yes, so in (1), pending_synced_default_change_ is not set, which means at the beginning of MergeDataAndStartSyncing we did not expect one of the incoming TURLs to be the synced DSP -OR- we already received the incoming DSP (earlier in MergeData, or during Prefs' MergeData). Either way, it seems safe to set the SyncedDSPGUID, since it's either a no-op (writing the same GUID into the pref), or we're telling Sync that the DSP should be this new value. One tricky edge case here stems from the fact that we are merging based on keywords. Back when we had MergeSyncAndLocalDuplicates, doing this would be fine since we know the URLs were the same. In this case, you are hoping that because they had the same keywords, they have similar URLs pointing to the same search host. So for example, if this edge case was triggered and the user had local_turl keyword "q" and URL "google.com?q={searchTerm} and sync_turl had keyword "q" with URL "yahoo.com?q={searchTerm}... the logic will assign this yahoo search engine to be the default. This is a weird edge case, but maybe it's OK because we want to go with the notion of sync data winning. In (2), the pending_synced_default_change_ flag is true, which means we are still waiting on a synced TURL to come in from this batch of sync data. If this currently sync_turl happens to be that entry... then the call to SetDefaultSearchProviderIfNewlySynced will take care of that after we replace local_turl's sync_guid with sync_turl's sync_guid. But yeah, it sounds like this is the case where we don't want to override the pref... since it'll lead to problems similar to what your initial fix to SetDefaultSearchProviderNoNotify addressed (to not overwrite the pref when we're waiting for the default to come). It's the same race issue. Now what if this batch of sync data doesn't come in with an entry that has sync_guid == the SyncedDSPGUID? (perhaps it was deleted due to the DSP swap bug... Raz still needs to get us numbers on this) In this case, would we want to attempt to repair the sync data by setting the current local DSP to be the synced default? (see my last my comment about waiting til the end of MergeDataAndStartSyncing to do this)
New logic LG, let me know when it's ready for the full review. https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... File chrome/browser/search_engines/template_url_service.cc (right): https://chromiumcodereview.appspot.com/10806065/diff/2001/chrome/browser/sear... chrome/browser/search_engines/template_url_service.cc:2355: // Synced GUID pref with the new GUID? What about in On 2012/07/24 21:51:18, SteveT wrote: > Yes, so in (1), pending_synced_default_change_ is not set, which means at the > beginning of MergeDataAndStartSyncing we did not expect one of the incoming > TURLs to be the synced DSP -OR- we already received the incoming DSP (earlier in > MergeData, or during Prefs' MergeData). Either way, it seems safe to set the > SyncedDSPGUID, since it's either a no-op (writing the same GUID into the pref), > or we're telling Sync that the DSP should be this new value. > > One tricky edge case here stems from the fact that we are merging based on > keywords. Back when we had MergeSyncAndLocalDuplicates, doing this would be fine > since we know the URLs were the same. In this case, you are hoping that because > they had the same keywords, they have similar URLs pointing to the same search > host. So for example, if this edge case was triggered and the user had > local_turl keyword "q" and URL "google.com?q={searchTerm} and sync_turl had > keyword "q" with URL "yahoo.com?q={searchTerm}... the logic will assign this > yahoo search engine to be the default. > > This is a weird edge case, but maybe it's OK because we want to go with the > notion of sync data winning. > > > In (2), the pending_synced_default_change_ flag is true, which means we are > still waiting on a synced TURL to come in from this batch of sync data. If this > currently sync_turl happens to be that entry... then the call to > SetDefaultSearchProviderIfNewlySynced will take care of that after we replace > local_turl's sync_guid with sync_turl's sync_guid. > > But yeah, it sounds like this is the case where we don't want to override the > pref... since it'll lead to problems similar to what your initial fix to > SetDefaultSearchProviderNoNotify addressed (to not overwrite the pref when we're > waiting for the default to come). It's the same race issue. > > > Now what if this batch of sync data doesn't come in with an entry that has > sync_guid == the SyncedDSPGUID? (perhaps it was deleted due to the DSP swap > bug... Raz still needs to get us numbers on this) In this case, would we want > to attempt to repair the sync data by setting the current local DSP to be the > synced default? (see my last my comment about waiting til the end of > MergeDataAndStartSyncing to do this) I don't think we should be attempting to repair anything. It's not clear to me we have enough information to detect that the dsp is not on it's way. The system can handle a pending DSP guid that never arrives. In this case, the local DSP will remain the same (although the guid changed) right? I think that's the right behavior if you have a DSP change that never arrives.
Okay, I still have to write some new unit tests for MergeInTemplateURL and the new ResolveSyncKeywordConflict, but I think we can start the full review for the functionality and changes to existing unit tests, if you don't mind. Sync Integration tests are all still green.
Some comments, still need to look at tests. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2310: DCHECK(applied_sync_turl->keyword() == unapplied_sync_turl->keyword()); dcheck_eq http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2311: DCHECK(!applied_sync_turl->IsExtensionKeyword()); dcheck that unapplied_sync_turl's guid is not known locally? (to ensure no one gets the params swapped) http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2331: TemplateURL new_turl(applied_sync_turl->profile(), data); Is this created because we can't directly applied_sync_turl? http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2355: bool should_add_sync_turl = conflicting_turl == NULL; nit: I think it's more readable to have should_add_sync_turl = true here, then change the if statement below to be if (conflicting_turl), and ensure you set to true/false as needed. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2385: // override the pref with our new GUID. Note that the default might I'm not sure I understand the "note:" part. What situation is it referring to? http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2405: // Remove the entry from the initial data so it isn't pushed up to Sync. Mention that we do this because we either removed it or it sync overrode it. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:535: // The criteria for if |local_turl| if better than |sync_turl| is as follows: "is as follows" -> "is whether any of the following are true" http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:558: // during MergeDataAndStartSyncing. mention that this should only be called during MergeData? http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:568: SyncDataMap* initial_data); perhaps rename initial_data to local_data?
I've addressed your initial comments and added my first round of tests (including bringing back still-applicable tests for ResolveSyncKeywordConflict). I have one more set of tests coming shortly... PTAL at the responses so far. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2310: DCHECK(applied_sync_turl->keyword() == unapplied_sync_turl->keyword()); On 2012/07/28 00:27:32, nzea wrote: > dcheck_eq Done. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2311: DCHECK(!applied_sync_turl->IsExtensionKeyword()); Hm, this DCHECK would fire in the scenario where we receive an ACTION_UPDATE for the case where unapplied_sync_turl is sent in as an UPDATE to an existing TemplateURL (their sync_guids would therefore be the same). This is something that ResolveSyncKeywordConflict has always supported as it is a legitimate case... I can't think of anything that might go wrong if we proceed without this check. However, the name unapplied_sync_turl might be a little misleading... Thoughts? http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2331: TemplateURL new_turl(applied_sync_turl->profile(), data); Yeah, that's how UpdateNoNotify works... it requires a second TemplateURL with the updated fields to apply changes to an existing TemplateURL. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2355: bool should_add_sync_turl = conflicting_turl == NULL; On 2012/07/28 00:27:32, nzea wrote: > nit: I think it's more readable to have should_add_sync_turl = true here, then > change the if statement below to be if (conflicting_turl), and ensure you set to > true/false as needed. Done. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2385: // override the pref with our new GUID. Note that the default might I was referring to the case where |pending_synced_default_search_| was initially true, but was set to false because we found and set the synced default earlier. Thinking about it more... if the above occurred, we wouldn't hit this code, since it means some other TURL was set as the DSP... and that TURL couldn't be conflicting_turl in this case, since we only hit this case if conflicting_turl was initially unknown to sync. I'll remove that Note part of the comment. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2405: // Remove the entry from the initial data so it isn't pushed up to Sync. On 2012/07/28 00:27:32, nzea wrote: > Mention that we do this because we either removed it or it sync overrode it. Done. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:535: // The criteria for if |local_turl| if better than |sync_turl| is as follows: On 2012/07/28 00:27:32, nzea wrote: > "is as follows" -> "is whether any of the following are true" Done. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:558: // during MergeDataAndStartSyncing. On 2012/07/28 00:27:32, nzea wrote: > mention that this should only be called during MergeData? Done. http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:568: SyncDataMap* initial_data); On 2012/07/28 00:27:32, nzea wrote: > perhaps rename initial_data to local_data? Done.
http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/10806065/diff/22001/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2311: DCHECK(!applied_sync_turl->IsExtensionKeyword()); On 2012/08/05 20:22:39, SteveT wrote: > Hm, this DCHECK would fire in the scenario where we receive an ACTION_UPDATE for > the case where unapplied_sync_turl is sent in as an UPDATE to an existing > TemplateURL (their sync_guids would therefore be the same). > > This is something that ResolveSyncKeywordConflict has always supported as it is > a legitimate case... I can't think of anything that might go wrong if we proceed > without this check. However, the name unapplied_sync_turl might be a little > misleading... Thoughts? Ah, I see. Not sure if there's a better name. I guess the only thing would be to ensure that applied_sync_turl's _is_ known locally, since unapplied might but isn't necessarily. http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2373: // |sync_turl|, skip the following preparation steps and just |sync_turl| and just -> and just add http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2419: // Remove the entry from the initial data so it isn't pushed up to Sync. initial data -> local data http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:532: // The criteria for if |local_turl| if better than |sync_turl| is whether any "if better" -> is better http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:736: // update. The local copy should have received the sync data's GUID. In this case, the reason we push the local copy upstream is to update the last modified time only right? http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:750: EXPECT_EQ(ASCIIToUTF16("key2"), key2->keyword()); check key2 url? http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:797: EXPECT_FALSE(model()->GetTemplateURLForGUID("aaa")); check processor didn't receive anything for either key1 or aaa http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1129: EXPECT_EQ(processor()->change_list_size(), 2U); Why does the synced other turl get updated? Doesn't it already have a keyword "sync2", and a later modified time (150)? http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1669: EXPECT_EQ(keyword, winner->keyword()); check url too? Also that the processor received an update for "key1" with the new url (whatever.com)?
Addressed your comments and added a final batch of tests (see MergeInSyncTemplateURL in the unit tests). I am not 100% satisfied with this pattern of looping over a set of test inputs. Originally I had a separate helper method on the TemplateURLServiceSyncTest class which took each of the test_case fields as a parameter. However, friend-ing the test case with the test class didn't seem to allow calling MergeInTemplateURL (which is private) from the helper method. Anyways... let me know what you think of this new test. Happy to hear suggestions. http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.cc (right): http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2373: // |sync_turl|, skip the following preparation steps and just |sync_turl| On 2012/08/06 19:35:23, nzea wrote: > and just -> and just add Done. http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.cc:2419: // Remove the entry from the initial data so it isn't pushed up to Sync. On 2012/08/06 19:35:23, nzea wrote: > initial data -> local data Done. http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service.h (right): http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service.h:532: // The criteria for if |local_turl| if better than |sync_turl| is whether any On 2012/08/06 19:35:23, nzea wrote: > "if better" -> is better Done. http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:736: // update. The local copy should have received the sync data's GUID. That is the idea, and this should still happen after our refactor which removes the original MergeSyncAndLocalDuplicates method. We update the timestamp so that the next time this item is brought in during MergeDataAndStartSyncing, we don't have to make any changes (since GUIDs and last_modified will match between the local and sync data). http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:750: EXPECT_EQ(ASCIIToUTF16("key2"), key2->keyword()); On 2012/08/06 19:35:23, nzea wrote: > check key2 url? Done. http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:797: EXPECT_FALSE(model()->GetTemplateURLForGUID("aaa")); On 2012/08/06 19:35:23, nzea wrote: > check processor didn't receive anything for either key1 or aaa Done. http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1129: EXPECT_EQ(processor()->change_list_size(), 2U); This particular test sets the autogenerate_keyword field of the TemplateURLs to true, which forces the TemplateURLService to generate a new keyword for the synced-in entries. I don't fully recall why this didn't just try to keep the keyword it had originally if that was unique to the system. Looking into it a bit more, the autogenerate_keyword field is something that Peter deprecated a few releases back. He mentions that we can eventually remove uses of this field from the code after we've shipped a few stable releases (which is roughly now, since M21 is stable and this change to autogenerate_keyword was made in M19 or so... though I haven't fully thought out how the throttling of M20 users might affect this). I can inquire with Peter about the details of this change and we can remove that field (and therefore this test) in a separate change? http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1669: EXPECT_EQ(keyword, winner->keyword()); On 2012/08/06 19:35:23, nzea wrote: > check url too? Also that the processor received an update for "key1" with the > new url (whatever.com)? Done.
http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/10806065/diff/22003/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1129: EXPECT_EQ(processor()->change_list_size(), 2U); On 2012/08/07 00:17:09, SteveT wrote: > This particular test sets the autogenerate_keyword field of the TemplateURLs to > true, which forces the TemplateURLService to generate a new keyword for the > synced-in entries. > > I don't fully recall why this didn't just try to keep the keyword it had > originally if that was unique to the system. Looking into it a bit more, the > autogenerate_keyword field is something that Peter deprecated a few releases > back. He mentions that we can eventually remove uses of this field from the code > after we've shipped a few stable releases (which is roughly now, since M21 is > stable and this change to autogenerate_keyword was made in M19 or so... though I > haven't fully thought out how the throttling of M20 users might affect this). > > I can inquire with Peter about the details of this change and we can remove that > field (and therefore this test) in a separate change? SG http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1652: TemplateURL* default_turl = CreateTestTemplateURL(keyword, url, nit: style guide prefers separate lines per param if they don't all fit on one line. http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1847: // Set up and executes a MergeInSyncTemplateURL test given a number of Set up -> sets up http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1882: // keywords, with no updates send. I suppose there's no sync change for the local here since that gets added by MergeDataAndStartSyncing, not MergeInSyncTemplateURL? Perhaps update the comment to make that clear http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1888: DCHECK(test_cases[i].conflict_winner != BOTH); dcheck_ne for all these http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1955: ASSERT_TRUE(model()->GetTemplateURLForGUID(local_guid)); I think it would be useful to verify that the keyword, url, and last modified time match the expected. Perhaps the easiest way to do that is to, above, set an expected_local_keyword/expected_sync_keyword as necessary (and url and mtime), instead of verifying directly, then do the actual verifying here?
PTAL. http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1652: TemplateURL* default_turl = CreateTestTemplateURL(keyword, url, On 2012/08/07 17:45:43, nzea wrote: > nit: style guide prefers separate lines per param if they don't all fit on one > line. Done. http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1847: // Set up and executes a MergeInSyncTemplateURL test given a number of On 2012/08/07 17:45:43, nzea wrote: > Set up -> sets up Done. http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1882: // keywords, with no updates send. You're referring to the ACTION_ADD for the local TemplateURL, right? If so, yes. I've updated the comment to mention this. http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1888: DCHECK(test_cases[i].conflict_winner != BOTH); On 2012/08/07 17:45:43, nzea wrote: > dcheck_ne for all these Done. http://codereview.chromium.org/10806065/diff/14006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1955: ASSERT_TRUE(model()->GetTemplateURLForGUID(local_guid)); Done for the expected keyword. Note that we don't have to keep separate copies of the expected URL and last_modified timestamp as they do not change in these test cases. We just need to verify that they're the same as when they were first created.
Nice job, LGTM! http://codereview.chromium.org/10806065/diff/22006/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/10806065/diff/22006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1875: // added to the model, and UPDATE for it is sent. and an
Hey, thanks for reviewing and putting up with the erratic patches - have been trying to parallelize this work as much as possible with other stuff. I'll be running a slew of manual tests to make sure things look right before committing this bad boy. Then I'll be watching the UMA stats to make sure things go well. I have a hunch that this may eliminate all the Model Association failure SyncErrors since we're now much more careful about when we send ACTION_UPDATEs. http://codereview.chromium.org/10806065/diff/22006/chrome/browser/search_engi... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): http://codereview.chromium.org/10806065/diff/22006/chrome/browser/search_engi... chrome/browser/search_engines/template_url_service_sync_unittest.cc:1875: // added to the model, and UPDATE for it is sent. On 2012/08/10 17:34:09, nzea wrote: > and an Done.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/stevet@chromium.org/10806065/27004
Change committed as 151391 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
