Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2659353002:
Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) (Closed)
|
Created:
3 years, 10 months ago by Alexander Yashkin Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland)
Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix.
This is reland of https://codereview.chromium.org/2516963002
and https://codereview.chromium.org/2524733008.
"Current TemplateUrl constructor performs replacement of
search_terms_replacement_key field after initialization from
TemplateURLData. This replacement was not handled correctly in
TemplateUrl::MatchesData function. As result, TemplateUrl created from
TemplateUrlData for google engine could not be matched to its own
source data."
Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines -
after RepairPrepopulatedEngines sync guid preference was not updated correctly.
R=pkasting@chromium.org, vasilii@chromium.org
BUG=686399
TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 and https://bugs.chromium.org/p/chromium/issues/detail?id=690345 are fixed.
Review-Url: https://codereview.chromium.org/2659353002
Cr-Commit-Position: refs/heads/master@{#450937}
Committed: https://chromium.googlesource.com/chromium/src/+/02d3f50b4f9429a499eb1604bf315f0ce03163a1
Patch Set 1 #Patch Set 2 : Fixed TemplateUrlService not updating sync guid after Repair #
Total comments: 15
Patch Set 3 : Added another test for RepairPrepopulatedSearchEngines #
Total comments: 20
Patch Set 4 : Fixed after review #
Total comments: 2
Patch Set 5 : Fixed after review, remove redundant check #
Total comments: 3
Patch Set 6 : Fixed after review round 3 #
Total comments: 33
Patch Set 7 : Fixed after review, round 4 #
Total comments: 11
Patch Set 8 : Fixed after review, round 5 #
Total comments: 4
Patch Set 9 : Fixed after review, round 6 #Patch Set 10 : Fixed deps #Messages
Total messages: 58 (21 generated)
https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1952: default_search_provider_source_ = source; Problem in https://bugs.chromium.org/p/chromium/issues/detail?id=680197 arise due to default_search_provider_source_ field being set after default_search_provider_ and all other engine manipulation. So for example when switching from USER to FALLBACK state (called from RepairPrepopulatedEngines) 1. ApplyDefaultSearchChangeNoMetrics is called with FALLBACK but current value of default_search_provider_source_ was still USER. 2. At the processing code for new FALLBACK state we call UpdateNoNotify which have check at the end: if (default_search_provider_ == existing_turl && default_search_provider_source_ == DefaultSearchManager::FROM_USER) { default_search_manager_.SetUserSelectedDefaultSearchEngine( default_search_provider_->data()); } And this check was succesfull because default_search_provider_source_ was still FROM_USER. And this code called SetUserSelectedDefaultSearchEngine which changed default search to user again and wrote fallback engine to user pref. 3. At the finish of RepairPrepopulatedEngines we had FALLBACK engine set as current source in TemplateURLService but fallback engine was also written to user pref and SyncGuid and on next browser start TemplateURLService is loaded as with user selected engine. As a solution I fixed current default_search_provider_source_ value assignment and added explicit set of user selected engine in RepairPrepopulatedEngines.
(Re-sending) This looks like a re-fix for the problem you fixed before, where the fix broke sync-after-reset. I still don't understand what the practical problem is that's being fixed here. Please explain how the issue you're fixing will manifest in real user-visible bad behavior. I'm especially gunshy now that I let one fix through even though I didn't understand the problem, and it broke things in subtle ways.
On 2017/01/30 at 12:08:23, pkasting wrote: > (Re-sending) > > This looks like a re-fix for the problem you fixed before, where the fix broke > sync-after-reset. > > I still don't understand what the practical problem is that's being fixed here. > Please explain how the issue you're fixing will manifest in real user-visible > bad behavior. I'm especially gunshy now that I let one fix through even though > I didn't understand the problem, and it broke things in subtle ways. 1. First and most important for me :) This CL is needed for my https://codereview.chromium.org/2639153002/ - "Make extensions DSE persistent in browser prefs" How I encountered this bug: when working on my CL for extensions provided search engine - I found that after my changes FindMatchingExtensionTemplateURL could not find extension installed engine from test extension. chrome/test/data/extensions/settings_override/manifest.json Test extension uses "prepopulated_id": 1 which corresponds to google engine so search_terms_replacement_key in extension engine has value kGoogleInstantExtendedEnabledKeyFull and is replaced inside TemplateURL constructor to google_util::kInstantExtendedAPIParam. After this replacement FindMatchingExtensionTemplateURL could not match TemplateURLData from extension DSE pref to extension engine stored in TemplateURLService. As a solution we could disable some extensions browser tests and hope that no one uses prepopulated_id=1 in extension manifest :) or rewrite FindMatchingExtensionTemplateURL to use extension_id for matching :) (sounds like this conversation already happened in another CL :)) 2. I think that you agree that TemplateURL::MatchesData has bug? It could not match google TemplateURLData to TemplateUrl created from it. Where are 5 calls to TemplateURL::MatchesData in chromium code and I bet that some of them already incorrectly matches google engine - which is not so rarely used in chromium :). Also looking at this substitution in TemplateUrl constructor, i could not find how its used in chromium and think that it could be removed at some time which will result in RepairPrepopulatedSearchEngines not correctly setting kSyncedDefaultSearchProviderGUID. 3. After looking at how TemplateURLService works while performing RepairPrepopulatedSearchEngines, I think its a miracle that it sets kSyncedDefaultSearchProviderGUID correctly :) and I strongly feel that this only happens if default fallback engine is google. Which is not problem for chromium but could be a problem for chromium derived browsers as Yandex browser. Also having TeplateURLService current engine source FROM_FALLBACK after RepairPrepopulatedSearchEngines and changing this to FROM_USER on browser restart seems to me like another strange behaviour. So I think my change in TeplateURLService is more of refactoring really than bugfix. Do you propose to keep this bugs and strange behaviours? Will we try to build new components around/based on them :)? Sorry for sarcasm or causticity, if any, could not keep feelings after several days parsing through code, trying to understand how this component correctly works.
Added another unittest to highlight problem described in third item above. Without my changes in TemplateURLService it incorrectly restores kSyncedDefaultSearchProviderGUID after RepairPrepopulatedSearchEngines.
a-v-y@yandex-team.ru changed reviewers: + vasilii@chromium.org
LGTM https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... File components/search_engines/template_url.cc (right): https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url.cc:179: return search_terms_replacement_key1 == search_terms_replacement_key2; Nit: This implementation is shorter and does the common-case check first, which as I recall you liked. I renamed the params since they didn't seem less clear and it cut the length noticeably. With the old names, this implementation is the same number of lines as your version. bool SearchTermsReplacementKeysMatch(const std::string& key1, const std::string& key2) { const auto IsInstantExtended = [](const std::string& key) { return (key == google_util::kInstantExtendedAPIParam) || (key == kGoogleInstantExtendedEnabledKeyFull); } return (key1 == key2) || (IsInstantExtended(key1) && IsInstantExtended(key2)); } https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:696: if (old_source == DefaultSearchManager::FROM_USER && Nit: Hoist this up to the parent conditional: if (...) { } else if (...) { } else { https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:698: DefaultSearchManager::FROM_FALLBACK) { If the old source was FROM_USER, will the new source always be FROM_FALLBACK? Or can it fall back to extension/policy sometimes? Separately, it'd be great if the Source enum actually documented exactly what each of these FROM_XXX values means :P https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:699: // Set fallback engine as user selected, because repair is considered user Nit: user -> a user https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:700: // action and new engine is expected to sync to other user devices. Nit: new -> the new; eliminate "user" https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1951: // is checking it to update user selected engine. Nit: Maybe "|default_search_provider_source_| must be set before calling UpdateNoNotify(), since that function needs to know the source of the update in question." I'd also stick a blank line after this assignment. https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:185: // Create an URL that appears to have been prepopulated, but won't be in the Nit: While here, can you fix "Create" to "Creates"? https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:191: void SetOverridenEngines(); Nit: Comment what this does. https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:303: new base::FundamentalValue(1)); Nit: Not for this CL, but it'd be nice if this API took a scoped_ptr to indicate ownership transfer. https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:926: TEST_F(TemplateURLServiceTest, RepairPrepopulatedEnginesUpdatesSyncGuid) { Nit: Maybe add a sentence above this declaration as to the purpose of the overall testcase? I would love to see this on all tests, honestly. https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:929: // Expect no synced DSE guid until user selected something or sync come in. Nit: Maybe "The synced DSE GUID should be empty until the user selects something or there is sync activity."? https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:940: // Add third-party default search engine. Nit: Blank line above this. It also seems a little odd to use "third-party" here, where I assume you just mean "user-provided". ("third-party" sounds to me like something that's sideloaded.) (2 places) https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:941: TemplateURL* user_dse = AddKeywordWithDate( Nit: Probably not for this CL, but it'd be nice if this function had fewer args overall (I think the last param is never set by any caller?), and more defaulted args, not only so that calls would be shorter, but so I wouldn't have to wonder about whether callsites that pass "UTF-8" vs. "UTF-8;UTF-16" are intentionally picking one vs. the other or just want a "sane default". https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:968: SetOverridenEngines(); Nit: "overridden" has two 'd's (many places) https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:975: TemplateURL* dse = model()->GetDefaultSearchProvider(); Nit: Can inline below https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:976: ASSERT_EQ(overriden_engine, dse); Nit: Can maybe be EXPECT?
How is everything happening in template_url_service.cc related to the CL description.
On 2017/02/02 at 14:57:05, vasilii wrote: > How is everything happening in template_url_service.cc related to the CL description. First attempt to land this change lead to uncovering another bug in TemplateURLService https://bugs.chromium.org/p/chromium/issues/detail?id=680197 I have written some details in bug description. There is a another bug in TemplateURLService::RepairPrepopulatedEngines function wich only starts to fail when this bug in MatchesData is fixed. Like one bug neutralize the effect of another. I think they must be fixed together, because reverting second fix without first fix can lead to broken RepairPrepopulatedEngines behaviour. Or do you mean that CL description should be updated?
On 2017/02/02 15:06:46, Alexander Yashkin wrote: > On 2017/02/02 at 14:57:05, vasilii wrote: > > How is everything happening in template_url_service.cc related to the CL > description. > > First attempt to land this change lead to uncovering another bug in > TemplateURLService > https://bugs.chromium.org/p/chromium/issues/detail?id=680197 > I have written some details in bug description. > > There is a another bug in TemplateURLService::RepairPrepopulatedEngines function > wich only starts to fail when this bug in MatchesData is fixed. > > Like one bug neutralize the effect of another. > I think they must be fixed together, because reverting second fix without first > fix can lead to broken RepairPrepopulatedEngines behaviour. > > Or do you mean that CL description should be updated? Is it possible to fix 680197 first and then land this one? If not, then the description is to be updated for sure because it's not covering template_url_service_* at all. Also rewrite the footer and make it clear which CL you are relanding and which bugs are fixed here.
Description was changed from ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data. R=pkasting@chromium.org BUG=686399 Review-Url: https://codereview.chromium.org/2516963002 Cr-Commit-Position: refs/heads/master@{#433504} ========== to ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix. This is reland of https://codereview.chromium.org/2516963002 and https://codereview.chromium.org/2524733008. "Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data." Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines - after RepairPrepopulatedEngines sync guid preference was not updated correctly. R=pkasting@chromium.org, vasilii@chromium.org BUG=686399 TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 is not reproducing. ==========
Description was changed from ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix. This is reland of https://codereview.chromium.org/2516963002 and https://codereview.chromium.org/2524733008. "Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data." Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines - after RepairPrepopulatedEngines sync guid preference was not updated correctly. R=pkasting@chromium.org, vasilii@chromium.org BUG=686399 TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 is not reproducing. ========== to ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix. This is reland of https://codereview.chromium.org/2516963002 and https://codereview.chromium.org/2524733008. "Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data." Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines - after RepairPrepopulatedEngines sync guid preference was not updated correctly. R=pkasting@chromium.org, vasilii@chromium.org BUG=686399 TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 is not reproducing. ==========
On 2017/02/02 at 15:23:57, vasilii wrote: > On 2017/02/02 15:06:46, Alexander Yashkin wrote: > > On 2017/02/02 at 14:57:05, vasilii wrote: > > > How is everything happening in template_url_service.cc related to the CL > > description. > > > > First attempt to land this change lead to uncovering another bug in > > TemplateURLService > > https://bugs.chromium.org/p/chromium/issues/detail?id=680197 > > I have written some details in bug description. > > > > There is a another bug in TemplateURLService::RepairPrepopulatedEngines function > > wich only starts to fail when this bug in MatchesData is fixed. > > > > Like one bug neutralize the effect of another. > > I think they must be fixed together, because reverting second fix without first > > fix can lead to broken RepairPrepopulatedEngines behaviour. > > > > Or do you mean that CL description should be updated? > > Is it possible to fix 680197 first and then land this one? If not, then the description is to be updated for sure because it's not covering template_url_service_* at all. > Also rewrite the footer and make it clear which CL you are relanding and which bugs are fixed here. Changed description and footer - see if it good enough.
> Is it possible to fix 680197 first and then land this one? If not, then the description is to be updated for sure because it's not covering template_url_service_* at all. > Also rewrite the footer and make it clear which CL you are relanding and which bugs are fixed here. I could fix 680197 first, but if this fix was to be reverted later, the problem with engine reset could return.
The CQ bit was checked by a-v-y@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2659353002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:694: ApplyDefaultSearchChange(new_dse, source); I can't correlate your changes with https://bugs.chromium.org/p/chromium/issues/detail?id=680197#c24 You claim that the engine isn't overwritten because MatchesData now returns true. But you won't move to the "else if" block you added in #695. What was happening and what should happen?
https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... File components/search_engines/template_url.cc (right): https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url.cc:179: return search_terms_replacement_key1 == search_terms_replacement_key2; On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: This implementation is shorter and does the common-case check first, which as I recall you liked. > > I renamed the params since they didn't seem less clear and it cut the length noticeably. With the old names, this implementation is the same number of lines as your version. > > bool SearchTermsReplacementKeysMatch(const std::string& key1, > const std::string& key2) { > const auto IsInstantExtended = [](const std::string& key) { > return (key == google_util::kInstantExtendedAPIParam) || > (key == kGoogleInstantExtendedEnabledKeyFull); > } > return (key1 == key2) || (IsInstantExtended(key1) && IsInstantExtended(key2)); > } Clever idea, done. https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:696: if (old_source == DefaultSearchManager::FROM_USER && On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: Hoist this up to the parent conditional: > > if (...) { > } else if (...) { > } else { Done https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:698: DefaultSearchManager::FROM_FALLBACK) { On 2017/02/02 at 00:04:26, Peter Kasting wrote: > If the old source was FROM_USER, will the new source always be FROM_FALLBACK? Or can it fall back to extension/policy sometimes? > I have kept the old logic here - fallback engine was written to user selected pref only if transferring FROM_USER -> FROM_FALLBACK At my current level of understanding, I think from FROM_USER we could only get to FROM_FALLBACK state. Yet, after encountering previous reset engine problem, I am a little bit gunshy of bold changes :) > Separately, it'd be great if the Source enum actually documented exactly what each of these FROM_XXX values means :P Done https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:699: // Set fallback engine as user selected, because repair is considered user On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: user -> a user Done https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:700: // action and new engine is expected to sync to other user devices. On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: new -> the new; eliminate "user" arghh, this articles, done. https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:1951: // is checking it to update user selected engine. On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: Maybe "|default_search_provider_source_| must be set before calling UpdateNoNotify(), since that function needs to know the source of the update in question." > > I'd also stick a blank line after this assignment. Done https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:191: void SetOverridenEngines(); On 2017/02/02 at 00:04:27, Peter Kasting wrote: > Nit: Comment what this does. Done https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:191: void SetOverridenEngines(); On 2017/02/02 at 00:04:27, Peter Kasting wrote: > Nit: Comment what this does. Done https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:303: new base::FundamentalValue(1)); On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: Not for this CL, but it'd be nice if this API took a scoped_ptr to indicate ownership transfer. Agree. https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:926: TEST_F(TemplateURLServiceTest, RepairPrepopulatedEnginesUpdatesSyncGuid) { On 2017/02/02 at 00:04:27, Peter Kasting wrote: > Nit: Maybe add a sentence above this declaration as to the purpose of the overall testcase? I would love to see this on all tests, honestly. Done https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:929: // Expect no synced DSE guid until user selected something or sync come in. On 2017/02/02 at 00:04:27, Peter Kasting wrote: > Nit: Maybe "The synced DSE GUID should be empty until the user selects something or there is sync activity."? Done, thanks. https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:940: // Add third-party default search engine. On 2017/02/02 at 00:04:27, Peter Kasting wrote: > Nit: Blank line above this. > > It also seems a little odd to use "third-party" here, where I assume you just mean "user-provided". ("third-party" sounds to me like something that's sideloaded.) (2 places) Done https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:941: TemplateURL* user_dse = AddKeywordWithDate( On 2017/02/02 at 00:04:27, Peter Kasting wrote: > Nit: Probably not for this CL, but it'd be nice if this function had fewer args overall (I think the last param is never set by any caller?), and more defaulted args, not only so that calls would be shorter, but so I wouldn't have to wonder about whether callsites that pass "UTF-8" vs. "UTF-8;UTF-16" are intentionally picking one vs. the other or just want a "sane default". Agree. https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:968: SetOverridenEngines(); On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: "overridden" has two 'd's (many places) Done https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:975: TemplateURL* dse = model()->GetDefaultSearchProvider(); On 2017/02/02 at 00:04:27, Peter Kasting wrote: > Nit: Can inline below Done https://codereview.chromium.org/2659353002/diff/40001/chrome/browser/search_e... chrome/browser/search_engines/template_url_service_unittest.cc:976: ASSERT_EQ(overriden_engine, dse); On 2017/02/02 at 00:04:26, Peter Kasting wrote: > Nit: Can maybe be EXPECT? Done https://codereview.chromium.org/2659353002/diff/60001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/60001/components/search_engin... components/search_engines/template_url_service.cc:694: ApplyDefaultSearchChange(new_dse, source); On 2017/02/03 at 13:13:57, vasilii wrote: > I can't correlate your changes with https://bugs.chromium.org/p/chromium/issues/detail?id=680197#c24 > You claim that the engine isn't overwritten because MatchesData now returns true. But you won't move to the "else if" block you added in #695. What was happening and what should happen? I object :) My original claim was - "As a result default search changes from RepairPrepopulatedSearchEngines call are not propagated to sync subsystem. So the previous default search selected by user is returned after next sync attempt." I did not claim that engine wasn't overwritten. It was, so default_search_provider_ was equal to new engine chosen from fallback. By propagating to sync subsystem I mean that new engine sync_guid (FROM_FALLBACK) was not written to kSyncedDefaultSearchProviderGUID pref. And it had old value from previously user selected engine and that means that on next sync attempt it was restored. I had added debug log, reverted my fixes and run my added unittest TemplateURLServiceTest.RepairPrepopulatedEnginesUpdatesSyncGuid so you could see the sequence of events, they are all in one callstack from RepairPrepopulatedSearchEngines. You could do the same to check, sequence of calls is rather complicated. [template_url_service.cc(639)] RepairPrepopulatedSearchEngines [template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER [template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER [template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER [default_search_manager.cc(188)] DefaultSearchManager::ClearUserSelectedDefaultSearchEngine [template_url_service.cc(1899)] OnDefaultSearchChange [template_url_service.cc(1927)] ApplyDefaultSearchChangeNoMetrics [template_url_service.cc(1928)] current source = FROM_USER new source FROM_FALLBACK [template_url_service.cc(1944)] current keyword = NULL DEFAULT ENGINE new keyword google.com [template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER [default_search_manager.cc(159)] DefaultSearchManager::SetUserSelectedDefaultSearchEngine new keyword = google.com [template_url_service.cc(1899)] OnDefaultSearchChange [template_url_service.cc(1927)] ApplyDefaultSearchChangeNoMetrics [template_url_service.cc(1928)] current source = FROM_USER new source FROM_USER [template_url_service.cc(1943)] current keyword = google.com new keyword google.com [template_url.cc(1224)] TemplateURL::MatchesData google.com vs google.com, espv vs {google:instantExtendedEnabledKey}, returns false [template_url_service.cc(1653)] UpdateNoNotify current source FROM_USER [default_search_manager.cc(159)] DefaultSearchManager::SetUserSelectedDefaultSearchEngine [default_search_manager.cc(160)] new keyword = google.com [template_url_service.cc(2027)] write to kSyncedDefaultSearchProviderGUID google.com [default_search_manager.cc(159)] DefaultSearchManager::SetUserSelectedDefaultSearchEngine [default_search_manager.cc(160)] new keyword = google.com Where is one call to MatchesData in this callstack and it returns false because its params are default_search_provider_ - TemplateUrl which contains google engine with already substituted search_terms_replacement_key and another is TemplateURLData for google engine So this MatchesData returns false and statements after it performs write of google engine sync_guid to kSyncedDefaultSearchProviderGUID
https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:698: DefaultSearchManager::FROM_FALLBACK) { On 2017/02/03 14:44:25, Alexander Yashkin wrote: > On 2017/02/02 at 00:04:26, Peter Kasting wrote: > > If the old source was FROM_USER, will the new source always be FROM_FALLBACK? > Or can it fall back to extension/policy sometimes? > > > I have kept the old logic here - fallback engine was written to user selected > pref only if transferring FROM_USER -> FROM_FALLBACK > At my current level of understanding, I think from FROM_USER we could only get > to FROM_FALLBACK state. > > Yet, after encountering previous reset engine problem, I am a little bit gunshy > of bold changes :) I guess the question is whether a user choice can override the default engine request of an extension or policy to begin with. If someone installs an extension that sets the DSE, can they override that without uninstalling the extension? If yes, can you test whether, in this case, we reach here with default_search_provider_source_ == FROM_EXTENSION? If so, we need to keep your conditional as you wrote it. (I'm asking because I'd rather not have the code have conditionals that are really always true, as it results in a lot of head-scratching later trying to figure out what's possible.)
On 2017/02/05 at 05:09:58, pkasting wrote: > https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... > File components/search_engines/template_url_service.cc (right): > > https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... > components/search_engines/template_url_service.cc:698: DefaultSearchManager::FROM_FALLBACK) { > On 2017/02/03 14:44:25, Alexander Yashkin wrote: > > On 2017/02/02 at 00:04:26, Peter Kasting wrote: > > > If the old source was FROM_USER, will the new source always be FROM_FALLBACK? > > Or can it fall back to extension/policy sometimes? > > > > > I have kept the old logic here - fallback engine was written to user selected > > pref only if transferring FROM_USER -> FROM_FALLBACK > > At my current level of understanding, I think from FROM_USER we could only get > > to FROM_FALLBACK state. > > > > Yet, after encountering previous reset engine problem, I am a little bit gunshy > > of bold changes :) > > I guess the question is whether a user choice can override the default engine request of an extension or policy to begin with. If someone installs an extension that sets the DSE, can they override that without uninstalling the extension? If yes, can you test whether, in this case, we reach here with default_search_provider_source_ == FROM_EXTENSION? If so, we need to keep your conditional as you wrote it. > > (I'm asking because I'd rather not have the code have conditionals that are really always true, as it results in a lot of head-scratching later trying to figure out what's possible.) I don't think that we can get from FROM_USER to any other choice than FROM_FALLBACK. I could not think of such scenario. If extension overriding DSE is installed user could not override it manually. DefaultSearchManager returns current default engine by choosing first policy if exists, than extension, than user, and fallback. So I don't see how we can get from FROM_USER to extension/policy. I will delete the check for FROM_FALLBACK.
https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/20001/components/search_engin... components/search_engines/template_url_service.cc:698: DefaultSearchManager::FROM_FALLBACK) { On 2017/02/05 at 05:09:58, Peter Kasting wrote: > On 2017/02/03 14:44:25, Alexander Yashkin wrote: > > On 2017/02/02 at 00:04:26, Peter Kasting wrote: > > > If the old source was FROM_USER, will the new source always be FROM_FALLBACK? > > Or can it fall back to extension/policy sometimes? > > > > > I have kept the old logic here - fallback engine was written to user selected > > pref only if transferring FROM_USER -> FROM_FALLBACK > > At my current level of understanding, I think from FROM_USER we could only get > > to FROM_FALLBACK state. > > > > Yet, after encountering previous reset engine problem, I am a little bit gunshy > > of bold changes :) > > I guess the question is whether a user choice can override the default engine request of an extension or policy to begin with. If someone installs an extension that sets the DSE, can they override that without uninstalling the extension? If yes, can you test whether, in this case, we reach here with default_search_provider_source_ == FROM_EXTENSION? If so, we need to keep your conditional as you wrote it. > > (I'm asking because I'd rather not have the code have conditionals that are really always true, as it results in a lot of head-scratching later trying to figure out what's possible.) I don't think that we can get from FROM_USER to any other choice than FROM_FALLBACK. I could not think of such scenario. If extension overriding DSE is installed user could not override it manually. DefaultSearchManager returns current default engine by choosing first policy if exists, than extension, than user, and fallback. So I don't see how we can get from FROM_USER to extension/policy. I will delete the check for FROM_FALLBACK.
https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:685: // If the default search provider came from a user pref we would have been If the reproduction steps are: - set the DSE to Bling - reset the settings then why don't we land in this block? How is it possible to end up here? I'm asking because the current behavior looks to me like this: - default_search_manager_.ClearUserSelectedDefaultSearchEngine does a hell of callbacks and something isn't done right there. - we are trying to patch the flaw by calling SetUserSelectedDefaultSearchEngine again. What is happening if there is an extension overriding the DSE (on top of the reproduction steps)? The control won't reach the new block in #698. However, the syncing DSE should still be Google and not Bling.
https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:685: // If the default search provider came from a user pref we would have been On 2017/02/07 14:45:37, vasilii wrote: > If the reproduction steps are: > - set the DSE to Bling > - reset the settings > then why don't we land in this block? How is it possible to end up here? We can land here only if previous DSE was fallback and new DSE is fallback also. DefaultSearchManager does not notify its observers on change from fallback to fallback, because this means nothing changed. In this case we reset default_search_provider_ at function start and it remains nullptr because DefaultSearchManager did not notify TemplateURLService. You can see details in ClearUserSelectedDefaultSearchEngine and OnDefaultSearchPrefChanged of DefaultSearchManager. This seems a little bit too much knowing of DefaultSearchManager internals here, yet here we are. > > I'm asking because the current behavior looks to me like this: > - default_search_manager_.ClearUserSelectedDefaultSearchEngine does a hell of > callbacks and something isn't done right there. I see it in different way - in old behaviour, default_search_manager_.ClearUserSelectedDefaultSearchEngine call from RepairPrepopulatedSearchEngines called too many callbacks of TemplateURLService only as consequence of two bugs in TemplateURL and in ApplyDefaultSearchChangeNoMetrics. Only by accident it set user selected engine pref and sync guid pref and only for google engine and only when changing from user DSE to fallback. ClearUserSelectedDefaultSearchEngine should do what it was made for - reset user engine and choose DSE according to its rules - fallback engine if no user/extension/policy engine is specified. > - we are trying to patch the flaw by calling SetUserSelectedDefaultSearchEngine > again. > I would say that RepairPrepopulatedEngines is special case of user engine reset - its called by user expicitly and is expected to propagate to sync. See comments in Reset bug: "e.g. if we set 'Show Home button' in preferences, that preference syncs. If we then click 'Reset settings', the 'Show home button' preference is reset to false and the new value is synced to other browsers / devices." So I have added an explicit call to SetUserSelectedDefaultSearchEngine in Repair and only when changing from user to fallback to keep previously implemented behaviour. > What is happening if there is an extension overriding the DSE (on top of the > reproduction steps)? The control won't reach the new block in #698. However, the > syncing DSE should still be Google and not Bling. I tried to minimize impact of my changes and kept previous logic here. I 'll try to check tomorrow how it worked.
https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... components/search_engines/template_url_service.cc:695: } else if (old_source == DefaultSearchManager::FROM_USER) { Your reasoning sounds good. It seems that we should call SetUserSelectedDefaultSearchEngine unconditionally. I don't understand why |old_source| matters here.
On 2017/02/07 at 17:12:56, vasilii wrote: > https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... > File components/search_engines/template_url_service.cc (right): > > https://codereview.chromium.org/2659353002/diff/80001/components/search_engin... > components/search_engines/template_url_service.cc:695: } else if (old_source == DefaultSearchManager::FROM_USER) { > Your reasoning sounds good. It seems that we should call SetUserSelectedDefaultSearchEngine unconditionally. I don't understand why |old_source| matters here. Created https://bugs.chromium.org/p/chromium/issues/detail?id=690345
I think this CL will fix both bugs.
On 2017/02/09 at 14:24:34, vasilii wrote: > I think this CL will fix both bugs. Not in current state. I checked that Reset settings is working as following: As prerequisites to reproduce case below install user selected DSE, and extension DSE afterwards in browser. Reset settings call lead to (implemented in ProfileResetter): 1. RepairPrepopulatedEngines is called and it calls default_search_manager_.ClearUserSelectedDefaultSearchEngine(). If previous default search was extension controlled it remains as default after RepairPrepopulatedEngines. Code block in line 695 will not be hit because old engine was controlled by extension and new is also extension DSE. User engine is removed from pref. 2. ResetExtensions is called, it disables user installed extension, and it will call default_search_manager_.ClearExtensionControlledDefaultSearchEngine which will set current DSE from fallback source. As a solution we could call ResetExtensions before ResetDefaultSearchEngine or change logic inside RepairPrepopulatedEngines to set fallback engine as user selected (and sync to other devices) even when extension is still controlling DSE after Repair. Also where is another very rare case, IMHO, when you set user selected DSE, afterwards domain policy is turned on and DSE will become policy controlled - should reset settings in this case lead to syncing fallback engine to other devices even if DSE is still controlled by policy? I am not even sure if settings sync is enabled when policy is controlling some settings. But still.
Sync will work when there are some policies. Some settings will be controlled on the enterprise machine. However, at home the user may tweak those settings. Thus, we should do the same as for the extension controlled DSE. We shouldn't rely on ResetExtensions. Calling SetUserSelectedDefaultSearchEngine unconditionally (for the prepopulated default search engine) seems to be the right thing. Even if there is an extension/policy and no visible effect on the current machine, the user set GUID will propagate to the sync server.
On 2017/02/09 at 16:06:39, vasilii wrote: > Sync will work when there are some policies. Some settings will be controlled on the enterprise machine. However, at home the user may tweak those settings. Thus, we should do the same as for the extension controlled DSE. > > We shouldn't rely on ResetExtensions. Calling SetUserSelectedDefaultSearchEngine unconditionally (for the prepopulated default search engine) seems to be the right thing. Even if there is an extension/policy and no visible effect on the current machine, the user set GUID will propagate to the sync server. Added fix for reset engines. I hope this will be enough.
Description was changed from ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix. This is reland of https://codereview.chromium.org/2516963002 and https://codereview.chromium.org/2524733008. "Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data." Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines - after RepairPrepopulatedEngines sync guid preference was not updated correctly. R=pkasting@chromium.org, vasilii@chromium.org BUG=686399 TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 is not reproducing. ========== to ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix. This is reland of https://codereview.chromium.org/2516963002 and https://codereview.chromium.org/2524733008. "Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data." Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines - after RepairPrepopulatedEngines sync guid preference was not updated correctly. R=pkasting@chromium.org, vasilii@chromium.org BUG=686399 TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 and https://bugs.chromium.org/p/chromium/issues/detail?id=690345 are fixed. ==========
On 2017/02/10 at 16:34:35, Alexander Yashkin wrote: > On 2017/02/09 at 16:06:39, vasilii wrote: > > Sync will work when there are some policies. Some settings will be controlled on the enterprise machine. However, at home the user may tweak those settings. Thus, we should do the same as for the extension controlled DSE. > > > > We shouldn't rely on ResetExtensions. Calling SetUserSelectedDefaultSearchEngine unconditionally (for the prepopulated default search engine) seems to be the right thing. Even if there is an extension/policy and no visible effect on the current machine, the user set GUID will propagate to the sync server. > > Added fix for reset engines. I hope this will be enough. Peter, Vasilii PTAL
The CQ bit was checked by a-v-y@yandex-team.ru to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:192: // Set custom search engine as default through overrides pref. Does it overwrite the user prefs? https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:306: auto overrides_list = base::MakeUnique<base::ListValue>(); I'd move this definition down to where it's used. https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:977: RepairPrepopulatedEnginesWithOverridesUpdatesSyncGuid) { I fail to understand how this test works and why it passes. SetOverriddenEngines() overwrites the user prefs. Later in SetUserSelectedDefaultSearchProvider() and RepairPrepopulatedSearchEngines() they are overwritten again. How can it be that |overridden_engine| becomes the DSE in #1010? https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.h (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/default_search_manager.h:105: // Returns a pointer to fallback engine. the fallback engine https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:707: fallback_engine->sync_guid()); Shouldn't we set all the user prefs via SetUserSelectedDefaultSearchEngine? Currently you overwrite the GUID but will other DSE properties be synced too?
https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:306: auto overrides_list = base::MakeUnique<base::ListValue>(); On 2017/02/13 14:37:54, vasilii wrote: > I'd move this definition down to where it's used. This, and adjust the other declarations and blank lines so the variable declarations and uses are grouped a bit more: auto entry = ...; entry->...; ... auto overrides_list = ...; overrides_list->...; auto prefs = ...; prefs->...; prefs->SetUserPref(prefs::kSearchProviderOverrides, overrides_list.release()); https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:928: // Test checks that RepairPrepopulatedEngines correctly updates sync guid for Nit: Test checks -> Checks (3 places) https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:943: // Google engine must exist. Rather than looking explicitly for Google, would it work the same to just get the current DSE at this point? That way the test would be robust against forks/l10n changes that made Google not the default engine in some area or distribution. https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:977: RepairPrepopulatedEnginesWithOverridesUpdatesSyncGuid) { On 2017/02/13 14:37:54, vasilii wrote: > I fail to understand how this test works and why it passes. > SetOverriddenEngines() overwrites the user prefs. It sets the kSearchProviderOverrides pref, which the next two things you cite don't touch. Those work on other prefs. https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1035: // Add user provided default search engine. Nit: Blank line above this https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1064: // google. Again, I'd try to write this to check for the prior DSE instead of checking specifically for Google. Also, it's weird that you have a comment here which applies to all the code below, but there are multiple blank lines in the code below. In general, don't have blank lines in any of the code a comment applies to. Put a blank line above the comment, then all the applicable code. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/default_search_manager.cc:158: return fallback_default_search_.get(); Shouldn't this check |g_fallback_search_engines_disabled|? (Then GetDefaultSearchEngine() could call this.) I don't think this will make a practical difference since that variable is only set by tests, and no tests should set it and also exercise the new code you're adding in RepairPrepopulatedSearchEngines(), but it seems more correct. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.h (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/default_search_manager.h:106: TemplateURLData* GetFallbackSearchEngine() const; Nit: const methods should not return non-const pointers; either return a pointer-to-const, or make the method non-const. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:707: fallback_engine->sync_guid()); On 2017/02/13 14:37:54, vasilii wrote: > Shouldn't we set all the user prefs via SetUserSelectedDefaultSearchEngine? > Currently you overwrite the GUID but will other DSE properties be synced too? As the comment notes, that will happen automatically via OnSyncedDefaultSearchProviderGUIDChanged(). However, I was going to ask the opposite -- why can't we just always write the GUID here, instead of only doing it for prepopulated engines? Non-prepopulated engines should have a sync GUID too. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_unittest.cc:1888: // constructor so must be handled specially inside MatchesData. Nit: If this is the specific bit you want to test, why not just test that?: 12345678901234567890123456789012345678901234567890123456789012345678901234567890 // Checks that TemplateURL::MatchesData() correctly handles a search term // replacement key of kGoogleInstantExtendedEnabledKeyFull, which gets // transformed in TemplateURL's constructor. TEST_F(TemplateURLTest, MatchesData) { TemplateURLData data; // Note: This requires exposing this constant in template_url.h for tests data.search_terms_replacement_key = TemplateURL::kGoogleInstantExtendedEnabledKeyFull; TemplateURL url(data); EXPECT_NE(TemplateURL::kGoogleInstantExtendedEnabledKeyFull, url.search_terms_replacement_key()); EXPECT_TRUE(TemplateURL::MatchesData(&url, &data, search_terms_data_)); } https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_unittest.cc:1895: TEST_F(TemplateURLTest, GoogleInstantExtendedEnabledReplacement) { This test feels like a change-detector test: it will break if we change the values of these params (at which point we'll have to fix the test), and it won't actually catch real bugs. I would remove it, unless there's something I'm missing about why it's important to check the specific strings these keys map to.
https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:192: // Set custom search engine as default through overrides pref. On 2017/02/13 at 14:37:54, vasilii wrote: > Does it overwrite the user prefs? Where is user pref kSearchProviderOverrides that can contain set of prepopulated engines to use instead of prepopulated engines hardcoded in chromium in template_url_prepopulate_data.cc and chosen for user country. This mechanism is useful for people who want to create their own chrome distributive with custom set of search engines to be used as default and fallback. No, it does not override user selected DSE pref. Maybe better comment would be "// Set custom search engine as default fallback through overrides pref." I have updated comment. https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:306: auto overrides_list = base::MakeUnique<base::ListValue>(); On 2017/02/13 at 14:37:54, vasilii wrote: > I'd move this definition down to where it's used. Done https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:928: // Test checks that RepairPrepopulatedEngines correctly updates sync guid for On 2017/02/13 at 22:24:57, Peter Kasting wrote: > Nit: Test checks -> Checks (3 places) Done https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:943: // Google engine must exist. On 2017/02/13 at 22:24:57, Peter Kasting wrote: > Rather than looking explicitly for Google, would it work the same to just get the current DSE at this point? > > That way the test would be robust against forks/l10n changes that made Google not the default engine in some area or distribution. Done. https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:977: RepairPrepopulatedEnginesWithOverridesUpdatesSyncGuid) { On 2017/02/13 at 14:37:54, vasilii wrote: > I fail to understand how this test works and why it passes. SetOverriddenEngines() overwrites the user prefs. Later in SetUserSelectedDefaultSearchProvider() and RepairPrepopulatedSearchEngines() they are overwritten again. How can it be that |overridden_engine| becomes the DSE in #1010? I tried to describe how kSearchProviderOverrides works above in my reply to your comment to SetOverridenEngines. https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1035: // Add user provided default search engine. On 2017/02/13 at 22:24:57, Peter Kasting wrote: > Nit: Blank line above this Done https://codereview.chromium.org/2659353002/diff/100001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1064: // google. On 2017/02/13 at 22:24:57, Peter Kasting wrote: > Again, I'd try to write this to check for the prior DSE instead of checking specifically for Google. > > Also, it's weird that you have a comment here which applies to all the code below, but there are multiple blank lines in the code below. In general, don't have blank lines in any of the code a comment applies to. Put a blank line above the comment, then all the applicable code. Fixed, thanks. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/default_search_manager.cc:158: return fallback_default_search_.get(); On 2017/02/13 at 22:24:57, Peter Kasting wrote: > Shouldn't this check |g_fallback_search_engines_disabled|? (Then GetDefaultSearchEngine() could call this.) > > I don't think this will make a practical difference since that variable is only set by tests, and no tests should set it and also exercise the new code you're adding in RepairPrepopulatedSearchEngines(), but it seems more correct. Done https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/default_search_manager.h (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/default_search_manager.h:105: // Returns a pointer to fallback engine. On 2017/02/13 at 14:37:54, vasilii wrote: > the fallback engine Done https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/default_search_manager.h:106: TemplateURLData* GetFallbackSearchEngine() const; On 2017/02/13 at 22:24:57, Peter Kasting wrote: > Nit: const methods should not return non-const pointers; either return a pointer-to-const, or make the method non-const. Done https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:707: fallback_engine->sync_guid()); On 2017/02/13 at 22:24:57, Peter Kasting wrote: > On 2017/02/13 14:37:54, vasilii wrote: > > Shouldn't we set all the user prefs via SetUserSelectedDefaultSearchEngine? > > Currently you overwrite the GUID but will other DSE properties be synced too? > > As the comment notes, that will happen automatically via OnSyncedDefaultSearchProviderGUIDChanged(). > > However, I was going to ask the opposite -- why can't we just always write the GUID here, instead of only doing it for prepopulated engines? Non-prepopulated engines should have a sync GUID too. Yes, that was my first solution, yet luckily one of my added test started to fail and I discovered, that sync_guid in fallback_default_search_(TemplateURLData structure) stored in DefaultSearchManager does not correspond to any TemplateURL stored in TemplateURLService. sync_guid field is automatically generated in TemplateURLData constructor. DefaultSearchManager creates its fallback_default_search_ structure calling GetPrepopulatedDefaultSearch. It creates TemplateURLData with new sync_guid on every browser start. TemplateURLs stored in TemplateURLService are loaded from keywords DB and have different sync_guids that are initialized once at keywords DB creation somewhere in profile creation. I think that looking for prepopulated_id will work in any scenario, especially after engines are repaired. Yet I have added write to SetUserSelectedDefaultSearchEngine if I am wrong and search for prepopulated_id will sometimes fail. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_unittest.cc:1888: // constructor so must be handled specially inside MatchesData. On 2017/02/13 at 22:24:57, Peter Kasting wrote: > Nit: If this is the specific bit you want to test, why not just test that?: > > 12345678901234567890123456789012345678901234567890123456789012345678901234567890 ???? Are this digits relevant to your comment? > // Checks that TemplateURL::MatchesData() correctly handles a search term > // replacement key of kGoogleInstantExtendedEnabledKeyFull, which gets > // transformed in TemplateURL's constructor. > TEST_F(TemplateURLTest, MatchesData) { > TemplateURLData data; > // Note: This requires exposing this constant in template_url.h for tests > data.search_terms_replacement_key = > TemplateURL::kGoogleInstantExtendedEnabledKeyFull; > TemplateURL url(data); > EXPECT_NE(TemplateURL::kGoogleInstantExtendedEnabledKeyFull, > url.search_terms_replacement_key()); > EXPECT_TRUE(TemplateURL::MatchesData(&url, &data, search_terms_data_)); > } Initially I wanted to check matching of all fields for google TemplateURLData in case of any other strange substitutions in constructor in feature. But since probability of shooting the same leg is small, I do not object, Done. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_unittest.cc:1895: TEST_F(TemplateURLTest, GoogleInstantExtendedEnabledReplacement) { On 2017/02/13 at 22:24:57, Peter Kasting wrote: > This test feels like a change-detector test: it will break if we change the values of these params (at which point we'll have to fix the test), and it won't actually catch real bugs. > > I would remove it, unless there's something I'm missing about why it's important to check the specific strings these keys map to. I have added it after your comment in https://codereview.chromium.org/2524733008: "Honestly I can't conceive mentally of which thing should be used when. I'm going to assume you checked carefully and did the right thing in each place. Can there be a test that would have caught this bug, so we don't make the same mistake again later?"
https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:707: fallback_engine->sync_guid()); On 2017/02/14 12:04:43, Alexander Yashkin wrote: > On 2017/02/13 at 22:24:57, Peter Kasting wrote: > > On 2017/02/13 14:37:54, vasilii wrote: > > > Shouldn't we set all the user prefs via SetUserSelectedDefaultSearchEngine? > > > Currently you overwrite the GUID but will other DSE properties be synced > too? > > > > As the comment notes, that will happen automatically via > OnSyncedDefaultSearchProviderGUIDChanged(). > > > > However, I was going to ask the opposite -- why can't we just always write the > GUID here, instead of only doing it for prepopulated engines? Non-prepopulated > engines should have a sync GUID too. > > Yes, that was my first solution, yet luckily one of my added test started to > fail and I discovered, that sync_guid in > fallback_default_search_(TemplateURLData structure) stored in > DefaultSearchManager does not correspond to any TemplateURL stored in > TemplateURLService. > > sync_guid field is automatically generated in TemplateURLData constructor. > DefaultSearchManager creates its fallback_default_search_ structure calling > GetPrepopulatedDefaultSearch. It creates TemplateURLData with new sync_guid on > every browser start. > > TemplateURLs stored in TemplateURLService are loaded from keywords DB and have > different sync_guids that are initialized once at keywords DB creation somewhere > in profile creation. This seems worrisome. Does this mean that if I reset, then I'll sync up an engine which is identical to some existing one in every way except it has a different GUID? Will this lead to the issue we've seen in the past, with duplicate search engine entries (which then get deduped by appending "_" to the keyword) proliferating? Or will the sync system figure this out and delete one, telling all clients to update to the other? > I think that looking for prepopulated_id will work in any scenario, especially > after engines are repaired. I think so too. Presumably the repair will have installed the prepopulated(/override) engines if they were missing, and overrides are required to set an ID, so this should always succeed. I think we should DCHECK that the fallback engine is non-null and eliminate the second conditional arm. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_unittest.cc:1888: // constructor so must be handled specially inside MatchesData. On 2017/02/14 12:04:43, Alexander Yashkin wrote: > On 2017/02/13 at 22:24:57, Peter Kasting wrote: > > Nit: If this is the specific bit you want to test, why not just test that?: > > > > > 12345678901234567890123456789012345678901234567890123456789012345678901234567890 > ???? > Are this digits relevant to your comment? Sorry, that's how I mark out 80 columns so I can ensure code samples I write in here can be used. I forgot to delete it. https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_unittest.cc:1895: TEST_F(TemplateURLTest, GoogleInstantExtendedEnabledReplacement) { On 2017/02/14 12:04:43, Alexander Yashkin wrote: > On 2017/02/13 at 22:24:57, Peter Kasting wrote: > > This test feels like a change-detector test: it will break if we change the > values of these params (at which point we'll have to fix the test), and it won't > actually catch real bugs. > > > > I would remove it, unless there's something I'm missing about why it's > important to check the specific strings these keys map to. > > I have added it after your comment in > https://codereview.chromium.org/2524733008: > > "Honestly I can't conceive mentally of which thing should be used when. I'm > going to assume you checked carefully and did the right thing in each place. > Can there be a test that would have caught this bug, so we don't make the same > mistake again later?" Fair enough. https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:314: Nit: Up to you, but I wouldn't add this, because |alternate_urls| really just exists as part of populating |entry|. https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1037: // Get initial DSE to check later its guid. Nit: later its guid -> its guid later https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... File components/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... components/search_engines/template_url_unittest.cc:1893: "?{google:instantExtendedEnabledKey}&q={searchTerms}"); Nit: I would trim the google:baseURL bit out of this and replace it with something static. No need to test that, and it could make the test flaky if the base URL is computed differently in another locale. (But see also comment below, which would moot that second concern a bit.) I would probably also string-concatenate in kGoogleInstantExtendedEnabledKeyFull here instead of retyping it. https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... components/search_engines/template_url_unittest.cc:1908: search_generated.spec()); Rather than string-match the spec, What about using a net::QueryIterator to grab the first query param off the GURL and checking that the key is google_util::kInstantExtendedAPIParam? This seems more targeted and less potentially flaky to e.g. changes in the default query string.
https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/100001/components/search_engi... components/search_engines/template_url_service.cc:707: fallback_engine->sync_guid()); On 2017/02/15 at 02:09:57, Peter Kasting wrote: > On 2017/02/14 12:04:43, Alexander Yashkin wrote: > > On 2017/02/13 at 22:24:57, Peter Kasting wrote: > > > On 2017/02/13 14:37:54, vasilii wrote: > > > > Shouldn't we set all the user prefs via SetUserSelectedDefaultSearchEngine? > > > > Currently you overwrite the GUID but will other DSE properties be synced > > too? > > > > > > As the comment notes, that will happen automatically via > > OnSyncedDefaultSearchProviderGUIDChanged(). > > > > > > However, I was going to ask the opposite -- why can't we just always write the > > GUID here, instead of only doing it for prepopulated engines? Non-prepopulated > > engines should have a sync GUID too. > > > > Yes, that was my first solution, yet luckily one of my added test started to > > fail and I discovered, that sync_guid in > > fallback_default_search_(TemplateURLData structure) stored in > > DefaultSearchManager does not correspond to any TemplateURL stored in > > TemplateURLService. > > > > sync_guid field is automatically generated in TemplateURLData constructor. > > DefaultSearchManager creates its fallback_default_search_ structure calling > > GetPrepopulatedDefaultSearch. It creates TemplateURLData with new sync_guid on > > every browser start. > > > > TemplateURLs stored in TemplateURLService are loaded from keywords DB and have > > different sync_guids that are initialized once at keywords DB creation somewhere > > in profile creation. > > This seems worrisome. Does this mean that if I reset, then I'll sync up an engine which is identical to some existing one in every way except it has a different GUID? Will this lead to the issue we've seen in the past, with duplicate search engine entries (which then get deduped by appending "_" to the keyword) proliferating? Or will the sync system figure this out and delete one, telling all clients to update to the other? I think this issue with sync is taken care of - see UpdateTemplateURLIfPrepopulated. If new engine will come from sync that has same prepopulated_id as local engine, local engine sync_guid will be updated. > > > I think that looking for prepopulated_id will work in any scenario, especially > > after engines are repaired. > > I think so too. Presumably the repair will have installed the prepopulated(/override) engines if they were missing, and overrides are required to set an ID, so this should always succeed. I think we should DCHECK that the fallback engine is non-null and eliminate the second conditional arm. Done. https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:314: On 2017/02/15 at 02:09:57, Peter Kasting wrote: > Nit: Up to you, but I wouldn't add this, because |alternate_urls| really just exists as part of populating |entry|. Removed, thanks. https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:1037: // Get initial DSE to check later its guid. On 2017/02/15 at 02:09:57, Peter Kasting wrote: > Nit: later its guid -> its guid later Done https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... File components/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... components/search_engines/template_url_unittest.cc:1893: "?{google:instantExtendedEnabledKey}&q={searchTerms}"); On 2017/02/15 02:09:57, Peter Kasting wrote: > Nit: I would trim the google:baseURL bit out of this and replace it with > something static. No need to test that, and it could make the test flaky if the > base URL is computed differently in another locale. (But see also comment > below, which would moot that second concern a bit.) > > I would probably also string-concatenate in kGoogleInstantExtendedEnabledKeyFull > here instead of retyping it. Done https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... components/search_engines/template_url_unittest.cc:1908: search_generated.spec()); On 2017/02/15 at 02:09:57, Peter Kasting wrote: > Rather than string-match the spec, What about using a net::QueryIterator to grab the first query param off the GURL and checking that the key is google_util::kInstantExtendedAPIParam? > > This seems more targeted and less potentially flaky to e.g. changes in the default query string. Done
LGTM https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... File chrome/browser/search_engines/template_url_service_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/120001/chrome/browser/search_... chrome/browser/search_engines/template_url_service_unittest.cc:314: On 2017/02/15 12:46:10, Alexander Yashkin wrote: > On 2017/02/15 at 02:09:57, Peter Kasting wrote: > > Nit: Up to you, but I wouldn't add this, because |alternate_urls| really just > exists as part of populating |entry|. > > Removed, thanks. Oh, I meant the blank line -- but if tests don't need the functionality, sure, kill it. In general, I would remove anything in here your tests don't actually rely on. Having extra stuff just obscures what you're testing. https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... File components/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... components/search_engines/template_url_unittest.cc:1893: "?{google:instantExtendedEnabledKey}&q={searchTerms}"); On 2017/02/15 12:46:10, Alexander Yashkin wrote: > On 2017/02/15 02:09:57, Peter Kasting wrote: > > Nit: I would trim the google:baseURL bit out of this and replace it with > > something static. No need to test that, and it could make the test flaky if > the > > base URL is computed differently in another locale. (But see also comment > > below, which would moot that second concern a bit.) > > > > I would probably also string-concatenate in > kGoogleInstantExtendedEnabledKeyFull > > here instead of retyping it. > > Done Looks like google:baseURL is still around and could be replaced. https://codereview.chromium.org/2659353002/diff/140001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:702: // We expect that fallback engine always exists after repair. Nit: Maybe: "The fallback engine is created from built-in/override data that should always have a prepopulate ID, so this engine should always exist after a repair." https://codereview.chromium.org/2659353002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:706: // OnSyncedDefaultSearchProviderGUIDChanged. Nit: Maybe: "Write the fallback engine's GUID to prefs, which will cause OnSyncedDefaultSearchProviderGUIDChanged() to set it as the new user-selected engine."
https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... File components/search_engines/template_url_unittest.cc (right): https://codereview.chromium.org/2659353002/diff/120001/components/search_engi... components/search_engines/template_url_unittest.cc:1893: "?{google:instantExtendedEnabledKey}&q={searchTerms}"); On 2017/02/16 at 00:12:10, Peter Kasting wrote: > On 2017/02/15 12:46:10, Alexander Yashkin wrote: > > On 2017/02/15 02:09:57, Peter Kasting wrote: > > > Nit: I would trim the google:baseURL bit out of this and replace it with > > > something static. No need to test that, and it could make the test flaky if > > the > > > base URL is computed differently in another locale. (But see also comment > > > below, which would moot that second concern a bit.) > > > > > > I would probably also string-concatenate in > > kGoogleInstantExtendedEnabledKeyFull > > > here instead of retyping it. > > > > Done > > Looks like google:baseURL is still around and could be replaced. Sorry, missed that, replaced with google.com. https://codereview.chromium.org/2659353002/diff/140001/components/search_engi... File components/search_engines/template_url_service.cc (right): https://codereview.chromium.org/2659353002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:702: // We expect that fallback engine always exists after repair. On 2017/02/16 at 00:12:11, Peter Kasting wrote: > Nit: Maybe: "The fallback engine is created from built-in/override data that should always have a prepopulate ID, so this engine should always exist after a repair." Thanks, done. https://codereview.chromium.org/2659353002/diff/140001/components/search_engi... components/search_engines/template_url_service.cc:706: // OnSyncedDefaultSearchProviderGUIDChanged. On 2017/02/16 at 00:12:11, Peter Kasting wrote: > Nit: Maybe: "Write the fallback engine's GUID to prefs, which will cause OnSyncedDefaultSearchProviderGUIDChanged() to set it as the new user-selected engine." Thanks done.
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2659353002/#ps160001 (title: "Fixed after review, round 6")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by a-v-y@yandex-team.ru
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2659353002/#ps180001 (title: "Fixed deps")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by a-v-y@yandex-team.ru
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1487243318430980,
"parent_rev": "ee70c9cdc585a45d3ba60b6dfcc74e79e99aab70", "commit_rev":
"02d3f50b4f9429a499eb1604bf315f0ce03163a1"}
Message was sent while issue was closed.
Description was changed from ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix. This is reland of https://codereview.chromium.org/2516963002 and https://codereview.chromium.org/2524733008. "Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data." Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines - after RepairPrepopulatedEngines sync guid preference was not updated correctly. R=pkasting@chromium.org, vasilii@chromium.org BUG=686399 TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 and https://bugs.chromium.org/p/chromium/issues/detail?id=690345 are fixed. ========== to ========== Fix TemplateUrl::MatchesData comparison of search_terms_replacement_key (reland) Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines which started to reproduce after first fix. This is reland of https://codereview.chromium.org/2516963002 and https://codereview.chromium.org/2524733008. "Current TemplateUrl constructor performs replacement of search_terms_replacement_key field after initialization from TemplateURLData. This replacement was not handled correctly in TemplateUrl::MatchesData function. As result, TemplateUrl created from TemplateUrlData for google engine could not be matched to its own source data." Also fixed linked bug in TemplateURLService::RepairPrepopulatedEngines - after RepairPrepopulatedEngines sync guid preference was not updated correctly. R=pkasting@chromium.org, vasilii@chromium.org BUG=686399 TEST=Ensure that https://bugs.chromium.org/p/chromium/issues/detail?id=680197 and https://bugs.chromium.org/p/chromium/issues/detail?id=690345 are fixed. Review-Url: https://codereview.chromium.org/2659353002 Cr-Commit-Position: refs/heads/master@{#450937} Committed: https://chromium.googlesource.com/chromium/src/+/02d3f50b4f9429a499eb1604bf31... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001) as https://chromium.googlesource.com/chromium/src/+/02d3f50b4f9429a499eb1604bf31... |
