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

chrome/browser/chromeos/printing/printer_discoverer.cc

Index: chrome/browser/chromeos/printing/printer_discoverer.cc
diff --git a/chrome/browser/chromeos/printing/printer_discoverer.cc b/chrome/browser/chromeos/printing/printer_discoverer.cc
new file mode 100644
index 0000000000000000000000000000000000000000..126bf83bb0b76aa1b55708516411741ef7aab404
--- /dev/null
+++ b/chrome/browser/chromeos/printing/printer_discoverer.cc
@@ -0,0 +1,111 @@
+// 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 {
+
+// Implementation of PrinterDiscoverer interface.
+class PrinterDiscovererImpl : public PrinterDiscoverer,
+                              public PrinterDetector::Observer {
+ public:
+  explicit PrinterDiscovererImpl(Profile* profile)
+      : observer_(this),
+        printers_manager_(
+            PrintersManagerFactory::GetForBrowserContext(profile)),

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

Even though this class is associated with |profile|, it is not a keyed service
so there is no obvious guarantee that it will outlive this keyed service (and
there is no explicit dependency).
Rather than store this in |printers_manager_|, it would be better to get the
pointer as needed. That way at least to code will crash on a CHECK or a null
pointer dereference rather than accessing a potentially invalid pointer.

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

Hmm, this is a good point.  

Practically speaking, the limited lifetime of the Discoverer means I don't think
it's possible to run into this issue, but that's an ugly assumption to embed
into the class in the long term.

I would happily turn this into a keyed service itself, if I could figure out how
to limit the lifetime appropriately.  My impression is that keyed services, once
instantiated, don't go away until the associated profile does, and I don't want
an instance of this class hanging around chewing up memory all the time when we
only care about printer discovery in very limited circumstances.

Is there any reasonably elegant way to do limited lifetime *and* have this be a
keyed service?

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

There may be but I don't know off the top of my head. You would have to examine
the code. Otherwise, the overhead for calling
PrintersManagerFactory::GetForBrowserContext as needed isn't significant.

[Carlson, 2017/04/11 17:47:45]

Done.

+        weak_ptr_factory_(this) {
+    PrinterDetector* detector =
+        PrinterDetectorFactory::GetInstance()->Get(profile);

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

DCHECK(detector);

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

Done.

+    observer_.Add(detector);
+    usb_printers_ = detector->GetPrinters();
+  }
+  ~PrinterDiscovererImpl() override = default;
+
+  // PrinterDiscoverer interface function.
+  void AddObserver(PrinterDiscoverer::Observer* observer) override {
+    observer_list_.AddObserver(observer);
+    // WrappedOnPrintersFound is a simple wrapper around
+    // Observer::OnPrintersFound which lets us safely do the initial
+    // OnPrintersFound call to the registered observer.  This wrapper buys us
+    // weak_ptr semantics on 'this', and also guards against removal of the
+    // observer from the Discoverer before this callback is issued.
+    base::SequencedTaskRunnerHandle::Get()->PostTask(
+        FROM_HERE, base::Bind(&PrinterDiscovererImpl::WrappedOnPrintersFound,
+                              weak_ptr_factory_.GetWeakPtr(), observer,
+                              GetAvailablePrinters()));

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

This is awkward. I would suggest instead providing a GetAllPrinters() method
that can be called explicitly instead.

e.g.

SomeClass::Init() {
  printer_discoverer_->AddObserver(this);
  UpdatePrinters(printer_discoverer_->GetAllPrinters());
}

SomeClass::OnPrintersFound(printers) {
  UpdatePrinters(printers);
}

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

I don't think I understand what you're suggesting here.  Are you suggesting that
we shift responsibility for doing the initial query to the observer, and expose
a PrinterDiscoverer method for the observer to make that query?

If so, I'd really argue against that, because I'd rather have a simpler external
interface with an uglier implementation than complicate the interface to buy a
little bit simpler implementation.  

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

So, that is what I was suggesting, it's a pretty common pattern, but I realize
now that I was more confused than I realized.

I see now that OnDiscoveryInitialScanDone() does not return the list, it calls
OnPrintersFound() with the list and then it calls OnDiscoveryInitialScanDone()
separately.

I find personally this API confusing, especially since GetAvailablePrinters() is
synchronous, but if the other reviewers are OK with it I won't push back against
it.

[skau, 2017/04/11 17:12:49]

The behavior for detection is a little abnormal.  There are two phases of
detection.  First we actively scan for devices and emit
OnDiscoveryInitialScanDone().  After that, we continue to listen for devices as
we respond to a device being plugged in or turned on.

[stevenjb, 2017/04/11 17:19:15]

What confuses me is that "actively scan" appears to involve "call
GetAvailablePrinters()" which is synchronous and doesn't appear to ensure that
the active scan completed?

If the intention is that this will eventually wait for a scan, but that isn't
implemented yet, it might be nicer to set that up initially, i.e. use
PostTaskAndReplyWithResult(... PrinterDiscovererImpl::ScanForPrinters, ...
PrinterDiscovererImpl::WrappedOnPrintersFound).

That would certainly make the intention more clear. You can comment
ScanForPrinters with a TODO.

[Carlson, 2017/04/11 17:47:45]

I agree the API seems weird given the current implementation, but I think that's
mostly because it's also intended to handle the (as of yet unimplemented)
network discovery of printers in this class.  *That* discovery will be
asynchronous, which is why the InitialScanDone hook exists.  

Since right now all we have is synchronous printers, we can just call both API
functions at once.  When we add networked printers, the observer will typically
get

OnPrintersFound(usb printers) posted immediately when observation starts

OnPrintersFound(network printers) sometime later (possibly repeatedly, depending
on how we get data back)

OnDiscoveryInitialScanDone() - when we believe we've found all the printers

OnPrintersFound(other stuff) if printers appear/disappear while still observing.

and the implementation will change accordingly -- we'll no longer call
OnDiscoveryInitialScanDone() from WrappedOnPrintersFound, it will have to wait
until the network stuff is done as well.

The corresponding UI piece is supposed to be a live-updating list of printers
with a spinner showing we're still looking (which might take several seconds for
network discovery), the spinner goes away when we think we've discovered
everything that exists at the point the scan started, but the list still can be
updated if the state of the world changes (e.g. the user plugs in a new USB
printer, or something new shows up on the network).

Does that make more sense to you?  If there is a better API for this, I'd like
to get that right.

As for the suggestion of using PostTaskAndReplyWithResult, I think that gets the
observer-removed case wrong, doesn't it?  If someone
goes and does

discoverer->AddObserver(some_observer);
discoverer->RemoveObserver(some_observer);

and AddObserver has gone and posted a task against the observer, there is
(AFAICT) nothing that prevents the observer callback from being invoked even
though the observer is no longer observing the discoverer (also, the observer
may no longer exist!).

That's why I'm doing the WrappedOnPrintersFound two-step here.  If I can get rid
of it and still provide a surprise-free API for observers, I'd love to do that.

(We could avoid this trouble by doing the OnPrintersFound() call directly from
AddObserver(), but that doesn't seem to be the common idiom for observer-based
patterns in Chrome, so I've avoided that).

+  }
+
+  // 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:
+  // Wrapper for doing the initial OnPrintersFound call on an observer of this
+  // object.
+  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_;

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

nit: detector_observer_ would be a little more clear since this class also has
its own observer list.

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

Done.

+  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