Cancel the JumpList update if top 5 most visited URLs are unchanged

chrome/browser/win/jumplist_update_util_unittest.cc

Index: chrome/browser/win/jumplist_update_util_unittest.cc
diff --git a/chrome/browser/win/jumplist_update_util_unittest.cc b/chrome/browser/win/jumplist_update_util_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..2b28323018bb00044f9a196581cc3814e0e33512
--- /dev/null
+++ b/chrome/browser/win/jumplist_update_util_unittest.cc
@@ -0,0 +1,83 @@
+// Copyright (c) 2017 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 "chrome/browser/win/jumplist_update_util.h"
+
+#include "algorithm"

[grt (UTC plus 2), 2017/05/30 08:51:42]

<algorithm>

[chengx, 2017/05/30 19:37:02]

Done.

+
+#include "base/containers/hash_tables.h"

[grt (UTC plus 2), 2017/05/30 08:51:42]

a comment in this file says:

// This header file is deprecated. Use the corresponding C++11 type
// instead. https://crbug.com/576864

[chengx, 2017/05/30 19:37:02]

I've removed this header file since kTestData doesn't use a map anymore.

+#include "base/strings/utf_string_conversions.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace {
+
+// An url-title map used to initilize the member variables in class
+// JumpListUpdateUtilTest.
+base::hash_map<std::string, std::string> url_title = {
+    {"https://www.google.com/", "Google"},
+    {"https://www.youtube.com/", "Youtube"},
+    {"https://www.gmail.com/", "Gmail"}};
+
+// Helper function to create a ShellLinkItem whose url is specified by |url|.
+scoped_refptr<ShellLinkItem> CreateShellLinkWithURL(const std::string& url) {
+  scoped_refptr<ShellLinkItem> item(new ShellLinkItem);

[grt (UTC plus 2), 2017/05/30 08:51:42]

  auto item = base::MakeRefCounted<ShellLinkItem>();

[chengx, 2017/05/30 19:37:02]

Done.

+  item->set_url(url);
+  return item;
+}
+
+}  // namespace
+
+class JumpListUpdateUtilTest : public testing::Test {

[grt (UTC plus 2), 2017/05/30 08:51:42]

i don't think this test fixture adds any value as-is. how about removing it and
using TEST below?

[chengx, 2017/05/30 19:37:03]

Done.

+ protected:
+  // A list of ShellLinkItem and each ShellLinkItem contains an URL.
+  ShellLinkItemList item_list;
+
+  // A list of history::MostVisitedURL, each of which contains an URL.
+  history::MostVisitedURLList url_list;
+};
+
+TEST_F(JumpListUpdateUtilTest, MostVisitedItemsUnchanged) {
+  // Initialize item_list and url_list using the url_title map.

[grt (UTC plus 2), 2017/05/30 08:51:42]

replace url_title in the unnamed namespace with the following here:

  static constexpr struct {
    const char* url;
    const wchar_t* title;
  } kTestData[] = {
    {"https://www.google.com/", L"Google"},
    {"https://www.youtube.com/", L"Youtube"},
    {"https://www.gmail.com/", L"Gmail"}};

[chengx, 2017/05/30 19:37:02]

Done.

+  for (const auto& url_title_pair : url_title) {
+    item_list.push_back(CreateShellLinkWithURL(url_title_pair.first));
+
+    GURL url(base::UTF8ToUTF16(url_title_pair.first));

[grt (UTC plus 2), 2017/05/30 08:51:42]

this conversion isn't needed -- GURL has a ctor that takes a narrow string

[chengx, 2017/05/30 19:37:02]

Done.

+    history::MostVisitedURL most_visited_url(
+        url, base::UTF8ToUTF16(url_title_pair.second));

[grt (UTC plus 2), 2017/05/30 08:51:42]

remove this conversion in favor of using a wchar_t literal in the test data (as
above).

[chengx, 2017/05/30 19:37:03]

Done.

+    url_list.push_back(most_visited_url);
+  }
+
+  // Now both item_list and url_list have 3 urls: Google, Youtube, Gmail.
+  // Also, their urls have the same order.
+  EXPECT_TRUE(MostVisitedItemsUnchanged(item_list, url_list, 3));
+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 2));
+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 1));
+
+  // Reverse url_list.
+  // Now item_list has 3 urls: Google, Youtube, Gmail,
+  // and url_list's 3 urls are in reverse order: Gmail, Youtube, Google.
+  std::reverse(url_list.begin(), url_list.end());
+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 3));
+
+  // Reverse url_list back.
+  std::reverse(url_list.begin(), url_list.end());
+  EXPECT_TRUE(MostVisitedItemsUnchanged(item_list, url_list, 3));
+
+  // Pop out the last url ("Gmail") from item_list.
+  // Now item_list has 2 urls: Google, Youtube,
+  // and url_list has 3 urls: Google, Youtube, Gmail.
+  item_list.pop_back();

[grt (UTC plus 2), 2017/05/30 08:51:42]

i'm finding it hard to follow what "url_list" and "item_list" are. i think it
would be more clear if they were called "history_items" and "jumplist_items" (or
"shell_links" or something).

[chengx, 2017/05/30 19:37:03]

I changed to history_items and jumplist_items.

+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 3));
+  EXPECT_TRUE(MostVisitedItemsUnchanged(item_list, url_list, 2));
+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 1));
+
+  // Pop out the last two urls ("Youtube", "Gmail") from url_list.
+  // Now item_list has 2 urls: Google, Youtube,
+  // and url_list has 1 url: Google.
+  url_list.pop_back();
+  url_list.pop_back();
+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 3));
+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 2));
+  EXPECT_FALSE(MostVisitedItemsUnchanged(item_list, url_list, 1));
+}