Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2344393002:
Add a PrinterDiscoverer fake object. (Closed)
DescriptionAdd a PrinterDiscoverer fake object.
We will eventually support detecting printers using mDNS and autodetect
USB devices. This will take a little while to implement. Providing a
fake in the meantime since the system is behind a flag. The fake may be
useful for some tests in the future.
BUG=631352
Committed: https://crrev.com/28fa7dc12fc5de5c75200ce40b666c078e836087
Cr-Commit-Position: refs/heads/master@{#422641}
Patch Set 1 #
Total comments: 10
Patch Set 2 : comments addressed #
Total comments: 4
Patch Set 3 : rebase #
Total comments: 14
Patch Set 4 : rename from detector to discoverer #Patch Set 5 : lint #Patch Set 6 : more comments #
Total comments: 11
Patch Set 7 : bounds #Patch Set 8 : easier interfaces #
Messages
Total messages: 23 (8 generated)
skau@chromium.org changed reviewers: + justincarlson@chromium.org, xdai@chromium.org
Provide a FakePrinterDetector for development and testing.
Please see the comments below. https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... chromeos/printing/fake_printer_detector.cc:76: if (clipped_end < end) { should be if (clipped_end < printers_.size()) ? https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... chromeos/printing/fake_printer_detector.cc:79: weak_ptr_factory_.GetWeakPtr(), end, end + end), I guess the last two parameters should be (clipped_end, clipped_end + 2) or something like that? https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/printer_d... File chromeos/printing/printer_detector.h (right): https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/printer_d... chromeos/printing/printer_detector.h:37: // attachd observer. nit: attached https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/printer_d... chromeos/printing/printer_detector.h:46: virtual void SetObserver(PrinterDetector::Observer* observer) = 0; How about maintaining a list of observers and then have functions like AddObserver() and RemoveObserver()? It might benefit the use case that when the user opens two Add printer discovery dialog in two Chrome browser windows at the same time.
Comments addressed. PTAL. https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... chromeos/printing/fake_printer_detector.cc:76: if (clipped_end < end) { On 2016/09/19 18:56:10, xdai1 wrote: > should be if (clipped_end < printers_.size()) ? I've changed it to end < printers_.size() since that's more straightforward. Using the computed value makes it unnecessarily complicated. https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... chromeos/printing/fake_printer_detector.cc:79: weak_ptr_factory_.GetWeakPtr(), end, end + end), On 2016/09/19 18:56:10, xdai1 wrote: > I guess the last two parameters should be (clipped_end, clipped_end + 2) or > something like that? Depends if you'd rather get [0,1],[2,3],...[10,11],[12,12] (multiples of 2) or [0,1],[2,3],[4,7],[8,12] (powers of 2). The actual batch sizes could be fairly random. https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/printer_d... File chromeos/printing/printer_detector.h (right): https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/printer_d... chromeos/printing/printer_detector.h:37: // attachd observer. On 2016/09/19 18:56:10, xdai1 wrote: > nit: attached Done. https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/printer_d... chromeos/printing/printer_detector.h:46: virtual void SetObserver(PrinterDetector::Observer* observer) = 0; On 2016/09/19 18:56:10, xdai1 wrote: > How about maintaining a list of observers and then have functions like > AddObserver() and RemoveObserver()? It might benefit the use case that when the > user opens two Add printer discovery dialog in two Chrome browser windows at the > same time. Done.
Please see my comments below. https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... chromeos/printing/fake_printer_detector.cc:76: if (clipped_end < end) { On 2016/09/20 01:56:47, skau wrote: > On 2016/09/19 18:56:10, xdai1 wrote: > > should be if (clipped_end < printers_.size()) ? > > I've changed it to end < printers_.size() since that's more straightforward. > Using the computed value makes it unnecessarily complicated. I think It is incorrect to use end here since |end| can be large than printers_.size() but |clipped_end| is guaranteed no larger than printers_.size(), which means we might lost the data between [clipped_end, printers_.size()] if we use |end| here. https://codereview.chromium.org/2344393002/diff/20001/chromeos/printing/fake_... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/20001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:70: return; Return early if start >= printers_.size() https://codereview.chromium.org/2344393002/diff/20001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:72: size_t clipped_end = std::min(printers_.size(), end); should be size_t clipped_end = std::min(printers_.size()-1, end); ?
skau@, fyi: please refer to my CL https://codereview.chromium.org/2380753004/. I patched your CL and made some modification based on your CL. It worked happily. I'll wait to get my CL reviewed until your CL gets landed. BTW: You might not name the class PrinterDetector. There is already a same class in the same namespace.
Just noticed I'm a reviewer on this, sorry for the slow reply. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:72: size_t clipped_end = std::min(printers_.size(), end); If you're making end "safe", you should probably do the same for start. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:78: std::vector<Printer> subset(start_iter, end_iter); I think the above can just be simplified to std::vector<Printer> subset(printers_.begin() + start, printers_.begin() + clipped_end); ? https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:83: if (end < printers_.size()) { This deserved a comment, I think. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... File chromeos/printing/printer_detector.h (right): https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:30: virtual void OnPrintersFound(std::vector<Printer> printers) = 0; Should this passed vector be a const reference? https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:33: // Static factory. Caller is responsible for the created object. "Caller is responsible" comment is not really needed given that you're returning a unique_ptr https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:42: // returns if it returned true. What should I expect if it returns false? https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:45: // Add an observer that will be notified of discovered printers. Add ownership (not taken) note. Probably also worth noting it's an error to add the same observer multiple times.
Description was changed from ========== Add a PrinterDetector fake object. We will eventually support detecting printers using mDNS and autodetect USB devices. This will take a little while to implement. Providing a fake in the meantime since the system is behind a flag. The fake may be useful for some tests in the future. BUG=631352 ========== to ========== Add a PrinterDiscoverer fake object. We will eventually support detecting printers using mDNS and autodetect USB devices. This will take a little while to implement. Providing a fake in the meantime since the system is behind a flag. The fake may be useful for some tests in the future. BUG=631352 ==========
Thanks for the review. PTAL. https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/1/chromeos/printing/fake_prin... chromeos/printing/fake_printer_detector.cc:76: if (clipped_end < end) { On 2016/09/20 04:16:50, xdai1 wrote: > On 2016/09/20 01:56:47, skau wrote: > > On 2016/09/19 18:56:10, xdai1 wrote: > > > should be if (clipped_end < printers_.size()) ? > > > > I've changed it to end < printers_.size() since that's more straightforward. > > Using the computed value makes it unnecessarily complicated. > > I think It is incorrect to use end here since |end| can be large than > printers_.size() but |clipped_end| is guaranteed no larger than > printers_.size(), which means we might lost the data between [clipped_end, > printers_.size()] if we use |end| here. Done. https://codereview.chromium.org/2344393002/diff/20001/chromeos/printing/fake_... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/20001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:70: return; On 2016/09/20 04:16:50, xdai1 wrote: > Return early if start >= printers_.size() Done. https://codereview.chromium.org/2344393002/diff/20001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:72: size_t clipped_end = std::min(printers_.size(), end); On 2016/09/20 04:16:50, xdai1 wrote: > should be size_t clipped_end = std::min(printers_.size()-1, end); ? No. Subset is exclusive of the upper bound. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... File chromeos/printing/fake_printer_detector.cc (right): https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:72: size_t clipped_end = std::min(printers_.size(), end); On 2016/10/03 18:00:51, Carlson wrote: > If you're making end "safe", you should probably do the same for start. Done. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:78: std::vector<Printer> subset(start_iter, end_iter); On 2016/10/03 18:00:51, Carlson wrote: > I think the above can just be simplified to > > std::vector<Printer> subset(printers_.begin() + start, > printers_.begin() + clipped_end); > > ? > Done. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/fake_... chromeos/printing/fake_printer_detector.cc:83: if (end < printers_.size()) { On 2016/10/03 18:00:51, Carlson wrote: > This deserved a comment, I think. Done. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... File chromeos/printing/printer_detector.h (right): https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:30: virtual void OnPrintersFound(std::vector<Printer> printers) = 0; On 2016/10/03 18:00:51, Carlson wrote: > Should this passed vector be a const reference? yes. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:33: // Static factory. Caller is responsible for the created object. On 2016/10/03 18:00:51, Carlson wrote: > "Caller is responsible" comment is not really needed given that you're returning > a unique_ptr Done. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:42: // returns if it returned true. On 2016/10/03 18:00:51, Carlson wrote: > What should I expect if it returns false? Done. https://codereview.chromium.org/2344393002/diff/40001/chromeos/printing/print... chromeos/printing/printer_detector.h:45: // Add an observer that will be notified of discovered printers. On 2016/10/03 18:00:51, Carlson wrote: > Add ownership (not taken) note. > > Probably also worth noting it's an error to add the same observer multiple > times. Done.
lgtm https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... File chromeos/printing/fake_printer_discoverer.cc (right): https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... chromeos/printing/fake_printer_discoverer.cc:70: if (!discovery_running_ || start >= printers_.size()) For orthogonality, suggest adding || end < 0 to this.
See comments. Thanks! https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... File chromeos/printing/fake_printer_discoverer.cc (right): https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... chromeos/printing/fake_printer_discoverer.cc:22: return base::MakeUnique<FakePrinterDiscoverer>(); It might be better to make it a lazy instance so that it will only be created when user sets up a printer? Something like: static base::LazyInstance<FakePrinterDiscovery> g_printer_discovery = LAZY_INSTANCE_INITIALIZER; // static PrinterDiscovery* PrinterDiscovery::GetInstance() { return g_printer_discovery.Pointer(); } https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... chromeos/printing/fake_printer_discoverer.cc:71: return; notify when the discovery is done. https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_discoverer.h (right): https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/prin... chromeos/printing/printer_discoverer.h:23: virtual void OnDiscoveryStarted() = 0; Better to not make it pure virtual? Because in the webui, I think I only need OnPrintersFound() and OnDiscoveryDone(). https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/prin... chromeos/printing/printer_discoverer.h:32: virtual void OnPrintersFound(const std::vector<Printer>& printers) = 0; Add a "virtual void OnDiscoveryDone() {}" function to notify the observers that the discovery is done and there is no more printers nearby (so that the spinner on the UI page can stop)
https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... File chromeos/printing/fake_printer_discoverer.cc (right): https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... chromeos/printing/fake_printer_discoverer.cc:70: if (!discovery_running_ || start >= printers_.size()) On 2016/10/03 23:13:23, Carlson wrote: > For orthogonality, suggest adding > > || end < 0 > > to this. Done.
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Updated per xdai's comments. PTAL. https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... File chromeos/printing/fake_printer_discoverer.cc (right): https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... chromeos/printing/fake_printer_discoverer.cc:22: return base::MakeUnique<FakePrinterDiscoverer>(); On 2016/10/03 23:34:38, xdai1 wrote: > It might be better to make it a lazy instance so that it will only be created > when user sets up a printer? Something like: > static base::LazyInstance<FakePrinterDiscovery> g_printer_discovery = > LAZY_INSTANCE_INITIALIZER; > > // static > PrinterDiscovery* PrinterDiscovery::GetInstance() { > return g_printer_discovery.Pointer(); > } My intention is that this object is only around while the dialog is open. Won't the lazy instance imply that it sticks around when it's not being used? https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... chromeos/printing/fake_printer_discoverer.cc:71: return; On 2016/10/03 23:34:38, xdai1 wrote: > notify when the discovery is done. Whoops. Forgot about that. https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_discoverer.h (right): https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/prin... chromeos/printing/printer_discoverer.h:23: virtual void OnDiscoveryStarted() = 0; On 2016/10/03 23:34:38, xdai1 wrote: > Better to not make it pure virtual? Because in the webui, I think I only need > OnPrintersFound() and OnDiscoveryDone(). Done. https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/prin... chromeos/printing/printer_discoverer.h:32: virtual void OnPrintersFound(const std::vector<Printer>& printers) = 0; On 2016/10/03 23:34:38, xdai1 wrote: > Add a "virtual void OnDiscoveryDone() {}" function to notify the observers that > the discovery is done and there is no more printers nearby (so that the spinner > on the UI page can stop) Done.
lgtm https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... File chromeos/printing/fake_printer_discoverer.cc (right): https://codereview.chromium.org/2344393002/diff/100001/chromeos/printing/fake... chromeos/printing/fake_printer_discoverer.cc:22: return base::MakeUnique<FakePrinterDiscoverer>(); On 2016/10/03 23:52:30, skau wrote: > On 2016/10/03 23:34:38, xdai1 wrote: > > It might be better to make it a lazy instance so that it will only be created > > when user sets up a printer? Something like: > > static base::LazyInstance<FakePrinterDiscovery> g_printer_discovery = > > LAZY_INSTANCE_INITIALIZER; > > > > // static > > PrinterDiscovery* PrinterDiscovery::GetInstance() { > > return g_printer_discovery.Pointer(); > > } > > My intention is that this object is only around while the dialog is open. Won't > the lazy instance imply that it sticks around when it's not being used? Acknowledged. Didn't thought about this way.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from justincarlson@chromium.org Link to the patchset: https://codereview.chromium.org/2344393002/#ps140001 (title: "easier interfaces")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Add a PrinterDiscoverer fake object. We will eventually support detecting printers using mDNS and autodetect USB devices. This will take a little while to implement. Providing a fake in the meantime since the system is behind a flag. The fake may be useful for some tests in the future. BUG=631352 ========== to ========== Add a PrinterDiscoverer fake object. We will eventually support detecting printers using mDNS and autodetect USB devices. This will take a little while to implement. Providing a fake in the meantime since the system is behind a flag. The fake may be useful for some tests in the future. BUG=631352 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add a PrinterDiscoverer fake object. We will eventually support detecting printers using mDNS and autodetect USB devices. This will take a little while to implement. Providing a fake in the meantime since the system is behind a flag. The fake may be useful for some tests in the future. BUG=631352 ========== to ========== Add a PrinterDiscoverer fake object. We will eventually support detecting printers using mDNS and autodetect USB devices. This will take a little while to implement. Providing a fake in the meantime since the system is behind a flag. The fake may be useful for some tests in the future. BUG=631352 Committed: https://crrev.com/28fa7dc12fc5de5c75200ce40b666c078e836087 Cr-Commit-Position: refs/heads/master@{#422641} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/28fa7dc12fc5de5c75200ce40b666c078e836087 Cr-Commit-Position: refs/heads/master@{#422641} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
