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/memory/ptr_util.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"

[stevenjb, 2017/03/17 17:34:10]

nit: I don't know if our precommit allows it, but it would be nice if we can put
this include first since it is the associated header for this class.
Alternatively put the class definition in a header for improved discoverability.

[Carlson, 2017/03/17 18:47:34]

Indeed, presubmit doesn't allow this, sadly.

I'm strongly opposed to putting implementation class declarations in a header as
the things that are in the implementation class but not in the interface
definition are explicitly local to this file, and putting them in a header makes
that less clear, weakening the interface abstraction.

+#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 "components/user_manager/user.h"
-#include "components/user_manager/user_manager.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/dbus/debug_daemon_client.h"
+#include "chromeos/printing/ppd_provider.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;
+// Aggregates the information needed for printer setup so it's easier to pass it
+// around.
+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, guesses 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(
-        FROM_HERE, base::Bind(&CupsPrinterDetectorImpl::Initialize,
-                              weak_ptr_factory_.GetWeakPtr()));
+    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;
 
-  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:
+  // Callback for initial enumeration of usb devices.
+  void OnGetDevices(
+      const std::vector<scoped_refptr<device::UsbDevice>>& devices) {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    for (const auto& device : devices) {
+      MaybeSetUpDevice(device, false);
     }
   }
 
- 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 {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    MaybeSetUpDevice(device, true);
+  }
 
-  // UsbService::observer override:
-  void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override {
-    const user_manager::User* user =
-        ProfileHelper::Get()->GetUserByProfile(profile_);
-    // 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()) {
+  // UsbService::observer override.
+  void OnDeviceRemoved(scoped_refptr<device::UsbDevice> device) override {
+    DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+    if (!UsbDeviceIsPrinter(device)) {
+      return;
+    }
+    known_printers_.erase(device->guid());
+    failed_setup_printers_.erase(device->guid());
+  }
+
+  // Returns the existing printer using this URI, if one exists, or
+  // null otherwise.
+  std::unique_ptr<Printer> FindExistingPrinter(const std::string& uri) {
+    // 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() == uri) {
+        // Found a match, so use the existing configuration.
+        return std::move(printer);
+      }
+    }
+    return nullptr;
+  }
+
+  // If this device is a printer and we haven't already tried to set it up,
+  // starts the process of setting the printer up.  |hotplugged|
+  // should be true if this was plugged in during the session.
+  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->device = device;
+    data->hotplugged = hotplugged;
+
+    data->printer = FindExistingPrinter(UsbPrinterUri(*device));
+    if (data->printer != nullptr) {
+      data->is_new = false;
+      OnPrinterResolved(std::move(data));
       return;
     }
 
-    device::UsbDeviceFilter printer_filter;
-    printer_filter.interface_class = kPrinterInterfaceClass;
-    if (!printer_filter.Matches(device))
+    // It's not a device we have configured previously.
+    //
+    // TODO(justincarlson): Add a notification that we are attempting to set up
+    // this printer at this point.
+    data->is_new = true;
+    data->printer = UsbDeviceToPrinter(*device);
+    if (data->printer == nullptr) {
       return;
+    }
 
-    ShowUSBPrinterSetupNotification(device);
+    // Look for an exact match based on USB ids.
+    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))));
   }
 
-  // Initializes the printer detector.
-  void Initialize() {
-    device::UsbService* usb_service =
-        device::DeviceClient::Get()->GetUsbService();
-    if (!usb_service)
-      return;
-    observer_.Add(usb_service);
+  void OnPrinterResolved(std::unique_ptr<SetUpPrinterData> data) {
+    // Copy fields that will be invalidated by std::move.
+    auto* configurer_tmp = data->configurer.get();

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

it would be nicer if this pattern could be avoided, but OK.

[stevenjb, 2017/03/17 17:34:10]

I agree this is confusing. We definitely shouldn't use auto here. Either:

a) Use a local pointer to data instead:

SetUpPrinterData* data_ptr = data.get();
data_ptr->configurer->SetupPrinter(*data_ptr->printer, ...);

b) name these configurer_ptr and printer_ptr and make both of them explicitly
typed pointers (printer_ptr can be dereferenced in the SetUpPrinter call).

[Carlson, 2017/03/17 18:47:34]

Using a local pointer to data is a much better idea, thanks.

+    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))));
+  }
+
+  // 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));
+  }
+
+  // Called 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) {

[stevenjb, 2017/03/17 17:34:10]

Remove the if () since it doesn't do anything yet, just modify the comment.

[Carlson, 2017/03/17 18:47:34]

Done.

+        // 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_;

[stevenjb, 2017/03/17 17:34:10]

having a chromeos::Printer class BTW is super confusing; a little namespacing
(e.g. chromeos::printing) would go a long way to help devs figure out where
'Printer' is defined. It doesn't help that it is in printer_configuration.h not
printer.h; the file or the class should be renamed. (Not in this CL of course,
but something to consider as a cleanup pass).

Printer is a non trivial class that we don't appear to actually use yet. Let's
not add it until we need it.

[Carlson, 2017/03/17 18:47:34]

Yeah, this sort of cleanup is probably a bit overdue at this point. filed
crbug/702710 
Disagree with this, because the next cl in this series directly depends on this
structure and the entire flow of how failed_setup_printers is managed is much
more related to this change than the followon; I think it will be easier to
understand history with this change here instead of punting it to the next cl.

If you really want to shift this to the next change, I will do that.

[stevenjb, 2017/03/17 20:49:54]

I would strongly encourage moving this to the next CL. My current thinking is
that storing |Printer| may be expensive and unnecessary, but since we don't
actually use the value there is no way for me to tell. It just involves moving a
few lines to the CL where they are actually used.

[Carlson, 2017/03/17 21:08:17]

I think what you're worried about here is a micro-optimization at best; the
structure involved is around 100-200 bytes, and we only have one of them per USB
printer plugged into the device that we haven't been able to configure.

But I'm more interested in getting this in than trying to win this argument, so
done.

[stevenjb, 2017/03/17 21:21:13]

I most likely agree, but my point is since this is unused in this CL it is
impossible for me to tell, which is why we tend to strongly discourage
committing commented out or unused code. Thanks for removing it.

 
   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