Store usb printer uri information properly

Before printerAddress contained the full uri path. Now the hostname and
path are properly stored in printerAddress, printerQueue respectively.

Also relax printer uri validation to allow printerQueue to contain both
the path and query portions of the uri.

BUG=chromium:1111654
TEST=Add brother printer to DUT via usb
TEST=chromeos_unittests --gtest_filter=PrinterTranslatorTest.*

Change-Id: I669a21e02b0fc485a00f1ad33d2f806a4c58042a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2331519Reviewed-by: Piotr Pawliczek <pawliczek@chromium.org>
Reviewed-by: James Cook <jamescook@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Commit-Queue: Pranav Batra <batrapranav@chromium.org>
Cr-Commit-Position: refs/heads/master@{#794622}

chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc

@@ -95,26 +95,6 @@ void RecordIppQueryResult(const PrinterQueryResult& result) {
   }
 }
 
-// Split the given |address| into host and port by the last occurrence of ':'.
-// If |address| has no ':', the |port| parameter is set to an empty string.
-// |host| and |port| cannot be nullptr.
-void SplitAddress(const std::string& address,
-                  std::string* host,
-                  std::string* port) {
-  DCHECK(host);
-  DCHECK(port);
-  const size_t pos = address.find_last_of(':');
-  if (pos == std::string::npos) {
-    // |address| has no ':' characters.
-    *host = address;
-    *port = "";
-    return;
-  }
-  *host = address.substr(0, pos);
-  // If |pos| points to the last character substr() returns an empty string.
-  *port = address.substr(pos + 1);
-}
-
 // Query an IPP printer to check for autoconf support where the printer is
 // located at |printer_uri|.  Results are reported through |callback|.  The
 // scheme of |printer_uri| must equal "ipp" or "ipps".
@@ -180,27 +160,14 @@ std::unique_ptr<chromeos::Printer> DictToPrinter(
   printer->set_make_and_model(printer_make_and_model);
   printer->set_print_server_uri(print_server_uri);
 
-  std::string printer_host;
-  std::string printer_port;
-  SplitAddress(printer_address, &printer_host, &printer_port);
-
-  Uri uri;
-  if (!uri.SetScheme(printer_protocol)) {
-    PRINTER_LOG(ERROR) << "Incorrect protocol: " << printer_protocol;
-    return nullptr;
-  }
-  if (!uri.SetHostEncoded(printer_host)) {
-    PRINTER_LOG(ERROR) << "Incorrect host: " << printer_host;
-    return nullptr;
-  }
-  if (!uri.SetPort(printer_port)) {
-    PRINTER_LOG(ERROR) << "Incorrect port: " << printer_port;
-    return nullptr;
-  }
-  if (!uri.SetPathEncoded(printer_queue)) {
-    PRINTER_LOG(ERROR) << "Incorrect path: " << printer_queue;
+  Uri uri(printer_protocol + url::kStandardSchemeSeparator + printer_address +
+          printer_queue);
+  if (uri.GetLastParsingError().status != Uri::ParserStatus::kNoErrors) {
+    PRINTER_LOG(ERROR) << "Uri parse error: "
+                       << static_cast<int>(uri.GetLastParsingError().status);
     return nullptr;
   }
+
   std::string message;
   if (!printer->SetUri(uri, &message)) {
     PRINTER_LOG(ERROR) << "Incorrect uri: " << message;
@@ -508,13 +475,9 @@ void CupsPrintersHandler::HandleGetPrinterInfo(const base::ListValue* args) {
   DCHECK(printer_protocol == kIppScheme || printer_protocol == kIppsScheme)
       << "Printer info requests only supported for IPP and IPPS printers";
 
-  std::string printer_host;
-  std::string printer_port;
-  SplitAddress(printer_address, &printer_host, &printer_port);
-
-  Uri uri;
-  if (!uri.SetScheme(printer_protocol) || !uri.SetHostEncoded(printer_host) ||
-      !uri.SetPort(printer_port) || !uri.SetPathEncoded(printer_queue) ||
+  Uri uri(printer_protocol + url::kStandardSchemeSeparator + printer_address +
+          printer_queue);
+  if (uri.GetLastParsingError().status != Uri::ParserStatus::kNoErrors ||
       !IsValidPrinterUri(uri)) {
     // Run the failure callback.
     OnAutoconfQueried(callback_id, PrinterQueryResult::UNKNOWN_FAILURE,

chromeos/printing/printer_translator.cc

@@ -15,6 +15,7 @@
 #include "chromeos/printing/cups_printer_status.h"
 #include "chromeos/printing/printer_configuration.h"
 #include "chromeos/printing/uri.h"
+#include "url/url_constants.h"
 
 using base::DictionaryValue;
 
@@ -199,23 +200,16 @@ std::unique_ptr<base::DictionaryValue> GetCupsPrinterInfo(
     return printer_info;
   }
 
-  if (printer.uri().GetScheme() == "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
-    // entire host/path/query block is the printer address for USB.
-    printer_info->SetString(
-        "printerAddress",
-        printer.uri().GetNormalized().substr(strlen("usb://")));
+  if (printer.uri().GetScheme() == "usb")
     printer_info->SetString("ppdManufacturer", printer.manufacturer());
-  } else {
-    printer_info->SetString("printerAddress", PrinterAddress(printer.uri()));
-    if (!printer.uri().GetPath().empty()) {
-      printer_info->SetString("printerQueue",
-                              printer.uri().GetPathEncodedAsString().substr(1));
-    }
-  }
   printer_info->SetString("printerProtocol", printer.uri().GetScheme());
+  printer_info->SetString("printerAddress", PrinterAddress(printer.uri()));
+  std::string printer_queue = printer.uri().GetPathEncodedAsString();
+  if (!printer_queue.empty())
+    printer_queue = printer_queue.substr(1);  // removes the leading '/'
+  if (!printer.uri().GetQueryEncodedAsString().empty())
+    printer_queue += "?" + printer.uri().GetQueryEncodedAsString();
+  printer_info->SetString("printerQueue", printer_queue);
 
   return printer_info;
 }

chromeos/printing/printer_translator_unittest.cc

@@ -301,7 +301,7 @@ TEST(PrinterTranslatorTest, GetCupsPrinterInfoGenericPrinterWithUsbUri) {
       GetCupsPrinterInfo(printer);
   CheckGenericPrinterInfo(CreateGenericPrinter(), *printer_info);
 
-  CheckPrinterInfoUri(*printer_info, "usb", "1234/af9d?serial=ink1", "");
+  CheckPrinterInfoUri(*printer_info, "usb", "1234", "af9d?serial=ink1");
 
   ExpectDictBooleanValue(false, *printer_info, "printerPpdReference.autoconf");
 }