Query the existence other forms of browsing history.

components/browsing_data_ui/history_notice_utils_unittest.cc

Index: components/browsing_data_ui/history_notice_utils_unittest.cc
diff --git a/components/browsing_data_ui/history_notice_utils_unittest.cc b/components/browsing_data_ui/history_notice_utils_unittest.cc
new file mode 100644
index 0000000000000000000000000000000000000000..f5e591e566222af2b1d03bcc5ad9809e311fcb6d
--- /dev/null
+++ b/components/browsing_data_ui/history_notice_utils_unittest.cc
@@ -0,0 +1,181 @@
+// Copyright 2016 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 "components/browsing_data_ui/history_notice_utils.h"
+
+#include <memory>
+
+#include "base/callback.h"
+#include "base/message_loop/message_loop.h"
+#include "base/run_loop.h"
+#include "components/history/core/test/fake_web_history_service.h"
+#include "components/signin/core/browser/account_tracker_service.h"
+#include "components/signin/core/browser/fake_profile_oauth2_token_service.h"
+#include "components/signin/core/browser/fake_signin_manager.h"
+#include "components/signin/core/browser/test_signin_client.h"
+#include "components/sync_driver/fake_sync_service.h"
+#include "components/version_info/version_info.h"
+#include "net/http/http_status_code.h"
+#include "net/url_request/url_request_test_util.h"
+#include "sync/internal_api/public/base/model_type.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace browsing_data_ui {
+
+namespace {
+
+class TestSyncService : public sync_driver::FakeSyncService {
+ public:
+  // Getters (FakeSyncService implementation). ---------------------------------
+  bool IsSyncActive() const override {
+   return sync_active_;
+  }
+
+  syncer::ModelTypeSet GetActiveDataTypes() const override {
+    return data_types_;
+  }
+
+  bool IsUsingSecondaryPassphrase() const override {
+    return passphrase_;
+  }
+
+  // Setters. ------------------------------------------------------------------
+  void SetSyncActive(bool active) {
+    sync_active_ = active;
+  }
+
+  void SetActiveDataTypes(syncer::ModelTypeSet data_types) {
+    data_types_ = data_types;
+  }
+
+  void SetUsingSecondaryPassphrase(bool passphrase) {
+    passphrase_ = passphrase;
+  }
+
+ private:
+  syncer::ModelTypeSet data_types_;
+  bool passphrase_ = false;
+  bool sync_active_ = false;
+};
+
+}  // namespace
+
+
+class HistoryNoticeUtilsTest : public ::testing::Test {
+ public:
+  HistoryNoticeUtilsTest()
+    : signin_client_(nullptr),
+      signin_manager_(&signin_client_, &account_tracker_) {
+  }
+
+  void SetUp() override {
+    sync_service_.reset(new TestSyncService());
+    history_service_.reset(new history::FakeWebHistoryService(
+        &oauth2_token_service_,
+        &signin_manager_,
+        url_request_context_));
+    history_service_->SetupFakeResponse(true /* success */, net::HTTP_OK);
+  }
+
+  TestSyncService* sync_service() {
+    return sync_service_.get();
+  }
+
+  history::FakeWebHistoryService* history_service() {
+    return history_service_.get();
+  }
+
+  void Expect(bool expected_test_case_result) {
+    got_result_ = false;

[msarda, 2016/05/19 13:35:44]

Nit: Can this be a local variable instead of a member of the class? I think you
should be able to bind a pointer to a local variable it in base::Bind.

[msramek, 2016/05/19 19:47:39]

Done.

+
+    ShouldPopupDialogAboutOtherFormsOfBrowsingHistory(
+        sync_service_.get(),
+        history_service_.get(),
+        version_info::Channel::STABLE,
+        "User agent is not important for this test",
+        base::Bind(&HistoryNoticeUtilsTest::Callback, base::Unretained(this)));
+
+    if (!got_result_) {
+      run_loop_.reset(new base::RunLoop());
+      run_loop_->Run();
+    }
+
+    EXPECT_EQ(expected_test_case_result, result_);
+  }
+
+ private:
+  void Callback(bool result) {
+    got_result_ = true;
+    result_ = result;
+
+    if (run_loop_)
+      run_loop_->Quit();
+  }
+
+  FakeProfileOAuth2TokenService oauth2_token_service_;
+  AccountTrackerService account_tracker_;
+  TestSigninClient signin_client_;
+  FakeSigninManagerBase signin_manager_;
+  scoped_refptr<net::URLRequestContextGetter> url_request_context_;
+  std::unique_ptr<TestSyncService> sync_service_;
+  std::unique_ptr<history::FakeWebHistoryService> history_service_;
+
+  std::unique_ptr<base::RunLoop> run_loop_;
+  bool result_;
+  bool got_result_;
+
+  base::MessageLoop message_loop_;
+};
+
+TEST_F(HistoryNoticeUtilsTest, NotSyncing) {
+  Expect(false);
+}
+
+TEST_F(HistoryNoticeUtilsTest, SyncingWithWrongParameters) {
+  sync_service()->SetSyncActive(true);
+
+  // Regardless of the state of the web history...
+  history_service()->SetWebAndAppActivityEnabled(true);
+  history_service()->SetOtherFormsOfBrowsingHistoryPresent(true);
+
+  // ...the response is false if there's custom passphrase...
+  sync_service()->SetActiveDataTypes(syncer::ModelTypeSet::All());
+  sync_service()->SetUsingSecondaryPassphrase(true);
+  Expect(false);
+
+  // ...or even if there's no custom passphrase, but we're not syncing history.
+  syncer::ModelTypeSet only_passwords(syncer::PASSWORDS);
+  sync_service()->SetActiveDataTypes(only_passwords);
+  sync_service()->SetUsingSecondaryPassphrase(false);
+  Expect(false);
+}
+
+TEST_F(HistoryNoticeUtilsTest, WebHistoryStates) {
+  // If we're syncing...

[msarda, 2016/05/19 13:35:44]

Do not use "we" in comments. (What is we here? Chrome / The Test/ The
programmer/ other?).

[msramek, 2016/05/19 19:47:39]

Done. I agree with you on this, but it's not that uncommon to colloquially use
"we" in comments in Chromium.

+  sync_service()->SetSyncActive(true);
+  sync_service()->SetActiveDataTypes(syncer::ModelTypeSet::All());
+
+  // ...the result is true if both web history queries return true...
+  history_service()->SetWebAndAppActivityEnabled(true);
+  history_service()->SetOtherFormsOfBrowsingHistoryPresent(true);
+  Expect(true);
+
+  // ...but not otherwise.
+  history_service()->SetOtherFormsOfBrowsingHistoryPresent(false);
+  Expect(false);
+  history_service()->SetWebAndAppActivityEnabled(false);
+  Expect(false);
+  history_service()->SetOtherFormsOfBrowsingHistoryPresent(true);
+  Expect(false);
+
+  // Invalid responses from the web history are interpreted as false.
+  history_service()->SetWebAndAppActivityEnabled(true);
+  history_service()->SetOtherFormsOfBrowsingHistoryPresent(true);
+  history_service()->SetupFakeResponse(true, net::HTTP_INTERNAL_SERVER_ERROR);
+  Expect(false);
+  history_service()->SetupFakeResponse(false, net::HTTP_OK);
+  Expect(false);

[msarda, 2016/05/19 13:35:44]

When I first read the code, I though this expect will always fail (Expect(false)
sounds like a failure). But then I realized, Expect is actually an internal
method of the test, not a generic EXPECT statement.

Consider renaming Expect to something more explicit. Maybe something like
ExpectShouldPopupDialogAboutOtherFormsOfBrowsingHistoryWithResult(false) would
make the code more readable.

[msramek, 2016/05/19 19:47:39]

Done.

But I would argue that shorter was more readable. Once you know what "Expect"
means, you can skim through the tests faster. This is of course my fault for
naming the method so long, but I really don't know how else to call it...

+}
+
+}  // namespace browsing_data_ui