Move DriveFS search logic into drivefs_host.cc

This allows proper unittests for this code and simplifies further
caching of results.

BUG=chromium:902600

Change-Id: Ife150c93225e54f6defab814ad5d9c12c8508106
Reviewed-on: https://chromium-review.googlesource.com/c/1327941
Commit-Queue: Sergei Datsenko <dats@chromium.org>
Reviewed-by: Sam McNally <sammc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606747}

chrome/browser/chromeos/extensions/file_manager/private_api_drive.cc

@@ -745,16 +745,8 @@ std::string MakeThumbnailDataUrlOnSequence(
   return base::StrCat({"data:image/png;base64,", encoded});
 }
 
-drivefs::mojom::QueryParameters::QuerySource SearchDriveFs(
-    scoped_refptr<ChromeAsyncExtensionFunction> function,
-    drivefs::mojom::QueryParametersPtr query,
-    bool filter_dirs,
-    base::OnceCallback<void(std::unique_ptr<base::ListValue>)> callback);
-
 void OnSearchDriveFs(
     scoped_refptr<ChromeAsyncExtensionFunction> function,
-    drivefs::mojom::SearchQueryPtr search,
-    drivefs::mojom::QueryParametersPtr query,
     bool filter_dirs,
     base::OnceCallback<void(std::unique_ptr<base::ListValue>)> callback,
     drive::FileError error,
@@ -766,22 +758,6 @@ void OnSearchDriveFs(
     return;
   }
 
-  if (error == drive::FILE_ERROR_NO_CONNECTION &&
-      query->query_source !=
-          drivefs::mojom::QueryParameters::QuerySource::kLocalOnly) {
-    // Retry with offline query.
-    query->query_source =
-        drivefs::mojom::QueryParameters::QuerySource::kLocalOnly;
-    if (query->text_content) {
-      // Full-text searches not supported offline.
-      std::swap(query->text_content, query->title);
-      query->text_content.reset();
-    }
-    SearchDriveFs(std::move(function), std::move(query), filter_dirs,
-                  std::move(callback));
-    return;
-  }
-
   if (error != drive::FILE_ERROR_OK || !items.has_value()) {
     std::move(callback).Run(nullptr);
     return;
@@ -824,24 +800,9 @@ drivefs::mojom::QueryParameters::QuerySource SearchDriveFs(
     base::OnceCallback<void(std::unique_ptr<base::ListValue>)> callback) {
   drive::DriveIntegrationService* const integration_service =
       drive::util::GetIntegrationServiceByProfile(function->GetProfile());
-  drivefs::mojom::SearchQueryPtr search;
-  integration_service->GetDriveFsInterface()->StartSearchQuery(
-      mojo::MakeRequest(&search), query.Clone());
-  drivefs::mojom::QueryParameters::QuerySource source = query->query_source;
-  if (net::NetworkChangeNotifier::IsOffline() &&
-      source != drivefs::mojom::QueryParameters::QuerySource::kLocalOnly) {
-    // No point trying cloud query if we know we are offline.
-    source = drivefs::mojom::QueryParameters::QuerySource::kLocalOnly;
-    OnSearchDriveFs(std::move(function), std::move(search), std::move(query),
-                    filter_dirs, std::move(callback),
-                    drive::FILE_ERROR_NO_CONNECTION, {});
-  } else {
-    auto* raw_search = search.get();
-    raw_search->GetNextPage(
-        base::BindOnce(&OnSearchDriveFs, std::move(function), std::move(search),
-                       std::move(query), filter_dirs, std::move(callback)));
-  }
-  return source;
+  return integration_service->GetDriveFsHost()->PerformSearch(
+      std::move(query), base::BindOnce(&OnSearchDriveFs, std::move(function),
+                                       filter_dirs, std::move(callback)));
 }
 
 void UmaEmitSearchOutcome(

chromeos/components/drivefs/BUILD.gn

@@ -64,6 +64,7 @@ source_set("unit_tests") {
     "//components/invalidation/impl:test_support",
     "//mojo/public/cpp/bindings",
     "//net",
+    "//net:test_support",
     "//services/identity/public/mojom",
     "//services/network/public/cpp:cpp",
     "//services/service_manager/public/cpp",

chromeos/components/drivefs/drivefs_host.cc

@@ -17,6 +17,7 @@
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/platform/platform_channel_endpoint.h"
 #include "mojo/public/cpp/system/invitation.h"
+#include "net/base/network_change_notifier.h"
 #include "services/identity/public/mojom/constants.mojom.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -200,7 +201,8 @@ class DriveFsHost::MountState
         mojo_connection_delegate_(
             host_->delegate_->CreateMojoConnectionDelegate()),
         pending_token_(base::UnguessableToken::Create()),
-        binding_(this) {
+        binding_(this),
+        weak_ptr_factory_(this) {
     host_->disk_mount_manager_->AddObserver(this);
     source_path_ = base::StrCat({kMountScheme, pending_token_.ToString()});
     std::string datadir_option =
@@ -219,9 +221,8 @@ class DriveFsHost::MountState
 
     // If unconsumed, the registration is cleaned up when |this| is destructed.
     PendingConnectionManager::Get().ExpectOpenIpcChannel(
-        pending_token_,
-        base::BindOnce(&DriveFsHost::MountState::AcceptMojoConnection,
-                       base::Unretained(this)));
+        pending_token_, base::BindOnce(&MountState::AcceptMojoConnection,
+                                       base::Unretained(this)));
 
     host_->disk_mount_manager_->MountPath(
         source_path_, "",
@@ -268,6 +269,27 @@ class DriveFsHost::MountState
     mojo_connection_delegate_->AcceptMojoConnection(std::move(handle));
   }
 
+  mojom::QueryParameters::QuerySource SearchDriveFs(
+      mojom::QueryParametersPtr query,
+      mojom::SearchQuery::GetNextPageCallback callback) {
+    drivefs::mojom::SearchQueryPtr search;
+    drivefs::mojom::QueryParameters::QuerySource source = query->query_source;
+    if (net::NetworkChangeNotifier::IsOffline() &&
+        source != drivefs::mojom::QueryParameters::QuerySource::kLocalOnly) {
+      // No point trying cloud query if we know we are offline.
+      source = drivefs::mojom::QueryParameters::QuerySource::kLocalOnly;
+      OnSearchDriveFs(std::move(search), std::move(query), std::move(callback),
+                      drive::FILE_ERROR_NO_CONNECTION, {});
+    } else {
+      drivefs_->StartSearchQuery(mojo::MakeRequest(&search), query.Clone());
+      auto* raw_search = search.get();
+      raw_search->GetNextPage(base::BindOnce(
+          &MountState::OnSearchDriveFs, weak_ptr_factory_.GetWeakPtr(),
+          std::move(search), std::move(query), std::move(callback)));
+    }
+    return source;
+  }
+
  private:
   // mojom::DriveFsDelegate:
   void GetAccessToken(const std::string& client_id,
@@ -428,6 +450,30 @@ class DriveFsHost::MountState
 
   void OnNotificationTimerFired() override { drivefs_->FetchAllChangeLogs(); }
 
+  void OnSearchDriveFs(
+      drivefs::mojom::SearchQueryPtr search,
+      drivefs::mojom::QueryParametersPtr query,
+      mojom::SearchQuery::GetNextPageCallback callback,
+      drive::FileError error,
+      base::Optional<std::vector<drivefs::mojom::QueryItemPtr>> items) {
+    if (error == drive::FILE_ERROR_NO_CONNECTION &&
+        query->query_source !=
+            drivefs::mojom::QueryParameters::QuerySource::kLocalOnly) {
+      // Retry with offline query.
+      query->query_source =
+          drivefs::mojom::QueryParameters::QuerySource::kLocalOnly;
+      if (query->text_content) {
+        // Full-text searches not supported offline.
+        std::swap(query->text_content, query->title);
+        query->text_content.reset();
+      }
+      SearchDriveFs(std::move(query), std::move(callback));
+      return;
+    }
+
+    std::move(callback).Run(error, std::move(items));
+  }
+
   // Owns |this|.
   DriveFsHost* const host_;
 
@@ -452,6 +498,8 @@ class DriveFsHost::MountState
   bool drivefs_has_terminated_ = false;
   bool token_fetch_attempted_ = false;
 
+  base::WeakPtrFactory<MountState> weak_ptr_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(MountState);
 };
 
@@ -525,4 +573,10 @@ mojom::DriveFs* DriveFsHost::GetDriveFsInterface() const {
   return mount_state_->GetDriveFsInterface();
 }
 
+mojom::QueryParameters::QuerySource DriveFsHost::PerformSearch(
+    mojom::QueryParametersPtr query,
+    mojom::SearchQuery::GetNextPageCallback callback) {
+  return mount_state_->SearchDriveFs(std::move(query), std::move(callback));
+}
+
 }  // namespace drivefs

chromeos/components/drivefs/drivefs_host.h

@@ -135,6 +135,12 @@ class COMPONENT_EXPORT(DRIVEFS) DriveFsHost {
 
   mojom::DriveFs* GetDriveFsInterface() const;
 
+  // Starts DriveFs search query and returns whether it will be
+  // performed localy or remotely. Assumes DriveFS to be mounted.
+  mojom::QueryParameters::QuerySource PerformSearch(
+      mojom::QueryParametersPtr query,
+      mojom::SearchQuery::GetNextPageCallback callback);
+
  private:
   class AccountTokenDelegate;
   class MountState;

chromeos/components/drivefs/drivefs_host_unittest.cc

@@ -25,6 +25,7 @@
 #include "components/invalidation/impl/fake_invalidation_service.h"
 #include "mojo/public/cpp/bindings/binding.h"
 #include "mojo/public/cpp/bindings/binding_set.h"
+#include "net/base/mock_network_change_notifier.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "services/service_manager/public/cpp/binder_registry.h"
 #include "services/service_manager/public/cpp/connector.h"
@@ -116,8 +117,11 @@ class ForwardingOAuth2MintTokenFlow : public OAuth2MintTokenFlow {
   MockOAuth2MintTokenFlow* mock_;
 };
 
-class MockDriveFs : public mojom::DriveFsInterceptorForTesting {
+class MockDriveFs : public mojom::DriveFsInterceptorForTesting,
+                    public mojom::SearchQuery {
  public:
+  MockDriveFs() : search_binding_(this) {}
+
   DriveFs* GetForwardingInterface() override {
     NOTREACHED();
     return nullptr;
@@ -136,6 +140,28 @@ class MockDriveFs : public mojom::DriveFsInterceptorForTesting {
       FetchChangeLogImpl,
       void(const std::vector<std::pair<int64_t, std::string>>& options));
   MOCK_METHOD0(FetchAllChangeLogs, void());
+
+  MOCK_CONST_METHOD1(OnStartSearchQuery, void(const mojom::QueryParameters&));
+  void StartSearchQuery(mojom::SearchQueryRequest query,
+                        mojom::QueryParametersPtr query_params) override {
+    if (search_binding_.is_bound())
+      search_binding_.Unbind();
+    OnStartSearchQuery(*query_params);
+    search_binding_.Bind(std::move(query));
+  }
+
+  MOCK_METHOD1(OnGetNextPage,
+               drive::FileError(
+                   base::Optional<std::vector<mojom::QueryItemPtr>>* items));
+
+  void GetNextPage(GetNextPageCallback callback) override {
+    base::Optional<std::vector<mojom::QueryItemPtr>> items;
+    auto error = OnGetNextPage(&items);
+    std::move(callback).Run(error, std::move(items));
+  }
+
+ private:
+  mojo::Binding<mojom::SearchQuery> search_binding_;
 };
 
 class TestingDriveFsHostDelegate : public DriveFsHost::Delegate,
@@ -442,6 +468,7 @@ class DriveFsHostTest : public ::testing::Test, public mojom::DriveFsBootstrap {
   std::unique_ptr<TestingDriveFsHostDelegate> host_delegate_;
   std::unique_ptr<DriveFsHost> host_;
   base::MockOneShotTimer* timer_;
+  net::test::MockNetworkChangeNotifier network_;
 
   mojo::Binding<mojom::DriveFsBootstrap> bootstrap_binding_;
   MockDriveFs mock_drivefs_;
@@ -1121,5 +1148,152 @@ TEST_F(DriveFsHostTest, Remount_RequestInflight) {
   ExpectAccessToken(mojom::AccessTokenStatus::kSuccess, "auth token");
 }
 
+ACTION_P(PopulateSearch, count) {
+  if (count < 0)
+    return;
+  std::vector<mojom::QueryItemPtr> items;
+  for (int i = 0; i < count; ++i) {
+    items.emplace_back(mojom::QueryItem::New());
+    items.back()->metadata = mojom::FileMetadata::New();
+    items.back()->metadata->capabilities = mojom::Capabilities::New();
+  }
+  *arg0 = std::move(items);
+}
+
+MATCHER_P5(MatchQuery, source, text, title, shared, offline, "") {
+  if (arg.query_source != source)
+    return false;
+  if (text != nullptr) {
+    if (!arg.text_content || *arg.text_content != base::StringPiece(text))
+      return false;
+  } else {
+    if (arg.text_content)
+      return false;
+  }
+  if (title != nullptr) {
+    if (!arg.title || *arg.title != base::StringPiece(title))
+      return false;
+  } else {
+    if (arg.title)
+      return false;
+  }
+  return arg.shared_with_me == shared && arg.available_offline == offline;
+};
+
+TEST_F(DriveFsHostTest, Search) {
+  ASSERT_NO_FATAL_FAILURE(DoMount());
+
+  EXPECT_CALL(mock_drivefs_, OnStartSearchQuery(_));
+  EXPECT_CALL(mock_drivefs_, OnGetNextPage(_))
+      .WillOnce(testing::DoAll(
+          PopulateSearch(3), testing::Return(drive::FileError::FILE_ERROR_OK)));
+
+  mojom::QueryParametersPtr params = mojom::QueryParameters::New();
+  params->query_source = mojom::QueryParameters::QuerySource::kLocalOnly;
+
+  bool called = false;
+  mojom::QueryParameters::QuerySource source = host_->PerformSearch(
+      std::move(params),
+      base::BindLambdaForTesting(
+          [&called](drive::FileError err,
+                    base::Optional<std::vector<mojom::QueryItemPtr>> items) {
+            called = true;
+            EXPECT_EQ(drive::FileError::FILE_ERROR_OK, err);
+            EXPECT_EQ(3u, items->size());
+          }));
+  EXPECT_EQ(mojom::QueryParameters::QuerySource::kLocalOnly, source);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+}
+
+TEST_F(DriveFsHostTest, Search_Fail) {
+  ASSERT_NO_FATAL_FAILURE(DoMount());
+
+  EXPECT_CALL(mock_drivefs_, OnStartSearchQuery(_));
+  EXPECT_CALL(mock_drivefs_, OnGetNextPage(_))
+      .WillOnce(testing::Return(drive::FileError::FILE_ERROR_ACCESS_DENIED));
+
+  mojom::QueryParametersPtr params = mojom::QueryParameters::New();
+  params->query_source = mojom::QueryParameters::QuerySource::kCloudOnly;
+
+  bool called = false;
+  mojom::QueryParameters::QuerySource source = host_->PerformSearch(
+      std::move(params),
+      base::BindLambdaForTesting(
+          [&called](drive::FileError err,
+                    base::Optional<std::vector<mojom::QueryItemPtr>> items) {
+            called = true;
+            EXPECT_EQ(drive::FileError::FILE_ERROR_ACCESS_DENIED, err);
+          }));
+  EXPECT_EQ(mojom::QueryParameters::QuerySource::kCloudOnly, source);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+}
+
+TEST_F(DriveFsHostTest, Search_OnlineToOffline) {
+  ASSERT_NO_FATAL_FAILURE(DoMount());
+
+  network_.SetConnectionType(
+      net::NetworkChangeNotifier::ConnectionType::CONNECTION_NONE);
+
+  EXPECT_CALL(mock_drivefs_, OnStartSearchQuery(_));
+  EXPECT_CALL(mock_drivefs_, OnGetNextPage(_))
+      .WillOnce(testing::DoAll(
+          PopulateSearch(3), testing::Return(drive::FileError::FILE_ERROR_OK)));
+
+  mojom::QueryParametersPtr params = mojom::QueryParameters::New();
+  params->query_source = mojom::QueryParameters::QuerySource::kCloudOnly;
+
+  bool called = false;
+  mojom::QueryParameters::QuerySource source = host_->PerformSearch(
+      std::move(params),
+      base::BindLambdaForTesting(
+          [&called](drive::FileError err,
+                    base::Optional<std::vector<mojom::QueryItemPtr>> items) {
+            called = true;
+            EXPECT_EQ(drive::FileError::FILE_ERROR_OK, err);
+            EXPECT_EQ(3u, items->size());
+          }));
+  EXPECT_EQ(mojom::QueryParameters::QuerySource::kLocalOnly, source);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+}
+
+TEST_F(DriveFsHostTest, Search_OnlineToOfflineFallback) {
+  ASSERT_NO_FATAL_FAILURE(DoMount());
+
+  EXPECT_CALL(mock_drivefs_,
+              OnStartSearchQuery(
+                  MatchQuery(mojom::QueryParameters::QuerySource::kCloudOnly,
+                             "foobar", nullptr, false, false)));
+  EXPECT_CALL(mock_drivefs_,
+              OnStartSearchQuery(
+                  MatchQuery(mojom::QueryParameters::QuerySource::kLocalOnly,
+                             nullptr, "foobar", false, false)));
+
+  EXPECT_CALL(mock_drivefs_, OnGetNextPage(_))
+      .WillOnce(testing::Return(drive::FileError::FILE_ERROR_NO_CONNECTION))
+      .WillOnce(testing::DoAll(
+          PopulateSearch(3), testing::Return(drive::FileError::FILE_ERROR_OK)));
+
+  mojom::QueryParametersPtr params = mojom::QueryParameters::New();
+  params->query_source = mojom::QueryParameters::QuerySource::kCloudOnly;
+  params->text_content = "foobar";
+
+  bool called = false;
+  mojom::QueryParameters::QuerySource source = host_->PerformSearch(
+      std::move(params),
+      base::BindLambdaForTesting(
+          [&called](drive::FileError err,
+                    base::Optional<std::vector<mojom::QueryItemPtr>> items) {
+            called = true;
+            EXPECT_EQ(drive::FileError::FILE_ERROR_OK, err);
+            EXPECT_EQ(3u, items->size());
+          }));
+  EXPECT_EQ(mojom::QueryParameters::QuerySource::kCloudOnly, source);
+  base::RunLoop().RunUntilIdle();
+  EXPECT_TRUE(called);
+}
+
 }  // namespace
 }  // namespace drivefs