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

Side by Side 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:

OLD	NEW
1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.	1 // Copyright (c) 2012 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "components/search_engines/template_url_service.h"	5 #include "components/search_engines/template_url_service.h"

6	6

7 #include <stddef.h>	7 #include <stddef.h>

8	8

9 #include <memory>	9 #include <memory>

10 #include <utility>	10 #include <utility>

(...skipping 1775 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1786 // Existing conflicting engine user3 will get keyword generated by appending	1786 // Existing conflicting engine user3 will get keyword generated by appending

1787 // '_' to its keyword because it can not get keyword autogenerated from search	1787 // '_' to its keyword because it can not get keyword autogenerated from search

1788 // url - "test2" keyword is already in use by user2 engine. Both engines must	1788 // url - "test2" keyword is already in use by user2 engine. Both engines must

1789 // be acessible.	1789 // be acessible.

1790 EXPECT_EQ(ASCIIToUTF16("common_keyword_"), user3->keyword());	1790 EXPECT_EQ(ASCIIToUTF16("common_keyword_"), user3->keyword());

1791 EXPECT_EQ(user3,	1791 EXPECT_EQ(user3,

1792 model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword_")));	1792 model()->GetTemplateURLForKeyword(ASCIIToUTF16("common_keyword_")));

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

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

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

	1796

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

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

	1799 // 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 Nit: Avoid describing past problems (because it's not clear when this refers to), and if you need to motivate something, just describe potential problems (e.g. "URLs with embedded HTML canonicalize to contain uppercase characters in the hostname. Ensure these URLs are still handled correctly for conflict resolution."). Alexander Yashkin 2017/04/20 07:05:53 Thanks, done. Show quoted text On 2017/04/18 at 18:46:19, Peter Kasting wrote: > Nit: Avoid describing past problems (because it's not clear when this refers to), and if you need to motivate something, just describe potential problems (e.g. "URLs with embedded HTML canonicalize to contain uppercase characters in the hostname. Ensure these URLs are still handled correctly for conflict resolution."). Thanks, done.
	1800 const TemplateURL* user5 =

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

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

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

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

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

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

	1807 const TemplateURL* user6 =

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

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

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

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

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

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

	1814 // Expect existing engine changed its keyword.

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

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

	1817 ASCIIToUTF16("embedded.%3chtml%3eweb_")));
	Peter Kasting 2017/04/18 18:46:20 Would this test have failed without this patch? W Would this test have failed without this patch? Wouldn't both keywords be uppercase, and thus still match each other, so the test would still pass? Alexander Yashkin 2017/04/20 07:05:53 This tests fails without my patch - first at diffe Show quoted text On 2017/04/18 at 18:46:20, Peter Kasting wrote: > Would this test have failed without this patch? Wouldn't both keywords be uppercase, and thus still match each other, so the test would still pass? 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.
1796 }	1818 }

1797	1819

1798 TEST_F(TemplateURLServiceTest, CheckReplaceableEnginesKeywordsConflicts) {	1820 TEST_F(TemplateURLServiceTest, CheckReplaceableEnginesKeywordsConflicts) {

1799 test_util()->VerifyLoad();	1821 test_util()->VerifyLoad();

1800	1822

1801 const base::string16 kCommonKeyword = ASCIIToUTF16("common_keyword");	1823 const base::string16 kCommonKeyword = ASCIIToUTF16("common_keyword");

1802 // 1. Add non replaceable user engine with common keyword.	1824 // 1. Add non replaceable user engine with common keyword.

1803 const TemplateURL* user1 =	1825 const TemplateURL* user1 =

1804 AddKeywordWithDate("nonreplaceable", "common_keyword", "http://test1",	1826 AddKeywordWithDate("nonreplaceable", "common_keyword", "http://test1",

1805 std::string(), std::string(), std::string(), false);	1827 std::string(), std::string(), std::string(), false);

(...skipping 131 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
1937 TemplateURL* update_url =	1959 TemplateURL* update_url =

1938 model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));	1960 model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));

1939 const Time update_last_modified = update_url->last_modified();	1961 const Time update_last_modified = update_url->last_modified();

1940 model()->SetUserSelectedDefaultSearchProvider(update_url);	1962 model()->SetUserSelectedDefaultSearchProvider(update_url);

1941 TemplateURL* reloaded_url =	1963 TemplateURL* reloaded_url =

1942 model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));	1964 model()->GetTemplateURLForKeyword(ASCIIToUTF16("engine_keyword"));

1943 const Time reloaded_last_modified = reloaded_url->last_modified();	1965 const Time reloaded_last_modified = reloaded_url->last_modified();

1944 EXPECT_NE(original_last_modified, reloaded_last_modified);	1966 EXPECT_NE(original_last_modified, reloaded_last_modified);

1945 EXPECT_EQ(update_last_modified, reloaded_last_modified);	1967 EXPECT_EQ(update_last_modified, reloaded_last_modified);

1946 }	1968 }

OLD	NEW

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