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

chrome/browser/chromeos/printer_detector/cups_printer_detector.cc

 // 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 <stdint.h>
 
 #include <memory>
 #include <utility>
 #include <vector>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/macros.h"
 #include "base/scoped_observer.h"
 #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"
 #include "device/usb/usb_service.h"
 #include "extensions/browser/extension_registry.h"
 #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
+// 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) {}
+  // The usb device causing this setup flow.
+  scoped_refptr<device::UsbDevice> device;
 
-  // NotificationDelegate override:
-  std::string id() const override { return kUSBPrinterFoundNotificationID; }
+  // True if this printer is one that the user doesn't already have a
+  // configuration for.
+  bool is_new;
 
-  // 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;
+  // 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;
+};
 
-  bool HasClickedListener() override { return true; }
-
- private:
-  ~USBPrinterSetupNotificationDelegate() override = default;
-
-  CupsPrinterDetectorImpl* printer_detector_;
-
-  DISALLOW_COPY_AND_ASSIGN(USBPrinterSetupNotificationDelegate);
-};
+// Given a usb device, guess the make and model for a driver lookup.
+//
+// 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,
                                 public device::UsbService::Observer {
  public:
   explicit CupsPrinterDetectorImpl(Profile* profile)
       : profile_(profile), observer_(this), weak_ptr_factory_(this) {
     extensions::ExtensionSystem::Get(profile)->ready().Post(

[tbarzic, 2017/03/14 01:43:32]

Is dependency on extensions system needed?

[Carlson, 2017/03/15 00:31:15]

I believe so, though my understanding of this aspect of the system is rather
tenuous, so I could be wrong.

We rely on the PrintersManager code to be instantiated when usb device
processing starts.  This means that we don't want to actually start listening
for and processing events until the extension system has fully instantiated
everything.  And I think this is the right way to do that?

Let me know if I'm misunderstanding this. 

[skau, 2017/03/15 04:05:53]

In order for services to be cleaned up in the correct order, they declare a
dependency hierarchy.
https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...

[Carlson, 2017/03/15 16:45:25]

I hadn't seen that, thanks.  Added the PrintersManagerFactor dependency, removed
the ExtensionSystem one.

         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 {
+  // Called when the extension system is "ready", meaning everything has been
+  // created.
+  void Initialize() {
     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()) {

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

I don't think this is right. Active user could change, so in multiuser
environment, the number of running PrinterDetectors might end up being
inconsistent (but I think generally, this will be created when the profile is
associated with the active user).

The main purpose of this check was not to show same notifications for all logged
in users when a printer is plugged in.

Given that this does some background processing too, it might be better to
restrict this to primary user only, or have it run for all logged in users.
Though, it mainly depends on how we want the feature to work in multi user
environment.

I assume printers are set up per user/profile?

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

Ha, I'd meant to figure this out later, but sure, we can work it out now. 

I just dug through a lot of layers and I believe it should be fine to do this
for *any* profile at any time, including multiple profiles concurrently. 
Additionally, we *want* to run this flow for any user who might want to use the
printer, so I've removed this check entirely.

I think when we re-add some notifications about what's going on, it may be that
we revisit this in some way, but this is good for now, I think.

       return;
     }
-
-    device::UsbDeviceFilter printer_filter;
-    printer_filter.interface_class = kPrinterInterfaceClass;
-    if (!printer_filter.Matches(device))
+    device::UsbService* usb_service =
+        device::DeviceClient::Get()->GetUsbService();
+    if (!usb_service) {
       return;
-
-    ShowUSBPrinterSetupNotification(device);
+    }
+    observer_.Add(usb_service);
+    usb_service->GetDevices(base::Bind(&CupsPrinterDetectorImpl::OnGetDevices,
+                                       weak_ptr_factory_.GetWeakPtr()));
   }
 
-  // Initializes the printer detector.
-  void Initialize() {
-    device::UsbService* usb_service =
-        device::DeviceClient::Get()->GetUsbService();
-    if (!usb_service)
+  // Callback for initial enumeration of usb devices.  UsbService invokes
+  // this on the UI thread.

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

nit: I'd remove the second sentence - it's clear from the code.
(same goes for other instances where you have Thread DCHECK)

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

Not sure I agree completely with this, I think there's value in having the
calling requirements documented at the comment level, but ok, done.

+  void OnGetDevices(
+      const std::vector<scoped_refptr<device::UsbDevice>>& devices) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    for (const auto& device : devices) {
+      MaybeSetUpDevice(device, false);
+    }
+  }
+
+  // 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);
+  }
+
+  // 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());
+

[tbarzic, 2017/03/14 01:43:32]

What if the removed printer is being set up? Should the setup be cancelled in
that case?

[Carlson, 2017/03/15 00:31:15]

It should be harmless.  We'll end up setting up the printer for use with CUPS,
but doesn't require actually hitting the USB connection to complete.  

There is an open question of "how do we determine which printers are available
at print time so we don't display options that the user can't actually print
to", and one idea that has been tossed around is de-configuring devices on USB
removal, in which case yes, we'll have to handle that race elegantly.

If/when we do want to handle it, I suspect the right answer will be "defer the
removal processing until setup completes".

+    // If this printer appears in the list of printers we've failed to set up,
+    // remove it.

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

nit: I'd remove the comment - the code is clear enough.

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

Done.

+    failed_setup_printers_.erase(device->guid());
+  }
+
+  // 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>();

[tbarzic, 2017/03/14 01:43:32]

I wonder if you could introduce a helper class that does setup here, and have
known_printers map device GUID to the setup helper instead of passing this
struct around?
I think that would look a bit more streamlined.

[Carlson, 2017/03/15 00:31:15]

I really liked this idea and played with implementing it.  However, the object
lifetime issues get sort of complicated quickly, particularly around the
transfer of control from the helper class back to this class, and I think that,
as a result, it ends up being a net loss.

+    data->configurer = PrinterConfigurer::Create(profile_);
+    data->printer = base::MakeUnique<Printer>();
+    data->device = device;
+    data->is_new = true;
+    data->hotplugged = hotplugged;
+    if (!UsbDeviceToPrinter(*device, data->printer.get())) {
+      LOG(ERROR)
+          << "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.
+    // (bug 700602)

[tbarzic, 2017/03/14 01:43:32]

nit: https://crbug.com/700602
(instead of bug 700602)

[Carlson, 2017/03/15 00:31:15]

Done.

+    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;
+
+        // Copy fields that will be invalidated by std::move.
+        auto* configurer_tmp = data->configurer.get();

[tbarzic, 2017/03/14 01:43:32]

This is basically the same as what's done in ResolveUsbIdDone
Would it make sense to extract this into a separate method:
OnPrinterResolved();

[Carlson, 2017/03/15 00:31:15]

Done.

+        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))));
+        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(), ppd_provider,
+                   base::Passed(std::move(data))));
+  }
+
+  // Called when the query for a driver based on usb ids completes.
+  void ResolveUsbIdsDone(scoped_refptr<PpdProvider> provider,

[tbarzic, 2017/03/14 01:43:32]

is it necessary to pass provider here?

[Carlson, 2017/03/15 00:31:15]

Upon a lot of reflection, no.  I thought I needed it to keep the provider from
being deleted at the end of this function, but as I understand it, it should
stay alive until its last callback is done anyways, 

The sort of deep black magic of how lifetime extension effectively works through
the bind makes me pretty deeply uncomfortable, though.

+                         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);
+    }
+
+    // Have to keep copies of some parameters that will be moved in the
+    // arguments and used in the same statement.
+    auto* configurer = data->configurer.get();
+    auto* printer = data->printer.get();
+
+    configurer->SetUpPrinter(
+        *printer, base::Bind(&CupsPrinterDetectorImpl::SetUpPrinterDone,
+                             weak_ptr_factory_.GetWeakPtr(),
+                             base::Passed(std::move(data))));
+  }
+
+  // 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 SetNotificationUIManagerForTesting(
       NotificationUIManager* manager) override {
     LOG(FATAL) << "Not implemented for CUPS";
   }
 
-  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;
+  // 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_;
 
-    g_browser_process->notification_ui_manager()->Add(*notification_, profile_);
-  }
-
-  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_);
-  }
-
-  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_);
-  }
-
-  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_);
-  }
-
-  std::unique_ptr<Notification> notification_;
-  ButtonCommand command_ = ButtonCommand::SETUP;
+  // 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
 std::unique_ptr<PrinterDetector> PrinterDetector::CreateCups(Profile* profile) {
   return base::MakeUnique<CupsPrinterDetectorImpl>(profile);
 }
 
 }  // namespace chromeos