Make CUPS USB printing play better with the settings page. This change does several things:

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/observer_list_threadsafe.h"
 #include "base/scoped_observer.h"
 #include "base/strings/utf_string_conversions.h"
+#include "base/synchronization/lock.h"
+#include "base/threading/sequenced_task_runner_handle.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_printer_util.h"
 #include "chrome/browser/chromeos/profiles/profile_helper.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/debug_daemon_client.h"
 #include "chromeos/printing/ppd_provider.h"
@@ skipping 38 matching lines @@
   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) {
+      : profile_(profile),
+        usb_observer_(this),
+        observer_list_(
+            new base::ObserverListThreadSafe<PrinterDetector::Observer>),
+        weak_ptr_factory_(this) {
     device::UsbService* usb_service =
         device::DeviceClient::Get()->GetUsbService();
     if (usb_service) {
-      observer_.Add(usb_service);
+      usb_observer_.Add(usb_service);
       usb_service->GetDevices(base::Bind(&CupsPrinterDetectorImpl::OnGetDevices,
                                          weak_ptr_factory_.GetWeakPtr()));
     }
   }
   ~CupsPrinterDetectorImpl() override = default;
 
+  // PrinterDetector interface function.
+  void AddObserver(PrinterDetector::Observer* observer) override {
+    observer_list_->AddObserver(observer);
+  }
+
+  // PrinterDetector interface function.
+  void RemoveObserver(PrinterDetector::Observer* observer) override {
+    observer_list_->RemoveObserver(observer);
+  }
+
+  // PrinterDetector interface function.
+  std::vector<Printer> GetPrinters() override {
+    base::AutoLock auto_lock(pp_lock_);
+    auto ret = GetPrintersLocked();
+    return ret;

[stevenjb, 2017/04/10 19:49:55]

local not needed.

Also, we could avoid an extra copy by passing a vector* to GetPrinters() instead
of returning a vector.
Alternately, return a scoped_ptr and pass ownership.

[Carlson, 2017/04/10 22:16:41]

Removed temporary.

RE: efficiency, is there a reason we expect RVO and/or rvalue reference
assignment/construction to not work here?  I would expect basically no
efficiency gains by going down the pass-a-pointer or return-a-unique-ptr routes.

+  }
+
  private:
+  std::vector<Printer> GetPrintersLocked() {
+    pp_lock_.AssertAcquired();
+    std::vector<Printer> printers;
+    printers.reserve(present_printers_.size());
+    for (const auto& entry : present_printers_) {
+      printers.push_back(*entry.second);
+    }
+    return printers;
+  }
+
   // 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);
     }
   }
 
   // UsbService::observer override.
   void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override {
     DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
     MaybeSetUpDevice(device, true);
   }
 
   // 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());
-    // TODO(justincarlson):  Update failed printers.
-  }
 
-  // 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);
-      }
+    base::AutoLock auto_lock(pp_lock_);
+    if (base::ContainsKey(present_printers_, device->guid())) {
+      present_printers_.erase(device->guid());
+      // We already have pp_lock_, so need to call the pre-locked version of
+      // GetPrinters to prevent deadlock.
+      observer_list_->Notify(
+          FROM_HERE, &PrinterDetector::Observer::OnAvailableUsbPrintersChanged,
+          GetPrintersLocked());

[stevenjb, 2017/04/10 19:49:55]

Each call to Notify is going to build a new vector. Instead call
GetPrintersLocked() once and pass the result to each observer.

[Carlson, 2017/04/10 22:16:41]

I don't understand your concern here, sorry.  There is only one call here, not a
looped construct.  What am I missing?

[stevenjb, 2017/04/11 16:27:42]

Apologies I was somehow thinking this was inside a loop.

+    } else {
+      // If the device has been removed but it's not in present_printers_, it
+      // must still be in the setup flow.
+      deferred_printer_removals_.insert(device->guid());
     }
-    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())) {
+    if (!UsbDeviceIsPrinter(*device)) {
       return;
     }
-    known_printers_.insert(device->guid());
 
+    // If we got this far, we want to try to auto-configure this printer.
     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->printer = UsbDeviceToPrinter(*device);
+    if (data->printer == nullptr) {
+      // We've failed to understand this printer device, an error will already
+      // have been logged, so just bail.
+      return;
+    }
+
+    // If the user already has a configuration for this device, substitute that
+    // one for the one we generated automatically and skip the parts where we
+    // try to automagically figure out the driver.
+    auto existing_printer_configuration =

[stevenjb, 2017/04/10 19:49:55]

Avoid using auto when the return type is not obvious to someone not super
familiar with the code (like me). auto is good for iterators and things like
MakeUnique. I assume this gets a Printer, but is it a *, scoped_pt, or &?
Better to be explicit here.

[Carlson, 2017/04/10 22:16:41]

Done.

+        PrintersManagerFactory::GetForBrowserContext(profile_)->GetPrinter(
+            data->printer->id());
+    if (existing_printer_configuration != nullptr) {
       data->is_new = false;
+      data->printer = std::move(existing_printer_configuration);
       OnPrinterResolved(std::move(data));
       return;
     }
 
     // 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;
-    }
 
     // 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(), ppd_provider,
                    base::Passed(std::move(data))));
   }
@@ skipping 35 matching lines @@
   }
 
   // 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) {
+        // We aren't done with data->printer yet, so we have to copy it instead
+        // of moving it.
+        auto printer_copy = base::MakeUnique<Printer>();
+        *printer_copy = *data->printer;

[stevenjb, 2017/04/10 19:49:55]

I think this would be more clear as:
auto printer_copy = base::MakeUnique<Printer>(new Printer(*data->printer));

[Carlson, 2017/04/10 22:16:41]

Don't need the "new Printer" part, but otherwise, done.

[stevenjb, 2017/04/11 16:27:42]

Acknowledged.

         PrintersManagerFactory::GetForBrowserContext(profile_)->RegisterPrinter(
-            std::move(data->printer));
+            std::move(printer_copy));
       }
       // TODO(justincarlson): If the device was hotplugged, pop a timed
       // notification that says the printer is now available for printing.
     }
-    // TODO(justincarlson): If this doesn't succeed, Pop a notification that
+    // TODO(justincarlson): If this doesn't succeed, pop a notification that
     // tells the user automatic setup failed and offers to open the CUPS printer
     // configuration settings.
-    //
-    // TODO(justincarlson): If this doesn't succeed, Update the list of printers
-    // that failed to set up.
+
+    if (base::ContainsKey(deferred_printer_removals_, data->device->guid())) {
+      // The device was removed before we finished the flow, so just don't add
+      // it to present_printers_;
+      deferred_printer_removals_.erase(data->device->guid());
+    } else {
+      base::AutoLock auto_lock(pp_lock_);
+      present_printers_.emplace(data->device->guid(), std::move(data->printer));
+      observer_list_->Notify(
+          FROM_HERE, &PrinterDetector::Observer::OnAvailableUsbPrintersChanged,
+          GetPrintersLocked());
+    }
   }
 
   void SetNotificationUIManagerForTesting(
       NotificationUIManager* manager) override {
     LOG(FATAL) << "Not implemented for CUPS";
   }
 
-  // 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_;
+  // Map from USB GUID to Printer that we have detected as being currently
+  // plugged in and have finished processing.  Note present_printers_ may be
+  // accessed from multiple threads, so is protected by pp_lock_.
+  std::map<std::string, std::unique_ptr<Printer>> present_printers_;
+  base::Lock pp_lock_;
+
+  // If the usb device is removed before we've finished processing it, we'll
+  // defer the cleanup until the setup flow finishes.  This is the set of
+  // guids which have been removed before the flow finished.
+  std::set<std::string> deferred_printer_removals_;
 
   Profile* profile_;
-  ScopedObserver<device::UsbService, device::UsbService::Observer> observer_;
+  ScopedObserver<device::UsbService, device::UsbService::Observer>
+      usb_observer_;
+  scoped_refptr<base::ObserverListThreadSafe<PrinterDetector::Observer>>
+      observer_list_;
   base::WeakPtrFactory<CupsPrinterDetectorImpl> weak_ptr_factory_;
 };
 
 }  // namespace
 
 // static
 std::unique_ptr<PrinterDetector> PrinterDetector::CreateCups(Profile* profile) {
   return base::MakeUnique<CupsPrinterDetectorImpl>(profile);
 }
 
 }  // namespace chromeos