Revamp logic of SmbShareFinder::GatherSharesInNetwork and SmbShareFinder::DiscoverHostInNetwork

- Simplified logic of handling HostDiscovery callbacks and shares
  callbacks in SmbShareFinder::GatherSharesInNetwork.
- SmbShareFinder::DiscoverHostsInNetwork now follows the same logic as
  SmbShareFinder::GatherShareInNetwork in how it determines when to run
  HostDiscovery.
- This change prevents SmbShareFinder::DiscoverHostsInNetwork from
  running multiple HostDiscovery's concurrently.

Bug: 922273
Test: Browser unit test
Change-Id: Ia0bba61c4cdce6629a42f29d5326161478dca168
Reviewed-on: https://chromium-review.googlesource.com/c/1474384Reviewed-by: Zentaro Kavanagh <zentaro@chromium.org>
Reviewed-by: Bailey Berro <baileyberro@chromium.org>
Commit-Queue: jimmy gong <jimmyxgong@chromium.org>
Cr-Commit-Position: refs/heads/master@{#634422}

chrome/browser/chromeos/smb_client/smb_share_finder.cc

@@ -19,29 +19,63 @@ SmbShareFinder::~SmbShareFinder() = default;
 void SmbShareFinder::GatherSharesInNetwork(
     HostDiscoveryResponse discovery_callback,
     GatherSharesInNetworkResponse shares_callback) {
-  const bool should_start_discovery = share_callbacks_.empty();
+  const bool is_host_discovery_pending = !discovery_callbacks_.empty();
+  const bool is_share_discovery_pending = !share_callbacks_.empty();
 
-  if (discovery_callbacks_.empty() && !should_start_discovery) {
-    // Host discovery completed but shares callback is still in progress.
-    std::move(discovery_callback).Run();
-    InsertShareCallback(std::move(shares_callback));
-  } else {
-    // Either GatherSharesInNetwork has not been called yet or it has and
-    // discovery has not yet finished.
+  if (is_host_discovery_pending) {
+    // Host discovery is currently running, add both |discovery_callback| and
+    // |share_callback| to their respective vectors.
     InsertDiscoveryAndShareCallbacks(std::move(discovery_callback),
                                      std::move(shares_callback));
+    return;
   }
-  if (should_start_discovery) {
-    scanner_.FindHostsInNetwork(
-        base::BindOnce(&SmbShareFinder::OnHostsFound, AsWeakPtr()));
+
+  if (is_share_discovery_pending) {
+    // Host discovery is complete but there are still share callbacks pending.
+    // Run |discovery_callback| because pending share discoveries and no pending
+    // host discoveries indicate that a host discovery must have recently
+    // completed.
+    std::move(discovery_callback).Run();
+    InsertShareCallback(std::move(shares_callback));
+    return;
   }
+
+  // No host discovery or share discovery in progress. This is only because
+  // GatherSharesInNetwork has not been called yet or the previous host
+  // discovery has been fully completed.
+  InsertDiscoveryAndShareCallbacks(std::move(discovery_callback),
+                                   std::move(shares_callback));
+  scanner_.FindHostsInNetwork(
+      base::BindOnce(&SmbShareFinder::OnHostsFound, AsWeakPtr()));
 }
 
 void SmbShareFinder::DiscoverHostsInNetwork(
     HostDiscoveryResponse discovery_callback) {
-  scanner_.FindHostsInNetwork(base::BindOnce(&SmbShareFinder::OnHostsDiscovered,
-                                             AsWeakPtr(),
-                                             std::move(discovery_callback)));
+  const bool is_host_discovery_pending = !discovery_callbacks_.empty();
+  const bool is_share_discovery_pending = !share_callbacks_.empty();
+
+  if (is_host_discovery_pending) {
+    // Host discovery is currently running, add |discovery_callback| to
+    // |discovery_callbacks|.
+    InsertDiscoveryCallback(std::move(discovery_callback));
+    return;
+  }
+
+  if (is_share_discovery_pending) {
+    // Host discovery is complete but there are still share callbacks pending.
+    // Run |discovery_callback| because pending share discoveries and no pending
+    // host discoveries indicate that a host discovery must have recently
+    // completed.
+    std::move(discovery_callback).Run();
+    return;
+  }
+
+  // No host discovery or share discovery in progress. This is only because
+  // GatherSharesInNetwork has not been called yet or the previous host
+  // discovery has been fully completed.
+  InsertDiscoveryCallback(std::move(discovery_callback));
+  scanner_.FindHostsInNetwork(
+      base::BindOnce(&SmbShareFinder::OnHostsFound, AsWeakPtr()));
 }
 
 void SmbShareFinder::RegisterHostLocator(std::unique_ptr<HostLocator> locator) {
@@ -67,12 +101,6 @@ bool SmbShareFinder::TryResolveUrl(const SmbUrl& url,
   return *updated_url != url.ToString();
 }
 
-void SmbShareFinder::OnHostsDiscovered(HostDiscoveryResponse discovery_callback,
-                                       bool success,
-                                       const HostMap& hosts) {
-  std::move(discovery_callback).Run();
-}
-
 void SmbShareFinder::OnHostsFound(bool success, const HostMap& hosts) {
   DCHECK_EQ(0u, host_counter_);
 
@@ -149,8 +177,8 @@ void SmbShareFinder::RunEmptySharesCallbacks() {
 void SmbShareFinder::InsertDiscoveryAndShareCallbacks(
     HostDiscoveryResponse discovery_callback,
     GatherSharesInNetworkResponse share_callback) {
-  discovery_callbacks_.push_back(std::move(discovery_callback));
-  share_callbacks_.push_back(std::move(share_callback));
+  InsertDiscoveryCallback(std::move(discovery_callback));
+  InsertShareCallback(std::move(share_callback));
 }
 
 void SmbShareFinder::InsertShareCallback(
@@ -158,5 +186,10 @@ void SmbShareFinder::InsertShareCallback(
   share_callbacks_.push_back(std::move(share_callback));
 }
 
+void SmbShareFinder::InsertDiscoveryCallback(
+    HostDiscoveryResponse discovery_callback) {
+  discovery_callbacks_.push_back(std::move(discovery_callback));
+}
+
 }  // namespace smb_client
 }  // namespace chromeos

chrome/browser/chromeos/smb_client/smb_share_finder.h

@@ -64,11 +64,6 @@ class SmbShareFinder : public base::SupportsWeakPtr<SmbShareFinder> {
   bool TryResolveUrl(const SmbUrl& url, std::string* updated_url) const;
 
  private:
-  // Handles the response from discovering hosts in the network.
-  void OnHostsDiscovered(HostDiscoveryResponse discovery_callback,
-                         bool success,
-                         const HostMap& hosts);
-
   // Handles the response from finding hosts in the network.
   void OnHostsFound(bool success, const HostMap& hosts);
 
@@ -96,6 +91,9 @@ class SmbShareFinder : public base::SupportsWeakPtr<SmbShareFinder> {
   // Inserts |shares_callback| to |share_callbacks_|.
   void InsertShareCallback(GatherSharesInNetworkResponse shares_callback);
 
+  // Inserts |discovery_callback| to |discovery_callbacks_|.
+  void InsertDiscoveryCallback(HostDiscoveryResponse discovery_callback);
+
   NetworkScanner scanner_;
 
   SmbProviderClient* client_;  // Not owned.