[Omnibox] Changing scheme from https:// to http:// results in DCHECK (Always).

chrome/browser/autocomplete/in_memory_url_index_unittest.cc

Index: chrome/browser/autocomplete/in_memory_url_index_unittest.cc
diff --git a/chrome/browser/autocomplete/in_memory_url_index_unittest.cc b/chrome/browser/autocomplete/in_memory_url_index_unittest.cc
index 5c68b4074e9bad4427cd9b4c86c10ffc85a2b8dc..26654b4486c3fe9bdaf0fca970c82f5130d699a5 100644
--- a/chrome/browser/autocomplete/in_memory_url_index_unittest.cc
+++ b/chrome/browser/autocomplete/in_memory_url_index_unittest.cc
@@ -9,6 +9,7 @@
 #include "base/files/file_path.h"
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
+#include "base/i18n/case_conversion.h"
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/strings/string16.h"
@@ -1193,6 +1194,86 @@ TEST_F(InMemoryURLIndexTest, MAYBE_RebuildFromHistoryIfCacheOld) {
   ExpectPrivateDataEqual(*old_data.get(), new_data);
 }
 
+// Helper function to get lower case |lower_string| and |lower_terms| (words

[Mark P, 2015/06/08 18:13:16]

get -> set

[Pritam Nikam, 2015/06/09 10:03:53]

Done.

+// list) based on supplied |search_string| and |cursor_position|.

[Mark P, 2015/06/08 18:13:16]

It's a bit confusing that we add a space in order to get |lower_string|.  Can
you perhaps add a comment here, something like the one in
url_index_private_data.cc:
   // If cursor position is set and useful (not at either end of the
  // string), allow the search string to be broken at cursor position.
  // We do this by pretending there's a space where the cursor is.
Also, it would help to explain that lower_terms is obtained by splitting
lower_string on whitespace.

[Pritam Nikam, 2015/06/09 10:03:52]

Done.

+void StringToTerms(const char* search_string,

[Mark P, 2015/06/08 18:13:16]

nit: Please move this helper function into the anonymous namespace at the top of
this file.

[Pritam Nikam, 2015/06/09 10:03:52]

Done.

+                   size_t cursor_position,
+                   base::string16* lower_string,
+                   String16Vector* lower_terms) {
+  *lower_string = base::i18n::ToLower(ASCIIToUTF16(search_string));
+  if ((cursor_position != base::string16::npos) &&
+      (cursor_position < lower_string->length()) && (cursor_position > 0)) {
+    lower_string->insert(cursor_position, base::ASCIIToUTF16(" "));
+  }
+
+  Tokenize(*lower_string, base::kWhitespaceUTF16, lower_terms);
+}
+
+TEST_F(InMemoryURLIndexTest, AddHistoryMatch) {
+  const struct {
+    const char* search_string;
+    size_t cursor_position;
+    const std::vector<size_t> expected_word_starts_offsets;
+  } test_cases[] = {
+      /* No punctuations, only cursor position change. */

[Mark P, 2015/06/08 18:13:16]

style nit: you have indented two space too many here and below

[Pritam Nikam, 2015/06/09 10:03:53]

Done.

+      {"ABCD", base::string16::npos, {0}},

[Mark P, 2015/06/08 18:13:16]

style nit: one space inside { before " and one space before the final }

[Pritam Nikam, 2015/06/09 10:03:52]

Done.

+      {"abcd", 0, {0}},

[Mark P, 2015/06/08 18:13:16]

optional nit: align the { in the expected_word_starts_offsets lists so it's easy
to read each line
(and also align the cursor_position field.  This is naturally mostly aligned
here because your test strings are usually the same length, but not always.)

[Pritam Nikam, 2015/06/09 10:03:52]

Done.

+      {"AbcD", 1, {0, 0}},
+      {"abcd", 4, {0}},
+
+      /* Staring with punctuation. */

[Mark P, 2015/06/08 18:13:16]

Staring -> Starting

[Pritam Nikam, 2015/06/09 10:03:52]

Done.

+      {".abcd", base::string16::npos, {1}},
+      {".abcd", 0, {1}},
+      {"!abcd", 1, {1, 0}},
+      {"::abcd", 1, {1, 1}},
+      {":abcd", 5, {1}},
+
+      /* Ending with punctuation. */
+      {"abcd://", base::string16::npos, {0}},
+      {"ABCD://", 0, {0}},
+      {"abcd://", 1, {0, 0}},
+      {"abcd://", 4, {0, 3}},
+      {"abcd://", 7, {0}},
+
+      /* Punctuation in the middle. */
+      {"ab.cd", base::string16::npos, {0}},
+      {"ab.cd", 0, {0}},
+      {"ab!cd", 1, {0, 0}},
+      {"AB.cd", 2, {0, 1}},
+      {"ab:cd", 5, {0}},

[Mark P, 2015/06/08 18:13:16]

Can you add a cursor_position = 3 test in this block too?

[Pritam Nikam, 2015/06/09 10:03:52]

Done.

+
+      /* Hyphenation */
+      {"Ab-cd", base::string16::npos, {0}},
+      {"ab-cd", 0, {0}},
+      {"-abcd", 0, {1}},
+      {"-abcd", 1, {1, 0}},
+      {"abcd-", 2, {0, 0}},
+      {"abcd-", 4, {0, 1}},
+      {"ab-cd", 5, {0}},
+  };
+
+  for (size_t i = 0; i < arraysize(test_cases); ++i) {
+    SCOPED_TRACE(testing::Message()
+                 << "search_string = " << test_cases[i].search_string
+                 << ", cursor_position = " << test_cases[i].cursor_position);
+
+    base::string16 lower_string;
+    String16Vector lower_terms;
+    StringToTerms(test_cases[i].search_string, test_cases[i].cursor_position,
+                  &lower_string, &lower_terms);
+
+    if (!lower_terms.empty()) {

[Mark P, 2015/06/08 18:13:16]

You shouldn't guard this code in this way.  For instance, suppose the _test_
erroneously returned lower_terms.size() == 0 always.  Then we'd claim that
AddHistoryMatch() is functioning correctly, even though never actually made any
calls to AddHistoryMatch.

I don't think you need the guard at all in this test.  Do you?  If not, please
remove it.

[Pritam Nikam, 2015/06/09 10:03:52]

Done.

+      URLIndexPrivateData::AddHistoryMatch match(
+          nullptr, *GetPrivateData(), kTestLanguages, lower_string, lower_terms,
+          base::Time::Now());
+
+      // Verify against expectations.
+      EXPECT_EQ(test_cases[i].expected_word_starts_offsets,
+                match.lower_terms_to_word_starts_offsets_);
+    }
+  }
+}
+
 class InMemoryURLIndexCacheTest : public testing::Test {
  public:
   InMemoryURLIndexCacheTest() {}