Use SmbUrl where appropriate instead of regressing to std::string

Functions which take an SmbUrl should also return an SmbUrl instead of
std::string. This allows strong typing to be maintained, and helps avoid
conversions between SmbUrl and std::string.

Bug: 939235
Change-Id: If52ccc58d3bff62902179c1c9999fbf8bff64964
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2035827Reviewed-by: Austin Tankiang <austinct@chromium.org>
Commit-Queue: Anand Mistry <amistry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#738498}

chrome/browser/chromeos/smb_client/smb_service.cc

@@ -414,9 +414,8 @@ void SmbService::MountInternal(
   } else {
     // If using kerberos, the hostname should not be resolved since kerberos
     // service tickets are keyed on hosname.
-    const std::string url = use_kerberos
-                                ? share_url.ToString()
-                                : share_finder_->GetResolvedUrl(share_url);
+    const SmbUrl url =
+        use_kerberos ? share_url : share_finder_->GetResolvedUrl(share_url);
 
     SmbProviderClient::MountOptions smb_mount_options;
     smb_mount_options.original_path = share_url.ToString();
@@ -427,7 +426,8 @@ void SmbService::MountInternal(
     smb_mount_options.account_hash = user->username_hash();
     smb_mount_options.skip_connect = skip_connect;
     GetSmbProviderClient()->Mount(
-        base::FilePath(url), smb_mount_options, MakeFdWithContents(password),
+        base::FilePath(url.ToString()), smb_mount_options,
+        MakeFdWithContents(password),
         base::BindOnce(&SmbService::OnProviderMountDone, AsWeakPtr(),
                        std::move(callback), options, save_credentials));
   }
@@ -552,9 +552,9 @@ void SmbService::OnHostsDiscoveredForUpdateSharePath(
     int32_t mount_id,
     const std::string& share_path,
     StartReadDirIfSuccessfulCallback reply) {
-  std::string resolved_url;
+  SmbUrl resolved_url(share_path);
   if (share_finder_->TryResolveUrl(SmbUrl(share_path), &resolved_url)) {
-    UpdateSharePath(mount_id, resolved_url, std::move(reply));
+    UpdateSharePath(mount_id, resolved_url.ToString(), std::move(reply));
   } else {
     std::move(reply).Run(false /* should_retry_start_read_dir */);
   }
@@ -594,10 +594,9 @@ void SmbService::Remount(const ProvidedFileSystemInfo& file_system_info) {
 
   // If using kerberos, the hostname should not be resolved since kerberos
   // service tickets are keyed on hosname.
-  const base::FilePath mount_path =
-      is_kerberos_chromad
-          ? base::FilePath(parsed_url.ToString())
-          : base::FilePath(share_finder_->GetResolvedUrl(parsed_url));
+  const SmbUrl resolved_url = is_kerberos_chromad
+                                  ? parsed_url
+                                  : share_finder_->GetResolvedUrl(parsed_url);
 
   // An empty password is passed to Mount to conform with the credentials API
   // which expects username & workgroup strings along with a password file
@@ -612,7 +611,8 @@ void SmbService::Remount(const ProvidedFileSystemInfo& file_system_info) {
       !username.empty() && !is_kerberos_chromad;
   smb_mount_options.account_hash = user->username_hash();
   GetSmbProviderClient()->Mount(
-      mount_path, smb_mount_options, MakeFdWithContents(""),
+      base::FilePath(resolved_url.ToString()), smb_mount_options,
+      MakeFdWithContents(""),
       base::BindOnce(&SmbService::OnRemountResponse, AsWeakPtr(),
                      file_system_info.file_system_id()));
 }
@@ -859,7 +859,7 @@ void SmbService::RequestUpdatedSharePath(
   }
   // Host discovery did not run, but try to resolve the hostname in case a
   // previous host discovery found the host.
-  std::string resolved_url;
+  SmbUrl resolved_url(share_path);
   if (share_finder_->TryResolveUrl(SmbUrl(share_path), &resolved_url)) {
     UpdateSharePath(mount_id, share_path, std::move(reply));
   } else {

chrome/browser/chromeos/smb_client/smb_share_finder.cc

@@ -84,23 +84,23 @@ void SmbShareFinder::RegisterHostLocator(std::unique_ptr<HostLocator> locator) {
   scanner_.RegisterHostLocator(std::move(locator));
 }
 
-std::string SmbShareFinder::GetResolvedUrl(const SmbUrl& url) const {
+SmbUrl SmbShareFinder::GetResolvedUrl(const SmbUrl& url) const {
   DCHECK(url.IsValid());
 
   const std::string ip_address = scanner_.ResolveHost(url.GetHost()).ToString();
   // Return the original URL if the resolved host cannot be found or if there is
   // no change in the resolved IP address.
   if (ip_address.empty() || ip_address == url.GetHost()) {
-    return url.ToString();
+    return url;
   }
 
   return url.ReplaceHost(ip_address);
 }
 
 bool SmbShareFinder::TryResolveUrl(const SmbUrl& url,
-                                   std::string* updated_url) const {
+                                   SmbUrl* updated_url) const {
   *updated_url = GetResolvedUrl(url);
-  return *updated_url != url.ToString();
+  return updated_url->ToString() != url.ToString();
 }
 
 void SmbShareFinder::OnHostsFound(bool success, const HostMap& hosts) {

chrome/browser/chromeos/smb_client/smb_share_finder.h

@@ -50,13 +50,13 @@ class SmbShareFinder : public base::SupportsWeakPtr<SmbShareFinder> {
   void RegisterHostLocator(std::unique_ptr<HostLocator> locator);
 
   // Attempts to resolve |url|. Returns the resolved url if successful,
-  // otherwise returns ToString of |url|.
-  std::string GetResolvedUrl(const SmbUrl& url) const;
+  // otherwise returns |url|.
+  SmbUrl GetResolvedUrl(const SmbUrl& url) const;
 
   // Attempts to resolve |url|. If able to resolve |url|, returns true and sets
   // |resolved_url| the the resolved url. If unable, returns false and sets
-  // |resolved_url| to ToString of |url|.
-  bool TryResolveUrl(const SmbUrl& url, std::string* updated_url) const;
+  // |resolved_url| to |url|.
+  bool TryResolveUrl(const SmbUrl& url, SmbUrl* updated_url) const;
 
  private:
   // Handles the response from finding hosts in the network.

chrome/browser/chromeos/smb_client/smb_share_finder_unittest.cc

@@ -114,7 +114,7 @@ class SmbShareFinderTest : public testing::Test {
 
   // Helper function that expects |url| to resolve to |expected|.
   void ExpectResolvedHost(const SmbUrl& url, const std::string& expected) {
-    EXPECT_EQ(expected, share_finder_->GetResolvedUrl(url));
+    EXPECT_EQ(expected, share_finder_->GetResolvedUrl(url).ToString());
   }
 
   void ExpectDiscoveryCalled(int32_t expected) {

chrome/browser/chromeos/smb_client/smb_url.cc

@@ -74,6 +74,10 @@ SmbUrl::~SmbUrl() = default;
 
 SmbUrl::SmbUrl(SmbUrl&& smb_url) = default;
 
+SmbUrl::SmbUrl(const SmbUrl& smb_url) = default;
+
+SmbUrl& SmbUrl::operator=(const SmbUrl& smb_url) = default;
+
 std::string SmbUrl::GetHost() const {
   DCHECK(IsValid());
 
@@ -92,12 +96,14 @@ const std::string& SmbUrl::ToString() const {
   return url_;
 }
 
-std::string SmbUrl::ReplaceHost(const std::string& new_host) const {
+SmbUrl SmbUrl::ReplaceHost(const std::string& new_host) const {
   DCHECK(IsValid());
 
   std::string temp = url_;
   temp.replace(host_.begin, host_.len, new_host);
-  return temp;
+  SmbUrl new_url(temp);
+  DCHECK(new_url.IsValid());
+  return new_url;
 }
 
 bool SmbUrl::IsValid() const {

chrome/browser/chromeos/smb_client/smb_url.h

@@ -7,7 +7,6 @@
 
 #include <string>
 
-#include "base/macros.h"
 #include "url/third_party/mozilla/url_parse.h"
 
 namespace chromeos {
@@ -24,6 +23,10 @@ class SmbUrl {
   ~SmbUrl();
   SmbUrl(SmbUrl&& smb_url);
 
+  // Allow copying.
+  SmbUrl(const SmbUrl& url);
+  SmbUrl& operator=(const SmbUrl& url);
+
   // Returns the host of the URL which can be resolved or unresolved.
   std::string GetHost() const;
 
@@ -35,7 +38,7 @@ class SmbUrl {
 
   // Replaces the host to |new_host| and returns the full URL. Does not
   // change the original URL.
-  std::string ReplaceHost(const std::string& new_host) const;
+  SmbUrl ReplaceHost(const std::string& new_host) const;
 
   // Returns true if the passed URL is valid and was properly parsed. This
   // should be called after the constructor.
@@ -66,8 +69,6 @@ class SmbUrl {
 
   // Share name component of the URL.
   std::string share_;
-
-  DISALLOW_COPY_AND_ASSIGN(SmbUrl);
 };
 
 }  // namespace smb_client

chrome/browser/chromeos/smb_client/smb_url_unittest.cc

@@ -6,6 +6,7 @@
 
 #include <string>
 
+#include "base/macros.h"
 #include "chrome/browser/chromeos/smb_client/smb_constants.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -111,7 +112,7 @@ TEST_F(SmbUrlTest, ReplacesHost) {
 
   const std::string new_host = "192.168.0.1";
   const std::string expected_url = "smb://192.168.0.1/share";
-  EXPECT_EQ(expected_url, smb_url.ReplaceHost(new_host));
+  EXPECT_EQ(expected_url, smb_url.ReplaceHost(new_host).ToString());
 
   // GetHost returns the original host.
   EXPECT_EQ(expected_host, smb_url.GetHost());