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

chrome/browser/autocomplete/in_memory_url_index_unittest.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 #include <fstream>
 
 #include "base/auto_reset.h"
 #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"
 #include "base/strings/string_util.h"
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/autocomplete/in_memory_url_index.h"
 #include "chrome/browser/autocomplete/in_memory_url_index_types.h"
 #include "chrome/browser/autocomplete/url_index_private_data.h"
 #include "chrome/browser/bookmarks/bookmark_model_factory.h"
 #include "chrome/browser/history/history_service_factory.h"
@@ skipping 1164 matching lines @@
   // Make sure the data we have was rebuilt from history.  (Version 0
   // means rebuilt from history; anything else means restored from
   // a cache version.)
   EXPECT_EQ(0, new_data.restored_cache_version_);
   EXPECT_NE(fake_rebuild_time, new_data.last_time_rebuilt_from_history_);
 
   // Compare the captured and restored for equality.
   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() {}
 
  protected:
   void SetUp() override;
   void TearDown() override;
 
   // Pass-through functions to simplify our friendship with InMemoryURLIndex.
   void set_history_dir(const base::FilePath& dir_path);
@@ skipping 36 matching lines @@
   ASSERT_TRUE(GetCacheFilePath(&full_file_path));
   std::vector<base::FilePath::StringType> actual_parts;
   full_file_path.GetComponents(&actual_parts);
   ASSERT_EQ(expected_parts.size(), actual_parts.size());
   size_t count = expected_parts.size();
   for (size_t i = 0; i < count; ++i)
     EXPECT_EQ(expected_parts[i], actual_parts[i]);
   // Must clear the history_dir_ to satisfy the dtor's DCHECK.
   set_history_dir(base::FilePath());
 }