Download autocheckout whitelist and enable autocheckout for whitelisted sites only.

chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc

Index: chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc
diff --git a/chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc b/chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..4eff4364beecb27ac61b3ff8c6176a1f854e348e
--- /dev/null
+++ b/chrome/browser/autofill/autocheckout/whitelist_manager_unittest.cc
@@ -0,0 +1,175 @@
+// Copyright (c) 2013 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 "base/command_line.h"
+#include "base/memory/scoped_ptr.h"
+#include "chrome/browser/autofill/autocheckout/whitelist_manager.h"
+#include "chrome/common/chrome_switches.h"
+#include "chrome/test/base/testing_profile.h"
+#include "content/public/test/test_browser_thread.h"
+#include "googleurl/src/gurl.h"
+#include "net/base/net_errors.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/test_url_fetcher_factory.h"
+#include "net/url_request/url_fetcher_delegate.h"
+#include "net/url_request/url_request_status.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Please omit the second newline.

[benquan, 2013/01/25 00:55:31]

Done.

+namespace {
+
+const char kDownloadWhitelistResponse[] =
+    "https://www.merchant1.com/checkout/\n"
+    "https://cart.merchant2.com/";
+
+}  // namespace
+
+namespace autocheckout {
+
+class WhitelistManagerTest : public testing::Test {
+ public:
+  WhitelistManagerTest() : io_thread_(content::BrowserThread::IO) {}
+
+  virtual void SetUp() {
+    io_thread_.StartIOThread();
+    profile_.CreateRequestContext();
+  }
+
+  virtual void TearDown() {
+    whitelist_manager_.reset();
+    profile_.ResetRequestContext();
+    io_thread_.Stop();
+  }
+
+ protected:
+  void EnsureCreateWhitelistManager() {
+    if (!whitelist_manager_.get())
+      whitelist_manager_.reset(new WhitelistManager::WhitelistManager(
+          profile_.GetRequestContext()));
+  }

[Ilya Sherman, 2013/01/24 22:01:55]

Why not just do this in the constructor?

[benquan, 2013/01/25 00:55:31]

WhitelistManager's constructor checks chrome_switches, some of the testes in
this file needs to override switches before make an instance of
WhitelistManager.

[Ilya Sherman, 2013/01/25 01:22:40]

Hmm, fair enough.

+
+  void DownloadWhitelist(int response_code, const std::string& response) {
+    // Create and register factory.
+    net::TestURLFetcherFactory factory;
+
+    EnsureCreateWhitelistManager();
+
+    whitelist_manager_->TriggerDownload();
+    net::TestURLFetcher* fetcher = factory.GetFetcherByID(0);
+    ASSERT_TRUE(fetcher != NULL);

[Ilya Sherman, 2013/01/24 22:01:55]

nit: You can drop the "!= NULL"

[benquan, 2013/01/25 00:55:31]

Done.

+    fetcher->set_response_code(response_code);
+    fetcher->SetResponseString(response);
+    fetcher->delegate()->OnURLFetchComplete(fetcher);
+  }
+
+  void ResetBackOff() {
+    whitelist_manager_->download_timer_.Stop();
+    whitelist_manager_->callback_pending_ = false;
+  }
+
+  const std::vector<std::string>& GetUrlPrefixes() {

[Ilya Sherman, 2013/01/24 22:01:55]

nit: const

[benquan, 2013/01/25 00:55:31]

Done.

+    return whitelist_manager_->url_prefixes_;
+  }
+
+ protected:
+  TestingProfile profile_;
+  scoped_ptr<WhitelistManager> whitelist_manager_;
+
+ private:
+  MessageLoopForIO message_loop_;
+  // The profile's request context must be released on the IO thread.
+  content::TestBrowserThread io_thread_;
+};
+
+TEST_F(WhitelistManagerTest, DownloadWhitelist) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableExperimentalFormFilling);
+  DownloadWhitelist(net::HTTP_OK, kDownloadWhitelistResponse);
+  EXPECT_EQ((size_t)2, GetUrlPrefixes().size());

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Use C++-style casts rather than C-style casts, i.e. static_cast<size_t>(2).
 But in this specific case, you can just write 2U (U stands for "unsigned")
instead of 2.

[Ilya Sherman, 2013/01/24 22:01:55]

nit: ASSERT_EQ, since the test code below this will crash if there aren't enough
elements.

[benquan, 2013/01/25 00:55:31]

Done.

[benquan, 2013/01/25 00:55:31]

Done.

+  EXPECT_EQ("https://www.merchant1.com/checkout/",
+            GetUrlPrefixes()[0]);
+  EXPECT_EQ("https://cart.merchant2.com/",
+            GetUrlPrefixes()[1]);
+}
+
+TEST_F(WhitelistManagerTest, DoNotDownloadWhitelistWhenSwitchIsOff) {
+  EnsureCreateWhitelistManager();
+  EXPECT_FALSE(whitelist_manager_->ScheduleDownload(3));
+}
+
+TEST_F(WhitelistManagerTest, DoNotDownloadWhitelistWhenBackOff) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableExperimentalFormFilling);
+  EnsureCreateWhitelistManager();
+  // First attempt should schedule a download.
+  EXPECT_TRUE(whitelist_manager_->ScheduleDownload(3));
+  // Second attempt should NOT schedule a download while there is already one.
+  EXPECT_FALSE(whitelist_manager_->ScheduleDownload(3));
+  // It should schedule a new download when not in backoff mode.
+  ResetBackOff();
+  EXPECT_TRUE(whitelist_manager_->ScheduleDownload(3));
+}
+
+TEST_F(WhitelistManagerTest, DownloadWhitelistFailed) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableExperimentalFormFilling);
+  DownloadWhitelist(net::HTTP_INTERNAL_SERVER_ERROR,
+                    kDownloadWhitelistResponse);
+  EXPECT_EQ((size_t)0, GetUrlPrefixes().size());
+
+  ResetBackOff();
+  DownloadWhitelist(net::HTTP_OK, kDownloadWhitelistResponse);
+  EXPECT_EQ((size_t)2, GetUrlPrefixes().size());
+
+  ResetBackOff();
+  DownloadWhitelist(net::HTTP_INTERNAL_SERVER_ERROR,
+                    kDownloadWhitelistResponse);
+  EXPECT_EQ((size_t)2, GetUrlPrefixes().size());
+}
+
+TEST_F(WhitelistManagerTest, IsAutocheckoutEnabled) {
+  CommandLine::ForCurrentProcess()->AppendSwitch(
+      switches::kEnableExperimentalFormFilling);
+  DownloadWhitelist(net::HTTP_OK, kDownloadWhitelistResponse);
+  EXPECT_EQ((size_t)2, GetUrlPrefixes().size());
+
+  // Empty url.
+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(GURL("")));

[Ilya Sherman, 2013/01/24 22:01:55]

nit: Prefer std::string() to ""

[benquan, 2013/01/25 00:55:31]

Done.

+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(GURL()));
+
+  // Positive tests.
+  EXPECT_TRUE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant1.com/checkout/")));
+  EXPECT_TRUE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant1.com/checkout/Shipping")));
+  EXPECT_TRUE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant1.com/checkout/?a=b&c=d")));
+  EXPECT_TRUE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://cart.merchant2.com/")));
+  EXPECT_TRUE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://cart.merchant2.com/ShippingInfo")));
+  EXPECT_TRUE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://cart.merchant2.com/ShippingInfo?a=b&c=d")));
+
+  // Negative tests.
+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant1.com/checkout")));
+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant1.com/")));
+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant1.com/Building")));
+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant2.com/cart")));
+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("a random string")));

[Ilya Sherman, 2013/01/24 22:01:55]

Doesn't calling spec() on an invalid URL cause an assertion failure?

[benquan, 2013/01/25 00:55:31]

No, it doesn't, spec() returns an empty string.
On 2013/01/24 22:01:55, Ilya Sherman wrote:

[Ilya Sherman, 2013/01/25 01:22:40]

In Debug mode, it will trigger a DCHECK:
https://code.google.com/searchframe#OAMlx_jo-ck/src/googleurl/src/gurl.cc&exa...

What am I missing?

On 2013/01/25 00:55:31, benquan wrote:

[benquan, 2013/01/26 02:05:53]

Oh, in my WhitelistManager::IsAutocheckoutEnabled(), I check url.is_empty()
first, it returns true for invalid urls, and it will not call spec().

+
+  // Test different cases in host name and path.
+  EXPECT_TRUE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.Merchant1.com/checkout/")));
+  EXPECT_FALSE(whitelist_manager_->IsAutocheckoutEnabled(
+      GURL("https://www.merchant1.com/CheckOut/")));
+}

[Ilya Sherman, 2013/01/24 22:01:55]

Please add test cases for http://, and for no specified protocol.

[benquan, 2013/01/25 00:55:31]

Done.

+
+}  // namespace autocheckout
+