Create test fixture for SafeBrowsingService

chrome/browser/safe_browsing/test_safe_browsing_service.cc

+// Copyright (c) 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 "chrome/browser/safe_browsing/test_safe_browsing_service.h"
+
+#include "base/strings/string_util.h"
+#include "chrome/browser/safe_browsing/download_protection_service.h"
+#include "chrome/browser/safe_browsing/ping_manager.h"
+#include "chrome/browser/safe_browsing/ui_manager.h"
+#include "components/safe_browsing_db/database_manager.h"
+#include "components/safe_browsing_db/test_database_manager.h"
+
+namespace safe_browsing {
+
+TestSafeBrowsingUIManager::TestSafeBrowsingUIManager()

[Nathan Parker, 2016/05/07 00:05:32]

nit: Can you add a comment/header between the different class groupings of
methods?  Makes it easier to read.

[Jialiu Lin, 2016/05/12 21:53:58]

Comments added. Also moved TestSafeBrowsingUIManager::* functions to the end to
make the order consistent with functions .h files

+    : SafeBrowsingUIManager(nullptr) {}
+
+TestSafeBrowsingUIManager::TestSafeBrowsingUIManager(
+    const scoped_refptr<SafeBrowsingService>& service)
+    : SafeBrowsingUIManager(service) {}
+
+void TestSafeBrowsingUIManager::SetSafeBrowsingService(
+    SafeBrowsingService* sb_service) {
+  sb_service_ = sb_service;
+}
+
+void TestSafeBrowsingUIManager::SendSerializedThreatDetails(
+    const std::string& serialized) {
+  details_.push_back(serialized);
+}
+
+std::list<std::string>* TestSafeBrowsingUIManager::GetThreatDetails() {
+  return &details_;
+}
+
+TestSafeBrowsingUIManager::~TestSafeBrowsingUIManager() {}
+
+TestSafeBrowsingService::TestSafeBrowsingService()
+    : protocol_manager_delegate_disabled_(false),
+      serialized_download_report_(base::EmptyString()) {}
+
+TestSafeBrowsingService::~TestSafeBrowsingService() {}
+
+SafeBrowsingProtocolConfig TestSafeBrowsingService::GetProtocolConfig() const {
+  if (protocol_config_)
+    return *protocol_config_;
+  return SafeBrowsingService::GetProtocolConfig();
+}
+
+V4ProtocolConfig TestSafeBrowsingService::GetV4ProtocolConfig() const {
+  if (v4_protocol_config_)
+    return *v4_protocol_config_;
+  return SafeBrowsingService::GetV4ProtocolConfig();
+}
+
+std::string TestSafeBrowsingService::serilized_download_report() {
+  return serialized_download_report_;
+}
+
+void TestSafeBrowsingService::SetDatabaseManager(
+    TestSafeBrowsingDatabaseManager* database_manager) {
+  database_manager_ = database_manager;
+  // Since TestSafeBrowsingDatabaseManager does not implement
+  // SafeBrowsingProtocolManagerDelegate, when it is used we need to disable
+  // protocol_manager_delegate.
+  protocol_manager_delegate_disabled_ = true;
+}
+
+void TestSafeBrowsingService::SetUIManager(
+    TestSafeBrowsingUIManager* ui_manager) {
+  ui_manager->SetSafeBrowsingService(this);
+  ui_manager_ = ui_manager;
+}
+
+SafeBrowsingDatabaseManager* TestSafeBrowsingService::CreateDatabaseManager() {
+  if (database_manager_)
+    return database_manager_.get();
+  return SafeBrowsingService::CreateDatabaseManager();

[Nathan Parker, 2016/05/07 00:05:32]

Should this store the created object?  The function below creates and stores.

[Jialiu Lin, 2016/05/12 21:53:58]

Since the only place CreateDatabaseManager() called is  "database_manager_ =
CreateDatabaseManager();", don't need to store the created object. 
CreateUIManager should be similar.  I modified CreateUIManager instead. 

+}
+
+SafeBrowsingUIManager* TestSafeBrowsingService::CreateUIManager() {
+  if (!ui_manager_)
+    ui_manager_ = new TestSafeBrowsingUIManager(this);
+  return ui_manager_.get();
+}
+
+SafeBrowsingProtocolManagerDelegate*
+TestSafeBrowsingService::GetProtocolManagerDelegate() {
+  if (protocol_manager_delegate_disabled_)
+    return nullptr;
+  return SafeBrowsingService::GetProtocolManagerDelegate();
+}
+
+void TestSafeBrowsingService::SendSerializedDownloadReport(
+    const std::string& report) {
+  serialized_download_report_ = report;
+}
+
+void TestSafeBrowsingService::SetProtocolConfig(
+    SafeBrowsingProtocolConfig* protocol_config) {

[Nathan Parker, 2016/05/07 00:05:32]

Would this be simpler as a pass- and store-by-value? That'd make it easier to
manage ownership of the thing.

[Jialiu Lin, 2016/05/12 21:53:58]

Currently, TestSafeBrowsingService relies on if(protocol_config_) to determine
if use customized protocol config or not. If it is stored by value, it is hard
to check protocol_config_ has been set of not. 

[Nathan Parker, 2016/05/16 19:56:39]

Makes sense.

I'm not advocating it, but I've been looking for an excuse to use the new
base::Optional.  Maybe for new things it'd be better.

+  protocol_config_.reset(protocol_config);
+}
+
+void TestSafeBrowsingService::SetV4ProtocolConfig(
+    V4ProtocolConfig* v4_protocol_config) {
+  v4_protocol_config_.reset(v4_protocol_config);
+}
+
+TestSafeBrowsingServiceFactory::TestSafeBrowsingServiceFactory()
+    : test_safe_browsing_service_(nullptr), test_protocol_config_(nullptr) {}
+
+TestSafeBrowsingServiceFactory::~TestSafeBrowsingServiceFactory() {}
+
+SafeBrowsingService*
+TestSafeBrowsingServiceFactory::CreateSafeBrowsingService() {
+  // Instantiate TestSafeBrowsingService.
+  test_safe_browsing_service_ = new TestSafeBrowsingService();
+  // Plug-in test member clases accordingly.
+  if (test_ui_manager_)
+    test_safe_browsing_service_->SetUIManager(test_ui_manager_.get());
+  if (test_database_manager_)
+    test_safe_browsing_service_->SetDatabaseManager(
+        test_database_manager_.get());
+  if (test_protocol_config_)
+    test_safe_browsing_service_->SetProtocolConfig(test_protocol_config_);
+  return test_safe_browsing_service_;

[Nathan Parker, 2016/05/07 00:05:32]

Does the factory need to retain a copy of this ptr?  Or if it does, should it
instead be unique_ptr so it owns it?

[Jialiu Lin, 2016/05/12 21:53:58]

hmm, it actually makes things more complicated. Since this object will be passed
to browser process and hold in scope_refptr. Keeping a ownership here make
destruction procedure super convoluted. 

+}
+
+TestSafeBrowsingService*
+TestSafeBrowsingServiceFactory::test_safe_browsing_service() {
+  return test_safe_browsing_service_;
+}
+
+void TestSafeBrowsingServiceFactory::SetTestUIManager(
+    TestSafeBrowsingUIManager* ui_manager) {
+  test_ui_manager_ = ui_manager;
+  if (test_safe_browsing_service_)
+    test_ui_manager_->SetSafeBrowsingService(test_safe_browsing_service_);
+}
+
+void TestSafeBrowsingServiceFactory::SetTestDatabaseManager(
+    TestSafeBrowsingDatabaseManager* database_manager) {
+  test_database_manager_ = database_manager;
+}
+
+void TestSafeBrowsingServiceFactory::SetTestProtocolConfig(
+    const SafeBrowsingProtocolConfig& protocol_config) {
+  test_protocol_config_ = new SafeBrowsingProtocolConfig(protocol_config);
+}
+
+}  // namespace safe_browsing