Implement basic USB setup in the cups printer detector. Factor out

chrome/browser/chromeos/printer_detector/cups_printer_detector.cc

Index: chrome/browser/chromeos/printer_detector/cups_printer_detector.cc
diff --git a/chrome/browser/chromeos/printer_detector/cups_printer_detector.cc b/chrome/browser/chromeos/printer_detector/cups_printer_detector.cc
index 4532256dc3c09642ac36568617d8fce6372f67d8..b75e0a6ec4a85d0b6ec7279210eeeb447b955a30 100644
--- a/chrome/browser/chromeos/printer_detector/cups_printer_detector.cc
+++ b/chrome/browser/chromeos/printer_detector/cups_printer_detector.cc
@@ -15,17 +15,17 @@
 #include "base/strings/utf_string_conversions.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/chromeos/printer_detector/printer_detector.h"
+#include "chrome/browser/chromeos/printing/ppd_provider_factory.h"
+#include "chrome/browser/chromeos/printing/printer_configurer.h"
+#include "chrome/browser/chromeos/printing/printers_manager_factory.h"
+#include "chrome/browser/chromeos/printing/usb_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
-#include "chrome/browser/notifications/notification.h"
-#include "chrome/browser/notifications/notification_delegate.h"
-#include "chrome/browser/notifications/notification_ui_manager.h"
-#include "chrome/browser/ui/browser_navigator.h"
-#include "chrome/browser/ui/browser_navigator_params.h"
-#include "chrome/common/url_constants.h"
-#include "chrome/grit/generated_resources.h"
-#include "chrome/grit/theme_resources.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/debug_daemon_client.h"
+#include "chromeos/printing/ppd_provider.h"
 #include "components/user_manager/user.h"
 #include "components/user_manager/user_manager.h"
+#include "content/public/browser/browser_thread.h"
 #include "device/base/device_client.h"
 #include "device/usb/usb_device.h"
 #include "device/usb/usb_device_filter.h"
@@ -33,46 +33,45 @@
 #include "extensions/browser/extension_registry.h"

[tbarzic, 2017/03/15 18:43:42]

this is not needed anymore, right?

[Carlson, 2017/03/15 23:06:16]

Done.

 #include "extensions/browser/extension_system.h"
 #include "extensions/common/one_shot_event.h"
-#include "ui/base/l10n/l10n_util.h"
-#include "ui/base/resource/resource_bundle.h"
 
 namespace chromeos {
 namespace {
 
-const char kUSBPrinterFoundNotificationID[] =
-    "chrome://settings/cupsPrinters/printer_found";
+using printing::PpdProvider;
 
-// Base class used for printer USB interfaces
-// (https://www.usb.org/developers/defined_class).
-const uint8_t kPrinterInterfaceClass = 7;
+// We have to carry quite a bit of data through to the callbacks for
+// getting a printer set up.  Rather than have a bunch of parameters
+// in a bunch of functions below, this struct aggregates everything

[tbarzic, 2017/03/15 18:43:42]

This comment could be a lot shorter :)
E.g.
Aggregates the information needed for printer setup so it's easier to pass it
around.

[Carlson, 2017/03/15 23:06:16]

Done.

+// we need to keep around during the setup.
+struct SetUpPrinterData {
+  // The configurer running the SetUpPrinter call.
+  std::unique_ptr<PrinterConfigurer> configurer;
 
-// USBPrinterSetupNotificationDelegate takes a pointer to the Impl class, so
-// we have to forward declare it.
-class CupsPrinterDetectorImpl;
+  // The printer being set up.
+  std::unique_ptr<Printer> printer;
 
-class USBPrinterSetupNotificationDelegate : public NotificationDelegate {
- public:
-  explicit USBPrinterSetupNotificationDelegate(
-      CupsPrinterDetectorImpl* printer_detector)
-      : printer_detector_(printer_detector) {}
-
-  // NotificationDelegate override:
-  std::string id() const override { return kUSBPrinterFoundNotificationID; }
-
-  // This is defined out of line because it needs the PrinterDetectorImpl
-  // full class declaration, not just the forward declaration.
-  void ButtonClick(int button_index) override;
-
-  bool HasClickedListener() override { return true; }
-
- private:
-  ~USBPrinterSetupNotificationDelegate() override = default;
+  // The usb device causing this setup flow.
+  scoped_refptr<device::UsbDevice> device;
 
-  CupsPrinterDetectorImpl* printer_detector_;
+  // True if this printer is one that the user doesn't already have a
+  // configuration for.
+  bool is_new;
 
-  DISALLOW_COPY_AND_ASSIGN(USBPrinterSetupNotificationDelegate);
+  // True if this was a printer that was plugged in during the session, false if
+  // it was already plugged in when the session started.
+  bool hotplugged;
 };
 
+// Given a usb device, guess the make and model for a driver lookup.

[tbarzic, 2017/03/15 18:43:42]

nit: s/guess/guesses/ 

[Carlson, 2017/03/15 23:06:16]

Done.

+//
+// TODO(justincarlson): Possibly go deeper and query the IEEE1284 fields
+// for make and model if we determine those are more likely to contain
+// what we want.
+std::string GuessEffectiveMakeAndModel(const device::UsbDevice& device) {
+  return base::UTF16ToUTF8(device.manufacturer_string()) + " " +
+         base::UTF16ToUTF8(device.product_string());
+}
+
 // The PrinterDetector that drives the flow for setting up a USB printer to use
 // CUPS backend.
 class CupsPrinterDetectorImpl : public PrinterDetector,
@@ -80,155 +79,187 @@ class CupsPrinterDetectorImpl : public PrinterDetector,
  public:
   explicit CupsPrinterDetectorImpl(Profile* profile)
       : profile_(profile), observer_(this), weak_ptr_factory_(this) {
-    extensions::ExtensionSystem::Get(profile)->ready().Post(
-        FROM_HERE, base::Bind(&CupsPrinterDetectorImpl::Initialize,
-                              weak_ptr_factory_.GetWeakPtr()));
-  }
-  ~CupsPrinterDetectorImpl() override = default;
-
-  void ClickOnNotificationButton(int button_index) {
-    // Remove the old notification first.
-    const ProfileID profile_id = NotificationUIManager::GetProfileID(profile_);
-    g_browser_process->notification_ui_manager()->CancelById(
-        kUSBPrinterFoundNotificationID, profile_id);
-
-    if (command_ == ButtonCommand::SETUP) {
-      OnSetUpUSBPrinterStarted();
-      // TODO(skau/xdai): call the CUPS backend to set up the USB printer and
-      // then call OnSetUpPrinterDone() or OnSetUpPrinterError() depending on
-      // the setup result.
-    } else if (command_ == ButtonCommand::CANCEL_SETUP) {
-      // TODO(skau/xdai): call the CUPS backend to cancel the printer setup.
-    } else if (command_ == ButtonCommand::GET_HELP) {
-      chrome::NavigateParams params(profile_,
-                                    GURL(chrome::kChromeUIMdCupsSettingsURL),
-                                    ui::PAGE_TRANSITION_LINK);
-      params.disposition = WindowOpenDisposition::NEW_FOREGROUND_TAB;
-      params.window_action = chrome::NavigateParams::SHOW_WINDOW;
-      chrome::Navigate(&params);
-    }
-  }
-
- private:
-  // Action that should be performed when a notification button is clicked.
-  enum class ButtonCommand {
-    SETUP,
-    CANCEL_SETUP,
-    CLOSE,
-    GET_HELP,
-  };
-
-  // UsbService::observer override:
-  void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override {
     const user_manager::User* user =
         ProfileHelper::Get()->GetUserByProfile(profile_);
+    // Don't bother to listen for USB events unless we are the active user.
+    //
     // TODO(justincarlson) - See if it's appropriate to relax any of these
     // constraints.
     if (!user || !user->HasGaiaAccount() || !user_manager::UserManager::Get() ||
         user != user_manager::UserManager::Get()->GetActiveUser()) {
       return;
     }
+    device::UsbService* usb_service =
+        device::DeviceClient::Get()->GetUsbService();
+    if (usb_service) {
+      observer_.Add(usb_service);
+      usb_service->GetDevices(base::Bind(&CupsPrinterDetectorImpl::OnGetDevices,
+                                         weak_ptr_factory_.GetWeakPtr()));
+    }
+  }
+  ~CupsPrinterDetectorImpl() override = default;
 
-    device::UsbDeviceFilter printer_filter;
-    printer_filter.interface_class = kPrinterInterfaceClass;
-    if (!printer_filter.Matches(device))
-      return;
+ private:
+  // Callback for initial enumeration of usb devices.  UsbService invokes
+  // this on the UI thread.
+  void OnGetDevices(
+      const std::vector<scoped_refptr<device::UsbDevice>>& devices) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    for (const auto& device : devices) {
+      MaybeSetUpDevice(device, false);
+    }
+  }
 
-    ShowUSBPrinterSetupNotification(device);
+  // UsbService::observer override.  This runs on the UI thread.
+  void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    MaybeSetUpDevice(device, true);
   }
 
-  // Initializes the printer detector.
-  void Initialize() {
-    device::UsbService* usb_service =
-        device::DeviceClient::Get()->GetUsbService();
-    if (!usb_service)
+  // UsbService::observer override.  This runs on the UI thread.
+  void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    if (!UsbDeviceIsPrinter(device)) {
       return;
-    observer_.Add(usb_service);
+    }
+    known_printers_.erase(device->guid());
+
+    // If this printer appears in the list of printers we've failed to set up,
+    // remove it.
+    failed_setup_printers_.erase(device->guid());
   }
 
-  void SetNotificationUIManagerForTesting(
-      NotificationUIManager* manager) override {
-    LOG(FATAL) << "Not implemented for CUPS";
+  // This is called on the UI thread.
+  void MaybeSetUpDevice(scoped_refptr<device::UsbDevice> device,
+                        bool hotplugged) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+    if (!UsbDeviceIsPrinter(device) ||
+        base::ContainsKey(known_printers_, device->guid())) {
+      return;
+    }
+    known_printers_.insert(device->guid());
+
+    auto data = base::MakeUnique<SetUpPrinterData>();
+    data->configurer = PrinterConfigurer::Create(profile_);
+    data->printer = base::MakeUnique<Printer>();

[tbarzic, 2017/03/15 18:43:42]

This still seems a little convoluted :/
Could we introduce a method GetUsbPrinterUri(const UsbDevice& device), and
change the flow to something like:

bool is_new = false;
std::unique_ptr<Printer>printer = GetPrinterForDevice(*device, &is_new);
if (!data->printer)
  return;
// Do stuff with printer.

GetPrinterForDevice(const UsbDevice& device, bool* is_new) {
  std::string uri = GetUsbPrinterUri(device);
  if (uri.empty())
    return nullptr;

  std::unique_ptr<Printer> = FindPrinterByUri(uri);
  if (printer) {
    *is_new = false;
    return printer;
  }

  *is_new = true;
  return UsbDeviceToPrinter(device);
}

std::unique_ptr<Printer> FindPrinterByUri(const std::string& uri) {
  for (printer : GetPrinters())
    if (printer->uri() == uri)
      return std::move(printer);
}

[Carlson, 2017/03/15 23:06:16]

Took some of these ideas to try to clarify, but did it a little differently. 
Let me know what you think.

[tbarzic, 2017/03/16 04:11:29]

looks good

+    data->device = device;
+    data->is_new = true;
+    data->hotplugged = hotplugged;
+    if (!UsbDeviceToPrinter(*device, data->printer.get())) {
+      LOG(ERROR)

[tbarzic, 2017/03/15 18:43:42]

UsbDeviceToPrinter already logs an error

[Carlson, 2017/03/15 23:06:16]

Done.

+          << "Failed to convert usb device to printer, abandoning setup.  "
+          << "Usb device details follow:";
+      LOG(ERROR) << UsbPrinterDeviceDetailsAsString(*device);
+      return;
+    }
+    // TODO(justincarlson): add a GetPrinterByURI to PrintersManager.
+    // https://crbug.com/700602
+    auto existing_printers =
+        PrintersManagerFactory::GetForBrowserContext(profile_)->GetPrinters();
+    for (std::unique_ptr<Printer>& printer : existing_printers) {
+      if (printer->uri() == data->printer->uri()) {
+        // Found a match, so use the existing configuration.
+        *(data->printer) = *printer;
+        data->is_new = false;
+        OnPrinterResolved(std::move(data));
+        return;
+      }
+    }
+
+    // It's not a device we know about.  First we see if we can get an exact
+    // driver match based on USB ids.
+    //
+    // TODO(justincarlson): Add a notification that we are attempting to set up
+    // this printer at this point.
+    scoped_refptr<PpdProvider> ppd_provider =
+        printing::CreateProvider(profile_);
+    ppd_provider->ResolveUsbIds(
+        device->vendor_id(), device->product_id(),
+        base::Bind(&CupsPrinterDetectorImpl::ResolveUsbIdsDone,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   base::Passed(std::move(data))));
   }
 
-  void ShowUSBPrinterSetupNotification(
-      scoped_refptr<device::UsbDevice> device) {
-    // TODO(justincarlson) - Test this notification across a wide variety of
-    // less-than-sane printers to make sure the notification text stays as
-    // intelligible as possible.
-    base::string16 printer_name = device->manufacturer_string() +
-                                  base::UTF8ToUTF16(" ") +
-                                  device->product_string();
-    ui::ResourceBundle& bundle = ui::ResourceBundle::GetSharedInstance();
-    message_center::RichNotificationData data;
-    data.buttons.push_back(message_center::ButtonInfo(l10n_util::GetStringUTF16(
-        IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_BUTTON)));
-    notification_.reset(new Notification(
-        message_center::NOTIFICATION_TYPE_SIMPLE,
-        l10n_util::GetStringUTF16(
-            IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_TITLE),      // title
-        printer_name,                                             // body
-        bundle.GetImageNamed(IDR_PRINTER_DETECTED_NOTIFICATION),  // icon
-        message_center::NotifierId(message_center::NotifierId::SYSTEM_COMPONENT,
-                                   kUSBPrinterFoundNotificationID),
-        base::string16(),  // display_source
-        GURL(), kUSBPrinterFoundNotificationID, data,
-        new USBPrinterSetupNotificationDelegate(this)));
-    notification_->SetSystemPriority();
-    command_ = ButtonCommand::SETUP;
-
-    g_browser_process->notification_ui_manager()->Add(*notification_, profile_);
+  void OnPrinterResolved(std::unique_ptr<SetUpPrinterData> data) {
+    // Copy fields that will be invalidated by std::move.
+    auto* configurer_tmp = data->configurer.get();
+    const Printer& printer_tmp = *(data->printer);
+    // off to the configurer.
+    configurer_tmp->SetUpPrinter(
+        printer_tmp, base::Bind(&CupsPrinterDetectorImpl::SetUpPrinterDone,
+                                weak_ptr_factory_.GetWeakPtr(),
+                                base::Passed(std::move(data))));
   }
 
-  void OnSetUpUSBPrinterStarted() {
-    notification_->set_title(l10n_util::GetStringUTF16(
-        IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_IN_PROGRESS_TITLE));
-    notification_->set_type(message_center::NOTIFICATION_TYPE_PROGRESS);
-    notification_->set_progress(-1);
-    std::vector<message_center::ButtonInfo> buttons;
-    buttons.push_back(message_center::ButtonInfo(l10n_util::GetStringUTF16(
-        IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_CANCEL_BUTTON)));
-    notification_->set_buttons(buttons);
-    command_ = ButtonCommand::CANCEL_SETUP;
-    g_browser_process->notification_ui_manager()->Add(*notification_, profile_);
+  // Called when the query for a driver based on usb ids completes.
+  void ResolveUsbIdsDone(std::unique_ptr<SetUpPrinterData> data,
+                         PpdProvider::CallbackResultCode result,
+                         const std::string& effective_make_and_model) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    if (result == PpdProvider::SUCCESS) {
+      // Got something based on usb ids.  Go with it.
+      data->printer->mutable_ppd_reference()->effective_make_and_model =
+          effective_make_and_model;
+    } else {
+      // Couldn't figure this printer out based on usb ids, fall back to
+      // guessing the make/model string from what the USB system reports.
+      //
+      // TODO(justincarlson): Consider adding a mechanism for aggregating data
+      // about which usb devices are in the wild but unsupported?
+      data->printer->mutable_ppd_reference()->effective_make_and_model =
+          GuessEffectiveMakeAndModel(*data->device);
+    }
+    OnPrinterResolved(std::move(data));

[tbarzic, 2017/03/15 18:43:42]

I wonder if it would make sense to register new printers with PrintersManager at
this point so the resolved data is not lost in case of a failure (given that it
seems that printers in PrintersManager are not necessarily set up); though I'm
not familiar with PrintersManager semantics enough to know whether that would be
the right thing to do.

[Carlson, 2017/03/15 23:06:16]

There's a reasonable argument to be made for that, but I think that's not the
right thing here.  PrintersManager has to cope with bad printers, as printers
may break or disappear, but I don't think there's value in having a USB printer
enter the PrintersManager list until we can print to it.  

Note that there's a separate mechanism I'm laying foundation for here to be able
to retrieve the list of printers we *failed* to set up, so we can show those to
the user in the settings dialog as printers that we can try to manually set up.

[tbarzic, 2017/03/16 04:11:28]

sounds good

   }
 
-  void OnSetUpUSBPrinterDone() {
-    notification_->set_title(l10n_util::GetStringUTF16(
-        IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_SUCCESS_TITLE));
-    notification_->set_type(message_center::NOTIFICATION_TYPE_SIMPLE);
-    std::vector<message_center::ButtonInfo> buttons;
-    buttons.push_back(message_center::ButtonInfo(l10n_util::GetStringUTF16(
-        IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_CLOSE_BUTTON)));
-    notification_->set_buttons(buttons);
-    command_ = ButtonCommand::CLOSE;
-    g_browser_process->notification_ui_manager()->Add(*notification_, profile_);
+  // Called on the UI thread with the result of asking to have a printer
+  // configured for CUPS.  If |printer_to_register| is non-null and we
+  // successfully configured, then the printer is registered with the printers
+  // manager.
+  void SetUpPrinterDone(std::unique_ptr<SetUpPrinterData> data,
+                        PrinterSetupResult result) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    if (result == PrinterSetupResult::SUCCESS) {
+      if (data->is_new) {
+        PrintersManagerFactory::GetForBrowserContext(profile_)->RegisterPrinter(
+            std::move(data->printer));
+      }
+      if (data->hotplugged) {
+        // TODO(justincarlson): Pop a timed notification that says the printer
+        // is now available for printing.
+      }
+    } else {
+      // TODO(justincarlson): Pop a notification that tells the user automatic
+      // setup failed and offers to open the CUPS printer configuration
+      // settings.
+      DCHECK(!base::ContainsKey(failed_setup_printers_, data->device->guid()));
+      failed_setup_printers_.insert({data->device->guid(), *(data->printer)});
+    }
   }
 
-  void OnSetUpUSBPrinterError() {
-    notification_->set_title(l10n_util::GetStringUTF16(
-        IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_FAILED_TITLE));
-    notification_->set_type(message_center::NOTIFICATION_TYPE_SIMPLE);
-    std::vector<message_center::ButtonInfo> buttons;
-    buttons.push_back(message_center::ButtonInfo(l10n_util::GetStringUTF16(
-        IDS_PRINTER_DETECTED_NOTIFICATION_SET_UP_GET_HELP_BUTTON)));
-    notification_->set_buttons(buttons);
-    command_ = ButtonCommand::GET_HELP;
-    g_browser_process->notification_ui_manager()->Add(*notification_, profile_);
+  void SetNotificationUIManagerForTesting(
+      NotificationUIManager* manager) override {
+    LOG(FATAL) << "Not implemented for CUPS";
   }
 
-  std::unique_ptr<Notification> notification_;
-  ButtonCommand command_ = ButtonCommand::SETUP;
+  // USB GUIDs of printers we've already dealt with.  There's an inherent race
+  // between initially querying all usb devices and receiving a notification
+  // about a new device, so this set lets us guarantee that we handle a given
+  // printer exactly once.
+  std::set<std::string> known_printers_;
+
+  // Printers that this class failed to set up automatically.  This is provided
+  // to the PrinterDiscoverer used in the settings flow to determine which
+  // printers are available to be set up.  This is a map from the USB GUID
+  // to Printer structure.
+  std::unordered_map<std::string, Printer> failed_setup_printers_;
 
   Profile* profile_;
   ScopedObserver<device::UsbService, device::UsbService::Observer> observer_;
   base::WeakPtrFactory<CupsPrinterDetectorImpl> weak_ptr_factory_;
 };
 
-void USBPrinterSetupNotificationDelegate::ButtonClick(int button_index) {
-  printer_detector_->ClickOnNotificationButton(button_index);
-}
-
 }  // namespace
 
 // static