Reland "Moving some relevant URI parsing functions..."

https://chromium-review.googlesource.com/c/chromium/src/+/861956 Fixing link error caused by the free ParseUri function in printer_configuration.h not being exported properly. This reverts commit 68d7f4c. Bug: 769893 Change-Id: I02a482272bf97e7166c221448dd2444bece00d03 Reviewed-on: https://chromium-review.googlesource.com/872265 Commit-Queue: David Valleau <valleau@chromium.org> Reviewed-by: Sean Kau <skau@chromium.org> Reviewed-by: Hector Carmona <hcarmona@chromium.org> Cr-Commit-Position: refs/heads/master@{#530404}
qweri0p · Jan 19, 2018 · 03e6fa6 · 03e6fa6
1 parent 8f3f15c
commit 03e6fa6
Show file tree

Hide file tree

Showing 8 changed files with 185 additions and 84 deletions.
diff --git a/chrome/browser/chromeos/printing/printer_configurer.cc b/chrome/browser/chromeos/printing/printer_configurer.cc
@@ -46,15 +46,6 @@ namespace chromeos {
 
 namespace {
 
-// Get the URI that we want for talking to cups.
-std::string URIForCups(const Printer& printer) {
-  if (!printer.effective_uri().empty()) {
-    return printer.effective_uri();
-  } else {
-    return printer.uri();
-  }
-}
-
 // Configures printers by downloading PPDs then adding them to CUPS through
 // debugd.  This class must be used on the UI thread.
 class PrinterConfigurerImpl : public PrinterConfigurer {
@@ -104,7 +95,7 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
 
     auto* client = DBusThreadManager::Get()->GetDebugDaemonClient();
     client->CupsAddAutoConfiguredPrinter(
-        printer.id(), URIForCups(printer),
+        printer.id(), printer.UriForCups(),
         base::BindOnce(&PrinterConfigurerImpl::OnAddedPrinter,
                        weak_factory_.GetWeakPtr(), printer,
                        std::move(callback)));
@@ -176,7 +167,7 @@ class PrinterConfigurerImpl : public PrinterConfigurer {
     auto* client = DBusThreadManager::Get()->GetDebugDaemonClient();
 
     client->CupsAddManuallyConfiguredPrinter(
-        printer.id(), URIForCups(printer), ppd_contents,
+        printer.id(), printer.UriForCups(), ppd_contents,
         base::BindOnce(&PrinterConfigurerImpl::OnAddedPrinter,
                        weak_factory_.GetWeakPtr(), printer, std::move(cb)));
   }
@@ -267,7 +258,7 @@ std::string PrinterConfigurer::SetupFingerprint(const Printer& printer) {
   base::MD5Context ctx;
   base::MD5Init(&ctx);
   base::MD5Update(&ctx, printer.id());
-  base::MD5Update(&ctx, URIForCups(printer));
+  base::MD5Update(&ctx, printer.UriForCups());
   base::MD5Update(&ctx, printer.ppd_reference().user_supplied_ppd_url);
   base::MD5Update(&ctx, printer.ppd_reference().effective_make_and_model);
   char autoconf = printer.ppd_reference().autoconf ? 1 : 0;

diff --git a/chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc b/chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc
@@ -33,41 +33,26 @@
 #include "chromeos/dbus/debug_daemon_client.h"
 #include "chromeos/printing/ppd_cache.h"
 #include "chromeos/printing/ppd_provider.h"
+#include "chromeos/printing/printer_configuration.h"
+#include "chromeos/printing/printing_constants.h"
+#include "chromeos/printing/uri_components.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/web_ui.h"
 #include "google_apis/google_api_keys.h"
 #include "net/base/filename_util.h"
 #include "net/url_request/url_request_context_getter.h"
 #include "printing/backend/print_backend.h"
-#include "url/third_party/mozilla/url_parse.h"
 
 namespace chromeos {
 namespace settings {
 
 namespace {
 
-constexpr char kIppScheme[] = "ipp";
-constexpr char kIppsScheme[] = "ipps";
-
-constexpr int kIppPort = 631;
-// IPPS commonly uses the HTTPS port despite the spec saying it should use the
-// IPP port.
-constexpr int kIppsPort = 443;
-
 // These values are written to logs.  New enum values can be added, but existing
 // enums must never be renumbered or deleted and reused.
 enum PpdSourceForHistogram { kUser = 0, kScs = 1, kPpdSourceMax };
 
-// A parsed representation of a printer uri.
-struct PrinterUri {
-  bool encrypted = false;
-  std::string scheme;
-  std::string host;
-  int port = url::SpecialPort::PORT_INVALID;
-  std::string path;
-};
-
 void RecordPpdSource(const PpdSourceForHistogram& source) {
   UMA_HISTOGRAM_ENUMERATION("Printing.CUPS.PpdSource", source, kPpdSourceMax);
 }
@@ -82,43 +67,6 @@ void RecordIppQuerySuccess(bool success) {
   UMA_HISTOGRAM_BOOLEAN("Printing.CUPS.IppAttributesSuccess", success);
 }
 
-// Parses |printer_uri| into its components and written into |uri|.  Returns
-// true if the uri was parsed successfully, returns false otherwise.  No changes
-// are made to |uri| if this function returns false.
-bool ParseUri(const std::string& printer_uri, PrinterUri* uri) {
-  DCHECK(uri);
-  const char* uri_ptr = printer_uri.c_str();
-  url::Parsed parsed;
-  url::ParseStandardURL(uri_ptr, printer_uri.length(), &parsed);
-  if (!parsed.scheme.is_valid() || !parsed.host.is_valid() ||
-      !parsed.path.is_valid()) {
-    return false;
-  }
-  base::StringPiece scheme(&uri_ptr[parsed.scheme.begin], parsed.scheme.len);
-  base::StringPiece host(&uri_ptr[parsed.host.begin], parsed.host.len);
-  base::StringPiece path(&uri_ptr[parsed.path.begin], parsed.path.len);
-
-  bool encrypted = scheme != kIppScheme;
-  int port = ParsePort(uri_ptr, parsed.port);
-  // Port not specified.
-  if (port == url::SpecialPort::PORT_UNSPECIFIED ||
-      port == url::SpecialPort::PORT_INVALID) {
-    if (scheme == kIppScheme) {
-      port = kIppPort;
-    } else if (scheme == kIppsScheme) {
-      port = kIppsPort;
-    }
-  }
-
-  uri->encrypted = encrypted;
-  uri->scheme = scheme.as_string();
-  uri->host = host.as_string();
-  uri->port = port;
-  uri->path = path.as_string();
-
-  return true;
-}
-
 // Returns true if |printer_uri| is an IPP uri.
 bool IsIppUri(base::StringPiece printer_uri) {
   base::StringPiece::size_type separator_location =
@@ -136,15 +84,17 @@ bool IsIppUri(base::StringPiece printer_uri) {
 // error to attempt this with a non-IPP printer.
 void QueryAutoconf(const std::string& printer_uri,
                    const PrinterInfoCallback& callback) {
-  PrinterUri uri;
+  auto optional = ParseUri(printer_uri);
   // Behavior for querying a non-IPP uri is undefined and disallowed.
-  if (!IsIppUri(printer_uri) || !ParseUri(printer_uri, &uri)) {
+  if (!IsIppUri(printer_uri) || !optional.has_value()) {
     LOG(WARNING) << "Printer uri is invalid: " << printer_uri;
     callback.Run(false, "", "", "", false);
     return;
   }
 
-  QueryIppPrinter(uri.host, uri.port, uri.path, uri.encrypted, callback);
+  UriComponents uri = optional.value();
+  QueryIppPrinter(uri.host(), uri.port(), uri.path(), uri.encrypted(),
+                  callback);
 }
 
 // Create an empty CupsPrinterInfo dictionary value. It should be consistent
@@ -193,8 +143,8 @@ std::unique_ptr<base::DictionaryValue> GetPrinterInfo(const Printer& printer) {
   printer_info->SetString("printerModel", printer.model());
   printer_info->SetString("printerMakeAndModel", printer.make_and_model());
 
-  PrinterUri uri;
-  if (!ParseUri(printer.uri(), &uri)) {
+  auto optional = printer.GetUriComponents();
+  if (!optional.has_value()) {
     // Uri is invalid so we set default values.
     LOG(WARNING) << "Could not parse uri.  Defaulting values";
     printer_info->SetString("printerAddress", "");
@@ -204,7 +154,9 @@ std::unique_ptr<base::DictionaryValue> GetPrinterInfo(const Printer& printer) {
     return printer_info;
   }
 
-  if (base::ToLowerASCII(uri.scheme) == "usb") {
+  UriComponents uri = optional.value();
+
+  if (base::ToLowerASCII(uri.scheme()) == "usb") {
     // USB has URI path (and, maybe, query) components that aren't really
     // associated with a queue -- the mapping between printing semantics and URI
     // semantics breaks down a bit here.  From the user's point of view, the
@@ -213,12 +165,12 @@ std::unique_ptr<base::DictionaryValue> GetPrinterInfo(const Printer& printer) {
                             printer.uri().substr(strlen("usb://")));
   } else {
     printer_info->SetString("printerAddress",
-                            PrinterAddress(uri.host, uri.port));
-    if (!uri.path.empty()) {
-      printer_info->SetString("printerQueue", uri.path.substr(1));
+                            PrinterAddress(uri.host(), uri.port()));
+    if (!uri.path().empty()) {
+      printer_info->SetString("printerQueue", uri.path().substr(1));
     }
   }
-  printer_info->SetString("printerProtocol", base::ToLowerASCII(uri.scheme));
+  printer_info->SetString("printerProtocol", base::ToLowerASCII(uri.scheme()));
 
   return printer_info;
 }
@@ -532,9 +484,17 @@ void CupsPrintersHandler::HandleAddCupsPrinter(const base::ListValue* args) {
   CHECK(args->GetDictionary(0, &printer_dict));
 
   std::unique_ptr<Printer> printer = DictToPrinter(*printer_dict);
-  PrinterUri uri;
-  if (!printer || !ParseUri(printer->uri(), &uri)) {
-    LOG(ERROR) << "Failed to parse printer";
+  if (!printer) {
+    LOG(ERROR) << "Failed to parse printer URI";
+    OnAddPrinterError(PrinterSetupResult::kFatalError);
+    return;
+  }
+
+  auto optional = printer->GetUriComponents();
+  if (!optional.has_value()) {
+    // If the returned optional does not contain a value then it means that the
+    // printer's uri was not able to be parsed successfully.
+    LOG(ERROR) << "Failed to parse printer URI";
     OnAddPrinterError(PrinterSetupResult::kFatalError);
     return;
   }
@@ -837,11 +797,17 @@ void CupsPrintersHandler::HandleAddDiscoveredPrinter(
   CHECK(args->GetString(0, &printer_id));
 
   std::unique_ptr<Printer> printer = printers_manager_->GetPrinter(printer_id);
-  PrinterUri uri;
-  if (printer == nullptr || !ParseUri(printer->uri(), &uri)) {
+  if (printer == nullptr) {
     // Printer disappeared, so we don't have information about it anymore and
-    // can't really do much.  Or the printer uri was not parsed successfully.
-    // Fail the add.
+    // can't really do much. Fail the add.
+    FireWebUIListener("on-add-cups-printer", base::Value(false),
+                      base::Value(printer_id));
+    return;
+  }
+
+  auto optional = printer->GetUriComponents();
+  if (!optional.has_value()) {
+    // The printer uri was not parsed successfully. Fail the add.
     FireWebUIListener("on-add-cups-printer", base::Value(false),
                       base::Value(printer_id));
     return;

diff --git a/chromeos/BUILD.gn b/chromeos/BUILD.gn
@@ -463,6 +463,8 @@ component("chromeos") {
     "printing/printer_configuration.h",
     "printing/printer_translator.cc",
     "printing/printer_translator.h",
+    "printing/uri_components.cc",
+    "printing/uri_components.h",
     "process_proxy/process_output_watcher.cc",
     "process_proxy/process_output_watcher.h",
     "process_proxy/process_proxy.cc",

diff --git a/chromeos/printing/printer_configuration.cc b/chromeos/printing/printer_configuration.cc
@@ -7,13 +7,46 @@
 #include <string>
 
 #include "base/guid.h"
+#include "base/optional.h"
 #include "base/strings/string_piece.h"
 #include "base/strings/string_util.h"
 #include "net/base/ip_endpoint.h"
+#include "url/third_party/mozilla/url_parse.h"
 #include "url/url_constants.h"
 
+#include "chromeos/printing/printing_constants.h"
+
 namespace chromeos {
 
+base::Optional<UriComponents> ParseUri(const std::string& printer_uri) {
+  const char* uri_ptr = printer_uri.c_str();
+  url::Parsed parsed;
+  url::ParseStandardURL(uri_ptr, printer_uri.length(), &parsed);
+  if (!parsed.scheme.is_valid() || !parsed.host.is_valid() ||
+      !parsed.path.is_valid()) {
+    return {};
+  }
+  base::StringPiece scheme(&uri_ptr[parsed.scheme.begin], parsed.scheme.len);
+  base::StringPiece host(&uri_ptr[parsed.host.begin], parsed.host.len);
+  base::StringPiece path(&uri_ptr[parsed.path.begin], parsed.path.len);
+
+  bool encrypted = scheme != kIppScheme;
+  int port = ParsePort(uri_ptr, parsed.port);
+  // Port not specified.
+  if (port == url::SpecialPort::PORT_UNSPECIFIED ||
+      port == url::SpecialPort::PORT_INVALID) {
+    if (scheme == kIppScheme) {
+      port = kIppPort;
+    } else if (scheme == kIppsScheme) {
+      port = kIppsPort;
+    }
+  }
+
+  return base::Optional<UriComponents>(base::in_place, encrypted,
+                                       scheme.as_string(), host.as_string(),
+                                       port, path.as_string());
+}
+
 namespace {
 
 // Returns the index of the first character representing the hostname in |uri|.
@@ -121,6 +154,18 @@ Printer::PrinterProtocol Printer::GetProtocol() const {
   return PrinterProtocol::kUnknown;
 }
 
+std::string Printer::UriForCups() const {
+  if (!effective_uri_.empty()) {
+    return effective_uri_;
+  } else {
+    return uri_;
+  }
+}
+
+base::Optional<UriComponents> Printer::GetUriComponents() const {
+  return chromeos::ParseUri(uri_);
+}
+
 bool Printer::PpdReference::operator==(
     const Printer::PpdReference& other) const {
   return user_supplied_ppd_url == other.user_supplied_ppd_url &&

diff --git a/chromeos/printing/printer_configuration.h b/chromeos/printing/printer_configuration.h
@@ -9,16 +9,26 @@
 #include <string>
 
 #include "base/macros.h"
+#include "base/optional.h"
 #include "base/time/time.h"
 #include "chromeos/chromeos_export.h"
 #include "net/base/host_port_pair.h"
+#include "url/third_party/mozilla/url_parse.h"
+
+#include "chromeos/printing/uri_components.h"
 
 namespace net {
 class IPEndPoint;
 }  // namespace net
 
 namespace chromeos {
 
+// Parses |printer_uri| into its components and returns an optional
+// UriComponents depending on whether or not |printer_uri| was parsed
+// successfully.
+CHROMEOS_EXPORT base::Optional<UriComponents> ParseUri(
+    const std::string& printer_uri);
+
 class CHROMEOS_EXPORT Printer {
  public:
   // Information needed to find the PPD file for this printer.
@@ -149,6 +159,14 @@ class CHROMEOS_EXPORT Printer {
   Source source() const { return source_; }
   void set_source(const Source source) { source_ = source; }
 
+  // Get the URI that we want for talking to cups.
+  std::string UriForCups() const;
+
+  // Parses the printers's uri into its components and returns an optional
+  // containing a UriComponents object depending on whether or not the uri was
+  // successfully parsed.
+  base::Optional<UriComponents> GetUriComponents() const;
+
  private:
   // Globally unique identifier. Empty indicates a new printer.
   std::string id_;

diff --git a/chromeos/printing/printing_constants.h b/chromeos/printing/printing_constants.h
@@ -12,6 +12,20 @@ namespace chromeos {
 // arbitrary, we just don't want to try to handle ridiculously huge files.
 constexpr size_t kMaxPpdSizeBytes = 250 * 1024;
 
+// Printing protocol schemes.
+constexpr char kIppScheme[] = "ipp";
+constexpr char kIppsScheme[] = "ipps";
+constexpr char kUsbScheme[] = "usb";
+constexpr char kHttpScheme[] = "http";
+constexpr char kHttpsScheme[] = "https";
+constexpr char kSocketScheme[] = "socket";
+constexpr char kLpdScheme[] = "lpd";
+
+constexpr int kIppPort = 631;
+// IPPS commonly uses the HTTPS port despite the spec saying it should use the
+// IPP port.
+constexpr int kIppsPort = 443;
+
 }  // namespace chromeos
 
 #endif  // CHROMEOS_PRINTING_PRINTING_CONSTANTS_H_
diff --git a/chromeos/printing/uri_components.cc b/chromeos/printing/uri_components.cc
@@ -0,0 +1,24 @@
+// Copyright 2017 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 "chromeos/printing/uri_components.h"
+
+#include "url/third_party/mozilla/url_parse.h"
+
+namespace chromeos {
+
+UriComponents::UriComponents(bool encrypted,
+                             const std::string& scheme,
+                             const std::string& host,
+                             int port,
+                             const std::string& path)
+    : encrypted_(encrypted),
+      scheme_(scheme),
+      host_(host),
+      port_(port),
+      path_(path) {}
+
+UriComponents::UriComponents(const UriComponents&) = default;
+
+}  // namespace chromeos