chrome/browser/search_engines/template_url_service_unittest.cc - Issue 2806593006: Changed GenerateKeyword to always return keyword in lowercase

Unified Diff: chrome/browser/search_engines/template_url_service_unittest.cc

Issue 2806593006: Changed GenerateKeyword to always return keyword in lowercase (Closed)

Patch Set: Created 3 years, 8 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Index: chrome/browser/search_engines/template_url_service_unittest.cc

diff --git a/chrome/browser/search_engines/template_url_service_unittest.cc b/chrome/browser/search_engines/template_url_service_unittest.cc

index c3b8de661f0c6b8c009edbf0d440e4c34cd821b0..19ba69eb95cc8cbe26bd9351bf8eea957fbf19cc 100644

--- a/chrome/browser/search_engines/template_url_service_unittest.cc

+++ b/chrome/browser/search_engines/template_url_service_unittest.cc

@@ -1793,6 +1793,28 @@ TEST_F(TemplateURLServiceTest, CheckNonreplaceableEnginesKeywordsConflicts) {

EXPECT_EQ(kCommonKeyword, user4->keyword());

EXPECT_EQ(user4, model()->GetTemplateURLForKeyword(kCommonKeyword));

EXPECT_EQ(user4, model()->GetTemplateURLForHost("test3"));

+ // Check conflict between search engines with html tags embedded in URL host.

+ // Where was problem with UniquifyKeyword incorrectly handling them because

+ // generated keyword for such URL contained upper case characters.

Peter Kasting 2017/04/18 18:46:19 Nit: Avoid describing past problems (because it's

Alexander Yashkin 2017/04/20 07:05:53 Thanks, done.

+ const TemplateURL* user5 =

+ AddKeywordWithDate("nonreplaceable5", "embedded.%3chtml%3eweb",

+ "http://embedded.<html>web/?q={searchTerms}",

+ std::string(), std::string(), std::string(), false);

+ EXPECT_EQ(ASCIIToUTF16("embedded.%3chtml%3eweb"), user5->keyword());

+ EXPECT_EQ(user5, model()->GetTemplateURLForKeyword(

+ ASCIIToUTF16("embedded.%3chtml%3eweb")));

+ const TemplateURL* user6 =

+ AddKeywordWithDate("nonreplaceable6", "embedded.%3chtml%3eweb",

+ "http://embedded.<html>web/?q={searchTerms}",

+ std::string(), std::string(), std::string(), false);

+ EXPECT_EQ(ASCIIToUTF16("embedded.%3chtml%3eweb"), user6->keyword());

+ EXPECT_EQ(user6, model()->GetTemplateURLForKeyword(

+ ASCIIToUTF16("embedded.%3chtml%3eweb")));

+ // Expect existing engine changed its keyword.

+ EXPECT_EQ(ASCIIToUTF16("embedded.%3chtml%3eweb_"), user5->keyword());

+ EXPECT_EQ(user5, model()->GetTemplateURLForKeyword(

+ ASCIIToUTF16("embedded.%3chtml%3eweb_")));

Peter Kasting 2017/04/18 18:46:20 Would this test have failed without this patch? W

Alexander Yashkin 2017/04/20 07:05:53 This tests fails without my patch - first at diffe

This tests fails without my patch - first at different DHCECKs, and if disabling them, it fails in checking expected keywords: Note: Google Test filter = TemplateURLServiceTest.CheckNonreplaceableEnginesKeywordsConflicts [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from TemplateURLServiceTest [ RUN ] TemplateURLServiceTest.CheckNonreplaceableEnginesKeywordsConflicts ../../chrome/browser/search_engines/template_url_service_unittest.cc:1814: Failure Value of: model()->GetTemplateURLForKeyword( ASCIIToUTF16("embedded.%3chtml%3eweb")) Actual: 0x7f81cd02dc00 Expected: user6 Which is: 0x7f81cd02e600 ../../chrome/browser/search_engines/template_url_service_unittest.cc:1816: Failure Value of: user5->keyword() Actual: embedded.%3chtml%3eweb Expected: ASCIIToUTF16("embedded.%3chtml%3eweb_") Which is: embedded.%3chtml%3eweb_ ../../chrome/browser/search_engines/template_url_service_unittest.cc:1818: Failure Value of: model()->GetTemplateURLForKeyword( ASCIIToUTF16("embedded.%3chtml%3eweb_")) Actual: NULL Expected: user5 Which is: 0x7f81cd02dc00 What happens without my patch: 1. Engine "nonreplaceable5" is added with keyword "embedded.%3chtml%3eweb" 2. Attempt to add engine (using AddNoNotify) "nonreplaceable6" with same keyword "embedded.%3chtml%3eweb" leads to call to UniquifyKeyword for existing engine "nonreplaceable5". UniquifyKeyword returns keyword autogenerated from engine URL (using GetKeyword()) - "embedded.%3Chtml%3Eweb"(contains uppercased characters). While generating keywords UniquifyKeyword checks that TemplateURLService does not contain engine with keyword "embedded.%3Chtml%3Eweb" and it succeeds. All keywords inside TemplateURLService are lowercased. 3. After receiving new keyword (with uppercase chars) from UniquifyKeyword AddNoNotify function tries to update engine "nonreplaceable5" with new keyword calling ResetTemplateURLNoNotify. 4. ResetTemplateURLNoNotify creates TemplateURLData with new values, and inside TemplateURLData::SetKeyword() keyword "embedded.%3Chtml%3Eweb" is converted to lower case. After that ResetTemplateURLNoNotify calls UpdateNoNotify for "nonreplaceable5" with lowercased keyword "embedded.%3ehtml%3eweb". 5. So after call to AddNoNotify "nonreplaceable5" engine have same keyword as before and new engine "nonreplaceable6" is also added to TemplateURLService with same keyword "embedded.%3ehtml%3eweb". So we have two normal search engines inside TemplateURLService with same keyword after AddNoNotify. Which is not intended result, IMHO.

}

TEST_F(TemplateURLServiceTest, CheckReplaceableEnginesKeywordsConflicts) {

« no previous file with comments | « no previous file | components/search_engines/template_url.cc » ('j') | components/search_engines/template_url_unittest.cc » ('J')