Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2067723002:
[sync] Search engine shortcuts get underscores at the end after sync (Closed)
|
Created:
4 years, 6 months ago by Patrick Noland Modified:
4 years, 6 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[sync] Search engine shortcuts get underscores at the end after sync
Currently, search engines with matching prepopulate_id are treated as distinct if their keywords don't match. This can cause multiple copies of prepopulated search engines to be created; one copy with the original keyword and another copy with the modified keyword.
In the linked bug, the problematic behavior has created a different default search engine on each client. Deleting the non-default on client 1 is an attempt to delete client 2's default. There is sync code that assumes this was a mistake and resurrects the local default by appending an underscore.
This CL treats matching prepopulate_id as a conflict, and resolves it in favor of the remote search engine if the local engine isn't yet known to sync and was modified less recently. Note that this won't fix existing clients, whose duplicated engines will be known to sync already.
R=pkasting@chromium.org, zea@chromium.org
BUG=613108
Committed: https://crrev.com/8ec6ae8d86bc2badeb731ad4d85521d65aceae9c
Cr-Commit-Position: refs/heads/master@{#400017}
Patch Set 1 : [sync] Search engine shortcuts get underscores at the end after sync #
Total comments: 23
Patch Set 2 : [sync] Search engine shortcuts get underscores at the end after sync #
Total comments: 5
Patch Set 3 : Added requested comments #
Messages
Total messages: 19 (8 generated)
Description was changed from ========== [sync] Search engine shortcuts get underscores at the end after sync R=pkasting@chromium.org, zea@chromium.org BUG=613108 ========== to ========== [sync] Search engine shortcuts get underscores at the end after sync Currently, search engines with matching prepopulate_id are treated as distinct if their keywords don't match. This can cause multiple copies of prepopulated search engines to be created; one copy with the original keyword and another copy with the modified keyword. In the linked bug, the problematic behavior has created a different default search engine on each client. Deleting the non-default on client 1 is an attempt to delete client 2's default. There is sync code that assumes this was a mistake and resurrects the local default by appending an underscore. This CL treats matching prepopulate_id as a conflict, and resolves it in favor of the remote search engine if the local engine isn't yet known to sync and was modified less recently. Note that this won't fix existing clients, whose duplicated engines will be known to sync already. R=pkasting@chromium.org, zea@chromium.org BUG=613108 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
zea: please look at sync-related code in TemplateURLService::MergeInSyncTemplateURL
pkasting, could you please review the entire CL? Thanks
https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:66: int prepopulate_id = -1) { Nit: Can you do this, or does the compiler barf? int prepopulate_id = turl.prepopulate_id()) { (I never used to think this was legal but I guess at least certain kinds of function calls during default arguments are allowed? Not sure you can use |turl| in the expression though.) https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2232: TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(NULL)); Nit: nullptr https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2246: new_data.date_created = Time::FromTimeT(100); Nit: Set fields in the order they're declared within the struct https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2255: std::unique_ptr<TemplateURL> sync_turl(new TemplateURL(new_data)); Nit: Use MakeUnique<> over raw new where possible https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2271: MergeConflictingPrepopulatedEngineIntoDefault) { All comments from above apply. It kinda sucks that this test is long and almost entirely copy-pasted from the one above. Try to find ways to combine these into one test or in some other way share all this code. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2275: if (!conflicting_turl) { Nit: Just move the body of this conditional onto the } else { arm of the conditional below. If you do this you'll need to tweak the comment above that conditional, e.g.: // Resolve conflicts with local TemplateURLs. if (conflicting_turl) { // Modify |conflicting_turl| to make room for |sync_turl|. ... } else { // ... https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2276: TemplateURL* conflicting_prepopulated_turl = Nit: Consider a comment up here that summarizes this whole block https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2291: merge_result->set_num_items_deleted(merge_result->num_items_deleted() + Nit: Break after ( instead https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.h:640: const TemplateURL* sync_turl); Nit: I'd combine these two functions into one and pass it a third argument, bool prefer_local_default, that would toggle between the two behaviors. This allows you to avoid copy and pasting the whole long comment, and share the (admittedly brief) implementations of the two functions.
https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2276: TemplateURL* conflicting_prepopulated_turl = On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: Consider a comment up here that summarizes this whole block Seconded. This is fairly subtle, so would be good to explain what's intended. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2285: ApplyDefaultSearchChange(&sync_turl->data(), Why don't we remove the conflicting prepopulated turl here too? https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2295: } To be clear, in the else clause here we're explicitly deciding to let both the local and the sync items live side by side, despite conflicting?
Patchset #2 (id:60001) has been deleted
PTAL https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:66: int prepopulate_id = -1) { On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: Can you do this, or does the compiler barf? > > int prepopulate_id = turl.prepopulate_id()) { > > (I never used to think this was legal but I guess at least certain kinds of > function calls during default arguments are allowed? Not sure you can use > |turl| in the expression though.) I'd like to do this, but the compiler complains about a default argument referring to a parameter. https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2232: TemplateURLPrepopulateData::GetPrepopulatedDefaultSearch(NULL)); On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: nullptr Done. https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2246: new_data.date_created = Time::FromTimeT(100); On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: Set fields in the order they're declared within the struct Done. https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2255: std::unique_ptr<TemplateURL> sync_turl(new TemplateURL(new_data)); On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: Use MakeUnique<> over raw new where possible Done. https://codereview.chromium.org/2067723002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2271: MergeConflictingPrepopulatedEngineIntoDefault) { On 2016/06/14 19:21:25, Peter Kasting wrote: > All comments from above apply. > > It kinda sucks that this test is long and almost entirely copy-pasted from the > one above. Try to find ways to combine these into one test or in some other way > share all this code. Done. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2275: if (!conflicting_turl) { On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: Just move the body of this conditional onto the } else { arm of the > conditional below. > > If you do this you'll need to tweak the comment above that conditional, e.g.: > > // Resolve conflicts with local TemplateURLs. > if (conflicting_turl) { > // Modify |conflicting_turl| to make room for |sync_turl|. > ... > } else { > // ... Done. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2276: TemplateURL* conflicting_prepopulated_turl = On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: Consider a comment up here that summarizes this whole block Done. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2285: ApplyDefaultSearchChange(&sync_turl->data(), On 2016/06/14 21:19:44, Nicolas Zea wrote: > Why don't we remove the conflicting prepopulated turl here too? The logic in ApplyDefaultSearchChange effectively merges sync_turl into the default instead of removing then adding. I think this is because calling Remove() on the default isn't supposed to happen, maybe as a way of ensuring there's always a default. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2291: merge_result->set_num_items_deleted(merge_result->num_items_deleted() + On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: Break after ( instead Done. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.cc:2295: } On 2016/06/14 21:19:44, Nicolas Zea wrote: > To be clear, in the else clause here we're explicitly deciding to let both the > local and the sync items live side by side, despite conflicting? Yes. I've added a comment to this effect. The likely scenario in which this would happen is one where the user has modified the keyword on both clients, but to two different values e.g. "g1" and "g2." I'm not sure how to pick a winner in this scenario, so it seemed safer to just preserve both. https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... File components/search_engines/template_url_service.h (right): https://codereview.chromium.org/2067723002/diff/40001/components/search_engin... components/search_engines/template_url_service.h:640: const TemplateURL* sync_turl); On 2016/06/14 19:21:26, Peter Kasting wrote: > Nit: I'd combine these two functions into one and pass it a third argument, bool > prefer_local_default, that would toggle between the two behaviors. > > This allows you to avoid copy and pasting the whole long comment, and share the > (admittedly brief) implementations of the two functions. Done.
LGTM https://codereview.chromium.org/2067723002/diff/80001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_sync_unittest.cc (right): https://codereview.chromium.org/2067723002/diff/80001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_sync_unittest.cc:2256: std::unique_ptr<TemplateURL> sync_turl = Nit: Above each group of code blocks that implement a single test, write an overall comment about what you're verifying. https://codereview.chromium.org/2067723002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2067723002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:2309: // Check for a turl with a conflicting prepopulate_id. Nit: I generally try to say TemplateURL instead of turl in comments. Maybe add the sentence "This detects the case where the user changes a prepopulated engine's keyword on one client. We want to reflect this sort of change to that prepopulated URL on other clients instead of assuming that the changed TemplateURL is a new entity." https://codereview.chromium.org/2067723002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:2315: // precdence if it's been modified more recently and the local turl isn't Nit: precedence https://codereview.chromium.org/2067723002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:2317: // it didn't, we could end up with two distinct default search engines. Nit: This sentence seems a bit misleading (TemplateURLService can't actually have multiple default search engines). Maybe something like this comment for the whole thing here? "If we found a conflict, and the sync entity is better, apply the remote changes locally. We will consider the sync entity better even if the local one is the current default, since in this case being default does not necessarily mean the user explicitly set this TemplateURL as default. If we didn't do this, changes to the keywords of prepopulated default engines would never be applied to other clients. "If we can't safely replace the local entry with the synced one (or merge the relevant changes in), we give up and leave both." https://codereview.chromium.org/2067723002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:2325: // |sync_turl|. Nit: Is this comment strictly accurate for the first conditional arm? Maybe verbs like "Merge" or "Replace" would be more accurate in describing what ends up happening here. But if you apply my comment suggestion above, you can probably just drop this comment as redundant.
lgtm
The CQ bit was checked by pnoland@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, zea@chromium.org Link to the patchset: https://codereview.chromium.org/2067723002/#ps100001 (title: "Added requested comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2067723002/100001
Message was sent while issue was closed.
Description was changed from ========== [sync] Search engine shortcuts get underscores at the end after sync Currently, search engines with matching prepopulate_id are treated as distinct if their keywords don't match. This can cause multiple copies of prepopulated search engines to be created; one copy with the original keyword and another copy with the modified keyword. In the linked bug, the problematic behavior has created a different default search engine on each client. Deleting the non-default on client 1 is an attempt to delete client 2's default. There is sync code that assumes this was a mistake and resurrects the local default by appending an underscore. This CL treats matching prepopulate_id as a conflict, and resolves it in favor of the remote search engine if the local engine isn't yet known to sync and was modified less recently. Note that this won't fix existing clients, whose duplicated engines will be known to sync already. R=pkasting@chromium.org, zea@chromium.org BUG=613108 ========== to ========== [sync] Search engine shortcuts get underscores at the end after sync Currently, search engines with matching prepopulate_id are treated as distinct if their keywords don't match. This can cause multiple copies of prepopulated search engines to be created; one copy with the original keyword and another copy with the modified keyword. In the linked bug, the problematic behavior has created a different default search engine on each client. Deleting the non-default on client 1 is an attempt to delete client 2's default. There is sync code that assumes this was a mistake and resurrects the local default by appending an underscore. This CL treats matching prepopulate_id as a conflict, and resolves it in favor of the remote search engine if the local engine isn't yet known to sync and was modified less recently. Note that this won't fix existing clients, whose duplicated engines will be known to sync already. R=pkasting@chromium.org, zea@chromium.org BUG=613108 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:100001)
Message was sent while issue was closed.
CQ bit was unchecked
Message was sent while issue was closed.
Description was changed from ========== [sync] Search engine shortcuts get underscores at the end after sync Currently, search engines with matching prepopulate_id are treated as distinct if their keywords don't match. This can cause multiple copies of prepopulated search engines to be created; one copy with the original keyword and another copy with the modified keyword. In the linked bug, the problematic behavior has created a different default search engine on each client. Deleting the non-default on client 1 is an attempt to delete client 2's default. There is sync code that assumes this was a mistake and resurrects the local default by appending an underscore. This CL treats matching prepopulate_id as a conflict, and resolves it in favor of the remote search engine if the local engine isn't yet known to sync and was modified less recently. Note that this won't fix existing clients, whose duplicated engines will be known to sync already. R=pkasting@chromium.org, zea@chromium.org BUG=613108 ========== to ========== [sync] Search engine shortcuts get underscores at the end after sync Currently, search engines with matching prepopulate_id are treated as distinct if their keywords don't match. This can cause multiple copies of prepopulated search engines to be created; one copy with the original keyword and another copy with the modified keyword. In the linked bug, the problematic behavior has created a different default search engine on each client. Deleting the non-default on client 1 is an attempt to delete client 2's default. There is sync code that assumes this was a mistake and resurrects the local default by appending an underscore. This CL treats matching prepopulate_id as a conflict, and resolves it in favor of the remote search engine if the local engine isn't yet known to sync and was modified less recently. Note that this won't fix existing clients, whose duplicated engines will be known to sync already. R=pkasting@chromium.org, zea@chromium.org BUG=613108 Committed: https://crrev.com/8ec6ae8d86bc2badeb731ad4d85521d65aceae9c Cr-Commit-Position: refs/heads/master@{#400017} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/8ec6ae8d86bc2badeb731ad4d85521d65aceae9c Cr-Commit-Position: refs/heads/master@{#400017} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
