New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 754834 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jan 2018
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Make USBPrinterDetector observer callbacks simpler

Project Member Reported by justincarlson@chromium.org, Aug 11 2017

Issue description

skau points out that the current behavior of the UsbPrinterDetector observer callbacks is unnecessarily arcane; this should be cleaned up.

See comment here:

https://chromium-review.googlesource.com/c/611103/3/chrome/browser/chromeos/printing/usb_printer_detector.cc#84
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jan 24 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e

commit d71c3f260508b4b52d2e1bef0fc09b4c444ce52e
Author: Justin Carlson <justincarlson@chromium.org>
Date: Wed Jan 24 20:41:46 2018

Simplify PrinterDetector Observer APIs.

This cl does several things.

First, It removes the PrinterDetector::Observer::OnPrinterScanComplete
API hook.  That hook exists because we expected zeroconf printer
detection to take a non-trivial amount of time and we wanted to be
able to enable some UI indication that we were still looking for
printers until the scan was complete.  However, it turns out that
mdns/dns-sd traffic is all cached anyways by the ServiceDiscovery,
so we get initial printers back for zeroconf almost instantly, making
this hook pointless.

Second, the Observer API is changed so that it no longer guarantees an
OnPrintersFound callback to the caller after registration; instead the
client observing printer change events is responsible for calling
PrinterDetector::GetPrinters() to do the initial population of the
printer list.  This removes a lot of weird hacks to get thread safety
right.

This change also removes CupsPrintersManager::Start() and
PrinterDetector::StartObservers(), as these are no longer needed to
make startup safe.  These calls were added out of concerns that we
would encounter something like:

(in class Foo's constructor):
my_detector_->AddObserver(this);
   ->OBSERVER CALLBACK HAPPENS BEFORE Foo::Foo() IS FINISHED<-
	 ...rest of Foo's constructor...

However, this isn't a real problem for the API AFAICT.  Since the
observer callbacks are on the same sequence as the constructor, they
can't start until after the constructor finishes, meaning this is a
non-issue.

I think.  I would appreciate reviewers taking a bit of time to think
this through and see if they also believe this is now safe without the
Start() calls, as concurrency is sometimes hard to reason about.

This cl also removes some tests around StartObservers() that are no
longer relevant with the API changes.

Bug:  754834 
Change-Id: Ia2e4c34b82ddf99a7551756c2bc037889f52df5b
Reviewed-on: https://chromium-review.googlesource.com/871660
Commit-Queue: Justin Carlson <justincarlson@chromium.org>
Reviewed-by: Sean Kau <skau@chromium.org>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Cr-Commit-Position: refs/heads/master@{#531673}
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/chromeos/BUILD.gn
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/chromeos/printing/cups_printers_manager.cc
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/chromeos/printing/cups_printers_manager.h
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/chromeos/printing/cups_printers_manager_unittest.cc
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/chromeos/printing/printer_detector.h
[delete] https://crrev.com/e81f86ad87908306eb054a8cbab479a61f86f62d/chrome/browser/chromeos/printing/printer_detector_test_util.h
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/chromeos/printing/usb_printer_detector.cc
[delete] https://crrev.com/e81f86ad87908306eb054a8cbab479a61f86f62d/chrome/browser/chromeos/printing/usb_printer_detector_unittest.cc
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/chromeos/printing/zeroconf_printer_detector.cc
[delete] https://crrev.com/e81f86ad87908306eb054a8cbab479a61f86f62d/chrome/browser/chromeos/printing/zeroconf_printer_detector_unittest.cc
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/ui/webui/print_preview/local_printer_handler_chromeos.cc
[modify] https://crrev.com/d71c3f260508b4b52d2e1bef0fc09b4c444ce52e/chrome/browser/ui/webui/settings/chromeos/cups_printers_handler.cc

Status: Fixed (was: Assigned)

Sign in to add a comment