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

chrome/browser/chromeos/printing/printer_discoverer.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 "chrome/browser/chromeos/printing/printer_discoverer.h"
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/memory/weak_ptr.h"
+#include "base/observer_list.h"
+#include "base/scoped_observer.h"
+#include "base/threading/sequenced_task_runner_handle.h"
+#include "chrome/browser/chromeos/printer_detector/printer_detector.h"
+#include "chrome/browser/chromeos/printer_detector/printer_detector_factory.h"
+#include "chrome/browser/chromeos/printing/printers_manager.h"
+#include "chrome/browser/chromeos/printing/printers_manager_factory.h"
+#include "chrome/browser/profiles/profile.h"
+#include "chromeos/printing/printer_configuration.h"
+
+namespace chromeos {
+namespace {
+
+class PrinterDiscovererImpl : public PrinterDiscoverer,
+                              public PrinterDetector::Observer {
+ public:
+  explicit PrinterDiscovererImpl(Profile* profile)
+      : observer_(this),
+        printers_manager_(
+            PrintersManagerFactory::GetForBrowserContext(profile)),
+        weak_ptr_factory_(this) {
+    PrinterDetector* detector =
+        PrinterDetectorFactory::GetInstance()->Get(profile);
+    observer_.Add(detector);
+    usb_printers_ = detector->GetPrinters();
+  }
+  ~PrinterDiscovererImpl() override = default;
+
+  // PrinterDiscoverer interface function.
+  void AddObserver(PrinterDiscoverer::Observer* observer) override {
+    observer_list_.AddObserver(observer);
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&PrinterDiscovererImpl::WrappedOnPrintersFound,
+                              weak_ptr_factory_.GetWeakPtr(), observer,
+                              GetAvailablePrinters()));
+  }
+
+  // PrinterDiscoverer interface function.
+  void RemoveObserver(PrinterDiscoverer::Observer* observer) override {
+    observer_list_.RemoveObserver(observer);
+  }
+
+  // PrinterDetector::Observer interface function.
+  void OnAvailableUsbPrintersChanged(
+      const std::vector<Printer>& printers) override {
+    usb_printers_ = printers;
+    std::vector<Printer> all_printers = GetAvailablePrinters();
+    for (PrinterDiscoverer::Observer& observer : observer_list_) {
+      observer.OnPrintersFound(all_printers);
+    }
+  }
+
+ private:
+  // This is a simple wrapper around Observer::OnPrintersFound which lets us
+  // safely do the initial OnPrintersFound call after an observer is registered.
+  // This wrapper buys us weak_ptr semantics on the Discoverer object, and also

[skau, 2017/03/31 17:50:11]

Do you mean you get weak_ptr semantics on the Observer?  I'm unclear what object
you're protecting against here.

[Carlson, 2017/03/31 18:20:07]

Long comment is insufficiently long.  :)

This is ultimately motivated by the fact that we want to ensure that a new
observer of this class gets an OnPrintersFound callback with the current state,
not just when the state changes.  So when the observer is added, I want to
schedule an OnPrintersFound callback.

But in between the time that I post that task and the time it executes, a couple
things could happen.

1)  The PrinterDiscoverer instance could be destroyed.  In which case we don't
want to execute the callback 
2)  The observer that was added could be destroyed (or, equivalently for our
purposes, removed as an observer of this class).  In that case we also don't
want to do the callback.

The PrinterDiscoverer weak_ptr stuff protects us from the first scenario.  The
HasObserver call protects us from the second.  Doing these things requires the
callback that we schedule at line 41 in AddObserver calls back to this wrapper
instead of directly to the observer itself.

Does that make sense?  If so, any suggestions on how to make the comment
clearer?

[skau, 2017/03/31 21:03:19]

That makes more sense.  You should probably mention where the method is called
since that informs the requirements.  I'm assuming this method does't need to be
thread safe because it's sequenced?

[Carlson, 2017/03/31 22:58:41]

Moved the bulk of the comment to the callsite and reworded a bit.

Yes to the question (the sequencing requirement is in a comment at the interface
level)

+  // guards against removal of the observer from the Discoverer before this
+  // callback is issued.
+  void WrappedOnPrintersFound(PrinterDiscoverer::Observer* observer,
+                              const std::vector<Printer>& printers) {
+    if (observer_list_.HasObserver(observer)) {
+      observer->OnPrintersFound(printers);
+      // Since USB is the only thing we're worried about at the moment,
+      // and we don't have to wait for those printers to be scanned, we
+      // can just tell the observer the initial scan is done now.
+      observer->OnDiscoveryInitialScanDone();
+    }
+  }
+
+  // Get the current set of discovered printers that are not already known
+  // to the user's PrintersManager.
+  std::vector<Printer> GetAvailablePrinters() {
+    // Only know about usb printers for now.  Eventually we'll add discovered
+    // network printers as well.
+    std::vector<Printer> ret;
+
+    for (const Printer& printer : usb_printers_) {
+      if (printers_manager_->GetPrinter(printer.id()).get() == nullptr) {
+        ret.push_back(printer);
+      }
+    }
+    return ret;
+  }
+
+  std::vector<Printer> usb_printers_;
+  base::ObserverList<PrinterDiscoverer::Observer> observer_list_;
+  ScopedObserver<PrinterDetector, PrinterDetector::Observer> observer_;
+  PrintersManager* printers_manager_;
+  base::WeakPtrFactory<PrinterDiscovererImpl> weak_ptr_factory_;
+};
+
+}  // namespace
+
+// static
+std::unique_ptr<PrinterDiscoverer> PrinterDiscoverer::Create(Profile* profile) {
+  return base::MakeUnique<PrinterDiscovererImpl>(profile);
+}
+
+}  // namespace chromeos