Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2738323003:
Implement basic USB setup in the cups printer detector. Factor out (Closed)
|
Created:
3 years, 9 months ago by Carlson Modified:
3 years, 9 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionImplement basic USB setup in the cups printer detector.
Factor out some utility usb functions into another library (these will be reused in discovery).
With this change, if you plug in a USB printer that we handle while
connected to the network, it should automatically get set up.
While I'm in there, rename chromeos::SetupResult to
chromeos::PrinterSetupResult to prevent polluting the chromeos
namespace with a relatively generic name.
BUG=616866
Review-Url: https://codereview.chromium.org/2738323003
Cr-Commit-Position: refs/heads/master@{#458536}
Committed: https://chromium.googlesource.com/chromium/src/+/506ac3f696915a9cf1e8da551e9f68b8a6408b45
Patch Set 1 #Patch Set 2 : Fix lint and other minor things #
Total comments: 19
Patch Set 3 : Implement basic USB setup in the cups printer detector. Factor out some utility usb functions into… #
Total comments: 30
Patch Set 4 : Address tbarzic comments #Patch Set 5 : Address a couple tbarzic comments I missed. #Patch Set 6 : Remove dep on extensions #
Total comments: 16
Patch Set 7 : Address more tbarzic comments #Patch Set 8 : Couple more tweaks. #Patch Set 9 : More minor tweaks. #
Total comments: 18
Patch Set 10 : Implement basic USB setup in the cups printer detector. Factor out some utility usb functions into… #Patch Set 11 : Remove failed printer tracking for now. #Patch Set 12 : Fix ppd provider lifetime. It was being inappropriately deleted which meant it caused USB resoluti… #Messages
Total messages: 32 (8 generated)
Description was changed from ========== Implement basic USB setup in the cups printer detector. Factor out some utility usb functions into another library (these will be reused in discovery). With this change, if you plug in a USB printer that we handle while connected to the network, it should automatically get set up. While I'm in there, rename chromeos::SetupResult to chromeos::PrinterSetupResult to prevent polluting the chromeos namespace with a relatively generic name. BUG=616866 ========== to ========== Implement basic USB setup in the cups printer detector. Factor out some utility usb functions into another library (these will be reused in discovery). With this change, if you plug in a USB printer that we handle while connected to the network, it should automatically get set up. While I'm in there, rename chromeos::SetupResult to chromeos::PrinterSetupResult to prevent polluting the chromeos namespace with a relatively generic name. BUG=616866 ==========
justincarlson@chromium.org changed reviewers: + skau@chromium.org
Sean, can you take an initial pass at this?
generally looks good https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:111: if (!known_printers_.insert(device->guid()).second) { Is this a common idiom? I didn't know that map::insert returned pair<iterator, bool> until I just looked it up. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:120: void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override { nit: DRY https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:163: PrintersManagerFactory::GetForBrowserContext(profile_)->GetPrinters(); TODO: add a GetPrinterByUri to PrintersManagerFactory https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:187: auto ppd_provider = printing::CreateProvider(profile_); scoped_refptf<PpdProvider>. In this case, it's important we know the exact type. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:217: base::UTF16ToUTF8(data->device->product_string()); This function should have a name. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:232: // configured for cups. If |printer_to_register| is non-null and we nit: s/cups/CUPS/ https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:275: scoped_refptr<printing::PpdProvider> ppd_provider_; This member appears unused. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:22: // recognizes. Cups hex-encodes '%', ' ', and anything with the MSB set, and Out of curiosity, how'd you find out it encodes characters with the MSB set? What encoding does this assume?
justincarlson@chromium.org changed reviewers: + tbarzic@chromium.org
PTAL? Adding tbarzic who may have opinions about what I'm doing to stuff I think he originated. :) https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:111: if (!known_printers_.insert(device->guid()).second) { On 2017/03/11 00:00:34, skau wrote: > Is this a common idiom? I didn't know that map::insert returned pair<iterator, > bool> until I just looked it up. I would have guessed yes, but apparently not: https://cs.chromium.org/search/?q=%5C.insert%5C(.*%5C)%5C.second+package:%5Ec... Broke it up into multiple lines. I generally default to *not* breaking up stuff like this because doing so hits the map and hash function multiple times without need, but OTOH we don't seem to have the sort of "bool InsertIfNotPresent(...)" function around that makes that easier to read... (Note this moved into [Maybe]SetUpDevice()) https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:120: void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override { On 2017/03/11 00:00:34, skau wrote: > nit: DRY Not sure which part you're referring to. You mean the stuff before SetUpDevice that's common between here and above? The number of times "device" appears in this line? Assuming the former, moved common stuff into SetUpDevice. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:163: PrintersManagerFactory::GetForBrowserContext(profile_)->GetPrinters(); On 2017/03/11 00:00:34, skau wrote: > TODO: add a GetPrinterByUri to PrintersManagerFactory Done. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:187: auto ppd_provider = printing::CreateProvider(profile_); On 2017/03/11 00:00:34, skau wrote: > scoped_refptf<PpdProvider>. In this case, it's important we know the exact > type. Done. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:217: base::UTF16ToUTF8(data->device->product_string()); On 2017/03/11 00:00:34, skau wrote: > This function should have a name. A function had no name. (Done.) https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:275: scoped_refptr<printing::PpdProvider> ppd_provider_; On 2017/03/11 00:00:34, skau wrote: > This member appears unused. Thanks, was doing this differently before. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:22: // recognizes. Cups hex-encodes '%', ' ', and anything with the MSB set, and On 2017/03/11 00:00:34, skau wrote: > Out of curiosity, how'd you find out it encodes characters with the MSB set? > What encoding does this assume? By inspecting the cups source code. Ultimately it's http_copy_encode() in cups/http-support.c that performs this function. As for encoding, you mean, like unicode? It's ASCII all the way down in CUPS, I think.
lgtm Thanks for the changes. I figured out why the encoding looks strange to me and left a note. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:120: void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override { On 2017/03/11 01:19:16, Carlson wrote: > On 2017/03/11 00:00:34, skau wrote: > > nit: DRY > > Not sure which part you're referring to. You mean the stuff before SetUpDevice > that's common between here and above? The number of times "device" appears in > this line? > > Assuming the former, moved common stuff into SetUpDevice. Yes. I was referring to the code above. Sorry for being unclear. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:22: // recognizes. Cups hex-encodes '%', ' ', and anything with the MSB set, and On 2017/03/11 01:19:17, Carlson wrote: > On 2017/03/11 00:00:34, skau wrote: > > Out of curiosity, how'd you find out it encodes characters with the MSB set? > > What encoding does this assume? > > By inspecting the cups source code. Ultimately it's http_copy_encode() in > cups/http-support.c that performs this function. > > As for encoding, you mean, like unicode? It's ASCII all the way down in CUPS, I > think. Thanks. It looks like they allow any ASCII character except for spaces. Can you say that we encode values outside the ASCII range (i.e. >127) rather than where the MSB is set? I do remember that ASCII is 128 characters. I don't remember what the decimal representation of 0x80 is (even if I should). On an unrelated note, I wouldn't expect /BEL to be a valid character in a printer name.
lgtm Thanks for the changes. I figured out why the encoding looks strange to me and left a note. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:120: void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override { On 2017/03/11 01:19:16, Carlson wrote: > On 2017/03/11 00:00:34, skau wrote: > > nit: DRY > > Not sure which part you're referring to. You mean the stuff before SetUpDevice > that's common between here and above? The number of times "device" appears in > this line? > > Assuming the former, moved common stuff into SetUpDevice. Yes. I was referring to the code above. Sorry for being unclear. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:22: // recognizes. Cups hex-encodes '%', ' ', and anything with the MSB set, and On 2017/03/11 01:19:17, Carlson wrote: > On 2017/03/11 00:00:34, skau wrote: > > Out of curiosity, how'd you find out it encodes characters with the MSB set? > > What encoding does this assume? > > By inspecting the cups source code. Ultimately it's http_copy_encode() in > cups/http-support.c that performs this function. > > As for encoding, you mean, like unicode? It's ASCII all the way down in CUPS, I > think. Thanks. It looks like they allow any ASCII character except for spaces. Can you say that we encode values outside the ASCII range (i.e. >127) rather than where the MSB is set? I do remember that ASCII is 128 characters. I don't remember what the decimal representation of 0x80 is (even if I should). On an unrelated note, I wouldn't expect /BEL to be a valid character in a printer name.
lgtm Thanks for the changes. I figured out why the encoding looks strange to me and left a note.
https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:82: extensions::ExtensionSystem::Get(profile)->ready().Post( Is dependency on extensions system needed? https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:135: What if the removed printer is being set up? Should the setup be cancelled in that case? https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:152: auto data = base::MakeUnique<SetUpPrinterData>(); I wonder if you could introduce a helper class that does setup here, and have known_printers map device GUID to the setup helper instead of passing this struct around? I think that would look a bit more streamlined. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:166: // (bug 700602) nit: https://crbug.com/700602 (instead of bug 700602) https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:176: auto* configurer_tmp = data->configurer.get(); This is basically the same as what's done in ResolveUsbIdDone Would it make sense to extract this into a separate method: OnPrinterResolved(); https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:202: void ResolveUsbIdsDone(scoped_refptr<PpdProvider> provider, is it necessary to pass provider here? https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:25: static const char kHexDigits[] = "0123456789ABCDEF"; I wonder if this should be done before sending request to CUPS rather than when creating a printer? (assuming CUPS formatted URI feels like breaking chromeos::Printer abstraction, though, you probably know this code better than I do). https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:73: // nit: rm this line https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:78: device.manufacturer_string().empty() || device.product_string().empty()) { Are these so important that we want to reject the device id they are not set? https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.h (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.h:12: #include "base/logging.h" can you remove includes you don't need here? https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.h:18: forward declare Printer and UsbDevice?
Appreciate the ideas and review. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:120: void OnDeviceAdded(scoped_refptr<device::UsbDevice> device) override { On 2017/03/13 21:56:25, skau wrote: > On 2017/03/11 01:19:16, Carlson wrote: > > On 2017/03/11 00:00:34, skau wrote: > > > nit: DRY > > > > Not sure which part you're referring to. You mean the stuff before > SetUpDevice > > that's common between here and above? The number of times "device" appears in > > this line? > > > > Assuming the former, moved common stuff into SetUpDevice. > > Yes. I was referring to the code above. Sorry for being unclear. Done. https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:22: // recognizes. Cups hex-encodes '%', ' ', and anything with the MSB set, and On 2017/03/13 21:56:25, skau wrote: > On 2017/03/11 01:19:17, Carlson wrote: > > On 2017/03/11 00:00:34, skau wrote: > > > Out of curiosity, how'd you find out it encodes characters with the MSB set? > > > > What encoding does this assume? > > > > By inspecting the cups source code. Ultimately it's http_copy_encode() in > > cups/http-support.c that performs this function. > > > > As for encoding, you mean, like unicode? It's ASCII all the way down in CUPS, > I > > think. > > Thanks. It looks like they allow any ASCII character except for spaces. Can > you say that we encode values outside the ASCII range (i.e. >127) rather than > where the MSB is set? I do remember that ASCII is 128 characters. I don't > remember what the decimal representation of 0x80 is (even if I should). > > On an unrelated note, I wouldn't expect /BEL to be a valid character in a > printer name. I agree that the escaping choice is...weird. Updated comment. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:82: extensions::ExtensionSystem::Get(profile)->ready().Post( On 2017/03/14 01:43:32, tbarzic wrote: > Is dependency on extensions system needed? I believe so, though my understanding of this aspect of the system is rather tenuous, so I could be wrong. We rely on the PrintersManager code to be instantiated when usb device processing starts. This means that we don't want to actually start listening for and processing events until the extension system has fully instantiated everything. And I think this is the right way to do that? Let me know if I'm misunderstanding this. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:135: On 2017/03/14 01:43:32, tbarzic wrote: > What if the removed printer is being set up? Should the setup be cancelled in > that case? It should be harmless. We'll end up setting up the printer for use with CUPS, but doesn't require actually hitting the USB connection to complete. There is an open question of "how do we determine which printers are available at print time so we don't display options that the user can't actually print to", and one idea that has been tossed around is de-configuring devices on USB removal, in which case yes, we'll have to handle that race elegantly. If/when we do want to handle it, I suspect the right answer will be "defer the removal processing until setup completes". https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:152: auto data = base::MakeUnique<SetUpPrinterData>(); On 2017/03/14 01:43:32, tbarzic wrote: > I wonder if you could introduce a helper class that does setup here, and have > known_printers map device GUID to the setup helper instead of passing this > struct around? > I think that would look a bit more streamlined. > I really liked this idea and played with implementing it. However, the object lifetime issues get sort of complicated quickly, particularly around the transfer of control from the helper class back to this class, and I think that, as a result, it ends up being a net loss. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:166: // (bug 700602) On 2017/03/14 01:43:32, tbarzic wrote: > nit: https://crbug.com/700602 > (instead of bug 700602) Done. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:176: auto* configurer_tmp = data->configurer.get(); On 2017/03/14 01:43:32, tbarzic wrote: > This is basically the same as what's done in ResolveUsbIdDone > Would it make sense to extract this into a separate method: > OnPrinterResolved(); Done. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:202: void ResolveUsbIdsDone(scoped_refptr<PpdProvider> provider, On 2017/03/14 01:43:32, tbarzic wrote: > is it necessary to pass provider here? Upon a lot of reflection, no. I thought I needed it to keep the provider from being deleted at the end of this function, but as I understand it, it should stay alive until its last callback is done anyways, The sort of deep black magic of how lifetime extension effectively works through the bind makes me pretty deeply uncomfortable, though. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:25: static const char kHexDigits[] = "0123456789ABCDEF"; On 2017/03/14 01:43:32, tbarzic wrote: > I wonder if this should be done before sending request to CUPS rather than when > creating a printer? > (assuming CUPS formatted URI feels like breaking chromeos::Printer abstraction, > though, you probably know this code better than I do). Hmm, that's a good point. I think we don't actually apply CUPS uri escaping to other printers at this point (though we probably should). Filing a bug for now, as I don't want to wait for that to be resolved. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:73: // On 2017/03/14 01:43:32, tbarzic wrote: > nit: rm this line Done. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.cc:78: device.manufacturer_string().empty() || device.product_string().empty()) { On 2017/03/14 01:43:32, tbarzic wrote: > Are these so important that we want to reject the device id they are not set? I think so; they are really the basic fields we need to be able to figure anything out about the printer. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/usb_util.h (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.h:12: #include "base/logging.h" On 2017/03/14 01:43:33, tbarzic wrote: > can you remove includes you don't need here? Done. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/usb_util.h:18: On 2017/03/14 01:43:32, tbarzic wrote: > forward declare Printer and UsbDevice? I didn't realize until today that the rules about this are different for Google Style Guide, and Chromium. Thanks.
https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:82: extensions::ExtensionSystem::Get(profile)->ready().Post( On 2017/03/15 00:31:15, Carlson wrote: > On 2017/03/14 01:43:32, tbarzic wrote: > > Is dependency on extensions system needed? > > I believe so, though my understanding of this aspect of the system is rather > tenuous, so I could be wrong. > > We rely on the PrintersManager code to be instantiated when usb device > processing starts. This means that we don't want to actually start listening > for and processing events until the extension system has fully instantiated > everything. And I think this is the right way to do that? > > Let me know if I'm misunderstanding this. > In order for services to be cleaned up in the correct order, they declare a dependency hierarchy. https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv...
https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:82: extensions::ExtensionSystem::Get(profile)->ready().Post( On 2017/03/15 04:05:53, skau wrote: > On 2017/03/15 00:31:15, Carlson wrote: > > On 2017/03/14 01:43:32, tbarzic wrote: > > > Is dependency on extensions system needed? > > > > I believe so, though my understanding of this aspect of the system is rather > > tenuous, so I could be wrong. > > > > We rely on the PrintersManager code to be instantiated when usb device > > processing starts. This means that we don't want to actually start listening > > for and processing events until the extension system has fully instantiated > > everything. And I think this is the right way to do that? > > > > Let me know if I'm misunderstanding this. > > > > In order for services to be cleaned up in the correct order, they declare a > dependency hierarchy. > https://cs.chromium.org/chromium/src/components/keyed_service/core/keyed_serv... I hadn't seen that, thanks. Added the PrintersManagerFactor dependency, removed the ExtensionSystem one.
https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:99: user != user_manager::UserManager::Get()->GetActiveUser()) { I don't think this is right. Active user could change, so in multiuser environment, the number of running PrinterDetectors might end up being inconsistent (but I think generally, this will be created when the profile is associated with the active user). The main purpose of this check was not to show same notifications for all logged in users when a printer is plugged in. Given that this does some background processing too, it might be better to restrict this to primary user only, or have it run for all logged in users. Though, it mainly depends on how we want the feature to work in multi user environment. I assume printers are set up per user/profile? https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:113: // this on the UI thread. nit: I'd remove the second sentence - it's clear from the code. (same goes for other instances where you have Thread DCHECK) https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:137: // remove it. nit: I'd remove the comment - the code is clear enough. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:33: #include "extensions/browser/extension_registry.h" this is not needed anymore, right? https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:44: // in a bunch of functions below, this struct aggregates everything This comment could be a lot shorter :) E.g. Aggregates the information needed for printer setup so it's easier to pass it around. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:65: // Given a usb device, guess the make and model for a driver lookup. nit: s/guess/guesses/ https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:145: data->printer = base::MakeUnique<Printer>(); This still seems a little convoluted :/ Could we introduce a method GetUsbPrinterUri(const UsbDevice& device), and change the flow to something like: bool is_new = false; std::unique_ptr<Printer>printer = GetPrinterForDevice(*device, &is_new); if (!data->printer) return; // Do stuff with printer. GetPrinterForDevice(const UsbDevice& device, bool* is_new) { std::string uri = GetUsbPrinterUri(device); if (uri.empty()) return nullptr; std::unique_ptr<Printer> = FindPrinterByUri(uri); if (printer) { *is_new = false; return printer; } *is_new = true; return UsbDeviceToPrinter(device); } std::unique_ptr<Printer> FindPrinterByUri(const std::string& uri) { for (printer : GetPrinters()) if (printer->uri() == uri) return std::move(printer); } https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:150: LOG(ERROR) UsbDeviceToPrinter already logs an error https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:213: OnPrinterResolved(std::move(data)); I wonder if it would make sense to register new printers with PrintersManager at this point so the resolved data is not lost in case of a failure (given that it seems that printers in PrintersManager are not necessarily set up); though I'm not familiar with PrintersManager semantics enough to know whether that would be the right thing to do. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/printer_detector_factory.cc (left): https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/printer_detector_factory.cc:38: extensions::ExtensionsBrowserClient::Get()->GetExtensionSystemFactory()); doesn't legacy printer detector still depend on extensions system?
https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:99: user != user_manager::UserManager::Get()->GetActiveUser()) { On 2017/03/15 18:43:42, tbarzic wrote: > I don't think this is right. Active user could change, so in multiuser > environment, the number of running PrinterDetectors might end up being > inconsistent (but I think generally, this will be created when the profile is > associated with the active user). > > The main purpose of this check was not to show same notifications for all logged > in users when a printer is plugged in. > > Given that this does some background processing too, it might be better to > restrict this to primary user only, or have it run for all logged in users. > Though, it mainly depends on how we want the feature to work in multi user > environment. > > I assume printers are set up per user/profile? Ha, I'd meant to figure this out later, but sure, we can work it out now. I just dug through a lot of layers and I believe it should be fine to do this for *any* profile at any time, including multiple profiles concurrently. Additionally, we *want* to run this flow for any user who might want to use the printer, so I've removed this check entirely. I think when we re-add some notifications about what's going on, it may be that we revisit this in some way, but this is good for now, I think. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:113: // this on the UI thread. On 2017/03/15 18:43:42, tbarzic wrote: > nit: I'd remove the second sentence - it's clear from the code. > (same goes for other instances where you have Thread DCHECK) Not sure I agree completely with this, I think there's value in having the calling requirements documented at the comment level, but ok, done. https://codereview.chromium.org/2738323003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:137: // remove it. On 2017/03/15 18:43:42, tbarzic wrote: > nit: I'd remove the comment - the code is clear enough. Done. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:33: #include "extensions/browser/extension_registry.h" On 2017/03/15 18:43:42, tbarzic wrote: > this is not needed anymore, right? Done. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:44: // in a bunch of functions below, this struct aggregates everything On 2017/03/15 18:43:42, tbarzic wrote: > This comment could be a lot shorter :) > E.g. > Aggregates the information needed for printer setup so it's easier to pass it > around. Done. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:65: // Given a usb device, guess the make and model for a driver lookup. On 2017/03/15 18:43:42, tbarzic wrote: > nit: s/guess/guesses/ Done. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:145: data->printer = base::MakeUnique<Printer>(); On 2017/03/15 18:43:42, tbarzic wrote: > This still seems a little convoluted :/ > Could we introduce a method GetUsbPrinterUri(const UsbDevice& device), and > change the flow to something like: > > bool is_new = false; > std::unique_ptr<Printer>printer = GetPrinterForDevice(*device, &is_new); > if (!data->printer) > return; > // Do stuff with printer. > > GetPrinterForDevice(const UsbDevice& device, bool* is_new) { > std::string uri = GetUsbPrinterUri(device); > if (uri.empty()) > return nullptr; > > std::unique_ptr<Printer> = FindPrinterByUri(uri); > if (printer) { > *is_new = false; > return printer; > } > > *is_new = true; > return UsbDeviceToPrinter(device); > } > > std::unique_ptr<Printer> FindPrinterByUri(const std::string& uri) { > for (printer : GetPrinters()) > if (printer->uri() == uri) > return std::move(printer); > } Took some of these ideas to try to clarify, but did it a little differently. Let me know what you think. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:150: LOG(ERROR) On 2017/03/15 18:43:42, tbarzic wrote: > UsbDeviceToPrinter already logs an error Done. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:213: OnPrinterResolved(std::move(data)); On 2017/03/15 18:43:42, tbarzic wrote: > I wonder if it would make sense to register new printers with PrintersManager at > this point so the resolved data is not lost in case of a failure (given that it > seems that printers in PrintersManager are not necessarily set up); though I'm > not familiar with PrintersManager semantics enough to know whether that would be > the right thing to do. There's a reasonable argument to be made for that, but I think that's not the right thing here. PrintersManager has to cope with bad printers, as printers may break or disappear, but I don't think there's value in having a USB printer enter the PrintersManager list until we can print to it. Note that there's a separate mechanism I'm laying foundation for here to be able to retrieve the list of printers we *failed* to set up, so we can show those to the user in the settings dialog as printers that we can try to manually set up. https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/printer_detector_factory.cc (left): https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/printer_detector_factory.cc:38: extensions::ExtensionsBrowserClient::Get()->GetExtensionSystemFactory()); On 2017/03/15 18:43:43, tbarzic wrote: > doesn't legacy printer detector still depend on extensions system? Ugh, got tunnel vision there for a minute, thanks for catching that.
LGTM https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:145: data->printer = base::MakeUnique<Printer>(); On 2017/03/15 23:06:16, Carlson wrote: > On 2017/03/15 18:43:42, tbarzic wrote: > > This still seems a little convoluted :/ > > Could we introduce a method GetUsbPrinterUri(const UsbDevice& device), and > > change the flow to something like: > > > > bool is_new = false; > > std::unique_ptr<Printer>printer = GetPrinterForDevice(*device, &is_new); > > if (!data->printer) > > return; > > // Do stuff with printer. > > > > GetPrinterForDevice(const UsbDevice& device, bool* is_new) { > > std::string uri = GetUsbPrinterUri(device); > > if (uri.empty()) > > return nullptr; > > > > std::unique_ptr<Printer> = FindPrinterByUri(uri); > > if (printer) { > > *is_new = false; > > return printer; > > } > > > > *is_new = true; > > return UsbDeviceToPrinter(device); > > } > > > > std::unique_ptr<Printer> FindPrinterByUri(const std::string& uri) { > > for (printer : GetPrinters()) > > if (printer->uri() == uri) > > return std::move(printer); > > } > > Took some of these ideas to try to clarify, but did it a little differently. > Let me know what you think. looks good https://codereview.chromium.org/2738323003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:213: OnPrinterResolved(std::move(data)); On 2017/03/15 23:06:16, Carlson wrote: > On 2017/03/15 18:43:42, tbarzic wrote: > > I wonder if it would make sense to register new printers with PrintersManager > at > > this point so the resolved data is not lost in case of a failure (given that > it > > seems that printers in PrintersManager are not necessarily set up); though I'm > > not familiar with PrintersManager semantics enough to know whether that would > be > > the right thing to do. > > There's a reasonable argument to be made for that, but I think that's not the > right thing here. PrintersManager has to cope with bad printers, as printers > may break or disappear, but I don't think there's value in having a USB printer > enter the PrintersManager list until we can print to it. > > Note that there's a separate mechanism I'm laying foundation for here to be able > to retrieve the list of printers we *failed* to set up, so we can show those to > the user in the settings dialog as printers that we can try to manually set up. sounds good https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:175: auto* configurer_tmp = data->configurer.get(); it would be nicer if this pattern could be avoided, but OK.
justincarlson@chromium.org changed reviewers: + stevenjb@chromium.org
Steven, can you look at this for OWNERS? You seem to be the lucky person who is in OWNERS in both areas this touches.
https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:18: #include "chrome/browser/chromeos/printer_detector/printer_detector.h" nit: I don't know if our precommit allows it, but it would be nice if we can put this include first since it is the associated header for this class. Alternatively put the class definition in a header for improved discoverability. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:175: auto* configurer_tmp = data->configurer.get(); On 2017/03/16 04:11:29, tbarzic wrote: > it would be nicer if this pattern could be avoided, but OK. I agree this is confusing. We definitely shouldn't use auto here. Either: a) Use a local pointer to data instead: SetUpPrinterData* data_ptr = data.get(); data_ptr->configurer->SetupPrinter(*data_ptr->printer, ...); b) name these configurer_ptr and printer_ptr and make both of them explicitly typed pointers (printer_ptr can be dereferenced in the SetUpPrinter call). https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:216: if (data->hotplugged) { Remove the if () since it doesn't do anything yet, just modify the comment. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: std::unordered_map<std::string, Printer> failed_setup_printers_; having a chromeos::Printer class BTW is super confusing; a little namespacing (e.g. chromeos::printing) would go a long way to help devs figure out where 'Printer' is defined. It doesn't help that it is in printer_configuration.h not printer.h; the file or the class should be renamed. (Not in this CL of course, but something to consider as a cleanup pass). Printer is a non trivial class that we don't appear to actually use yet. Let's not add it until we need it. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_configurer.h (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_configurer.h:17: enum PrinterSetupResult { Thank you for the less generic name :) It would be even better if we could scope this in a class somewhere (or namespace all of this; the repository has 47 files with the name 'printer' in them). https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printing/usb_util.cc:35: std::string CupsURIEscape(const std::string& in) { What is 'in'? Document or use a more descriptive name, e.g. 'uri_in'. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/usb_util.h (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printing/usb_util.h:8: #define CHROME_BROWSER_CHROMEOS_PRINTING_USB_UTIL_H__ nit: Even though this is in chromeos/printing, it would be helpful for discoverability to name the file something like usb_printer_util.
Thanks! PTAL? https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:18: #include "chrome/browser/chromeos/printer_detector/printer_detector.h" On 2017/03/17 17:34:10, stevenjb wrote: > nit: I don't know if our precommit allows it, but it would be nice if we can put > this include first since it is the associated header for this class. > Alternatively put the class definition in a header for improved discoverability. Indeed, presubmit doesn't allow this, sadly. I'm strongly opposed to putting implementation class declarations in a header as the things that are in the implementation class but not in the interface definition are explicitly local to this file, and putting them in a header makes that less clear, weakening the interface abstraction. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:175: auto* configurer_tmp = data->configurer.get(); On 2017/03/17 17:34:10, stevenjb wrote: > On 2017/03/16 04:11:29, tbarzic wrote: > > it would be nicer if this pattern could be avoided, but OK. > I agree this is confusing. We definitely shouldn't use auto here. Either: > > a) Use a local pointer to data instead: > > SetUpPrinterData* data_ptr = data.get(); > data_ptr->configurer->SetupPrinter(*data_ptr->printer, ...); > > b) name these configurer_ptr and printer_ptr and make both of them explicitly > typed pointers (printer_ptr can be dereferenced in the SetUpPrinter call). Using a local pointer to data is a much better idea, thanks. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:216: if (data->hotplugged) { On 2017/03/17 17:34:10, stevenjb wrote: > Remove the if () since it doesn't do anything yet, just modify the comment. Done. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: std::unordered_map<std::string, Printer> failed_setup_printers_; On 2017/03/17 17:34:10, stevenjb wrote: > having a chromeos::Printer class BTW is super confusing; a little namespacing > (e.g. chromeos::printing) would go a long way to help devs figure out where > 'Printer' is defined. It doesn't help that it is in printer_configuration.h not > printer.h; the file or the class should be renamed. (Not in this CL of course, > but something to consider as a cleanup pass). Yeah, this sort of cleanup is probably a bit overdue at this point. filed crbug/702710 > > Printer is a non trivial class that we don't appear to actually use yet. Let's > not add it until we need it. Disagree with this, because the next cl in this series directly depends on this structure and the entire flow of how failed_setup_printers is managed is much more related to this change than the followon; I think it will be easier to understand history with this change here instead of punting it to the next cl. If you really want to shift this to the next change, I will do that. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/printer_configurer.h (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printing/printer_configurer.h:17: enum PrinterSetupResult { On 2017/03/17 17:34:11, stevenjb wrote: > Thank you for the less generic name :) It would be even better if we could scope > this in a class somewhere (or namespace all of this; the repository has 47 files > with the name 'printer' in them). > I think this will take some thought about what makes sense in the larger context w.r.t. other symbols as well, and will be a part of the crbug/702710 cleanup. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/usb_util.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printing/usb_util.cc:35: std::string CupsURIEscape(const std::string& in) { On 2017/03/17 17:34:11, stevenjb wrote: > What is 'in'? Document or use a more descriptive name, e.g. 'uri_in'. Renamed uri_in https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printing/usb_util.h (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printing/usb_util.h:8: #define CHROME_BROWSER_CHROMEOS_PRINTING_USB_UTIL_H__ On 2017/03/17 17:34:11, stevenjb wrote: > nit: Even though this is in chromeos/printing, it would be helpful for > discoverability to name the file something like usb_printer_util. Done.
owner lgtm w/ the one comment/suggestion. https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: std::unordered_map<std::string, Printer> failed_setup_printers_; On 2017/03/17 18:47:34, Carlson wrote: > On 2017/03/17 17:34:10, stevenjb wrote: > > having a chromeos::Printer class BTW is super confusing; a little namespacing > > (e.g. chromeos::printing) would go a long way to help devs figure out where > > 'Printer' is defined. It doesn't help that it is in printer_configuration.h > not > > printer.h; the file or the class should be renamed. (Not in this CL of course, > > but something to consider as a cleanup pass). > > Yeah, this sort of cleanup is probably a bit overdue at this point. filed > crbug/702710 > > > > > Printer is a non trivial class that we don't appear to actually use yet. Let's > > not add it until we need it. > > Disagree with this, because the next cl in this series directly depends on this > structure and the entire flow of how failed_setup_printers is managed is much > more related to this change than the followon; I think it will be easier to > understand history with this change here instead of punting it to the next cl. > > If you really want to shift this to the next change, I will do that. > I would strongly encourage moving this to the next CL. My current thinking is that storing |Printer| may be expensive and unnecessary, but since we don't actually use the value there is no way for me to tell. It just involves moving a few lines to the CL where they are actually used.
https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: std::unordered_map<std::string, Printer> failed_setup_printers_; On 2017/03/17 20:49:54, stevenjb wrote: > On 2017/03/17 18:47:34, Carlson wrote: > > On 2017/03/17 17:34:10, stevenjb wrote: > > > having a chromeos::Printer class BTW is super confusing; a little > namespacing > > > (e.g. chromeos::printing) would go a long way to help devs figure out where > > > 'Printer' is defined. It doesn't help that it is in printer_configuration.h > > not > > > printer.h; the file or the class should be renamed. (Not in this CL of > course, > > > but something to consider as a cleanup pass). > > > > Yeah, this sort of cleanup is probably a bit overdue at this point. filed > > crbug/702710 > > > > > > > > Printer is a non trivial class that we don't appear to actually use yet. > Let's > > > not add it until we need it. > > > > Disagree with this, because the next cl in this series directly depends on > this > > structure and the entire flow of how failed_setup_printers is managed is much > > more related to this change than the followon; I think it will be easier to > > understand history with this change here instead of punting it to the next cl. > > > > If you really want to shift this to the next change, I will do that. > > > > I would strongly encourage moving this to the next CL. My current thinking is > that storing |Printer| may be expensive and unnecessary, but since we don't > actually use the value there is no way for me to tell. It just involves moving a > few lines to the CL where they are actually used. I think what you're worried about here is a micro-optimization at best; the structure involved is around 100-200 bytes, and we only have one of them per USB printer plugged into the device that we haven't been able to configure. But I'm more interested in getting this in than trying to win this argument, so done.
https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: std::unordered_map<std::string, Printer> failed_setup_printers_; On 2017/03/17 21:08:17, Carlson wrote: > On 2017/03/17 20:49:54, stevenjb wrote: > > On 2017/03/17 18:47:34, Carlson wrote: > > > On 2017/03/17 17:34:10, stevenjb wrote: > > > > having a chromeos::Printer class BTW is super confusing; a little > > namespacing > > > > (e.g. chromeos::printing) would go a long way to help devs figure out > where > > > > 'Printer' is defined. It doesn't help that it is in > printer_configuration.h > > > not > > > > printer.h; the file or the class should be renamed. (Not in this CL of > > course, > > > > but something to consider as a cleanup pass). > > > > > > Yeah, this sort of cleanup is probably a bit overdue at this point. filed > > > crbug/702710 > > > > > > > > > > > Printer is a non trivial class that we don't appear to actually use yet. > > Let's > > > > not add it until we need it. > > > > > > Disagree with this, because the next cl in this series directly depends on > > this > > > structure and the entire flow of how failed_setup_printers is managed is > much > > > more related to this change than the followon; I think it will be easier to > > > understand history with this change here instead of punting it to the next > cl. > > > > > > If you really want to shift this to the next change, I will do that. > > > > > > > I would strongly encourage moving this to the next CL. My current thinking is > > that storing |Printer| may be expensive and unnecessary, but since we don't > > actually use the value there is no way for me to tell. It just involves moving > a > > few lines to the CL where they are actually used. > > I think what you're worried about here is a micro-optimization at best; the > structure involved is around 100-200 bytes, and we only have one of them per USB > printer plugged into the device that we haven't been able to configure. > > But I'm more interested in getting this in than trying to win this argument, so > done. I most likely agree, but my point is since this is unused in this CL it is impossible for me to tell, which is why we tend to strongly discourage committing commented out or unused code. Thanks for removing it.
On 2017/03/17 21:21:13, stevenjb wrote: > https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... > File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): > > https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... > chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: > std::unordered_map<std::string, Printer> failed_setup_printers_; > On 2017/03/17 21:08:17, Carlson wrote: > > On 2017/03/17 20:49:54, stevenjb wrote: > > > On 2017/03/17 18:47:34, Carlson wrote: > > > > On 2017/03/17 17:34:10, stevenjb wrote: > > > > > having a chromeos::Printer class BTW is super confusing; a little > > > namespacing > > > > > (e.g. chromeos::printing) would go a long way to help devs figure out > > where > > > > > 'Printer' is defined. It doesn't help that it is in > > printer_configuration.h > > > > not > > > > > printer.h; the file or the class should be renamed. (Not in this CL of > > > course, > > > > > but something to consider as a cleanup pass). > > > > > > > > Yeah, this sort of cleanup is probably a bit overdue at this point. filed > > > > crbug/702710 > > > > > > > > > > > > > > Printer is a non trivial class that we don't appear to actually use yet. > > > Let's > > > > > not add it until we need it. > > > > > > > > Disagree with this, because the next cl in this series directly depends on > > > this > > > > structure and the entire flow of how failed_setup_printers is managed is > > much > > > > more related to this change than the followon; I think it will be easier > to > > > > understand history with this change here instead of punting it to the next > > cl. > > > > > > > > If you really want to shift this to the next change, I will do that. > > > > > > > > > > I would strongly encourage moving this to the next CL. My current thinking > is > > > that storing |Printer| may be expensive and unnecessary, but since we don't > > > actually use the value there is no way for me to tell. It just involves > moving > > a > > > few lines to the CL where they are actually used. > > > > I think what you're worried about here is a micro-optimization at best; the > > structure involved is around 100-200 bytes, and we only have one of them per > USB > > printer plugged into the device that we haven't been able to configure. > > > > But I'm more interested in getting this in than trying to win this argument, > so > > done. > > I most likely agree, but my point is since this is unused in this CL it is > impossible for me to tell, which is why we tend to strongly discourage > committing commented out or unused code. Thanks for removing it. I appreciate the attention to detail, even if I disagree with some of the conclusions :). I can haz lg?
On 2017/03/17 21:29:14, Carlson wrote: > On 2017/03/17 21:21:13, stevenjb wrote: > > > https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... > > File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc > (right): > > > > > https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... > > chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: > > std::unordered_map<std::string, Printer> failed_setup_printers_; > > On 2017/03/17 21:08:17, Carlson wrote: > > > On 2017/03/17 20:49:54, stevenjb wrote: > > > > On 2017/03/17 18:47:34, Carlson wrote: > > > > > On 2017/03/17 17:34:10, stevenjb wrote: > > > > > > having a chromeos::Printer class BTW is super confusing; a little > > > > namespacing > > > > > > (e.g. chromeos::printing) would go a long way to help devs figure out > > > where > > > > > > 'Printer' is defined. It doesn't help that it is in > > > printer_configuration.h > > > > > not > > > > > > printer.h; the file or the class should be renamed. (Not in this CL of > > > > course, > > > > > > but something to consider as a cleanup pass). > > > > > > > > > > Yeah, this sort of cleanup is probably a bit overdue at this point. > filed > > > > > crbug/702710 > > > > > > > > > > > > > > > > > Printer is a non trivial class that we don't appear to actually use > yet. > > > > Let's > > > > > > not add it until we need it. > > > > > > > > > > Disagree with this, because the next cl in this series directly depends > on > > > > this > > > > > structure and the entire flow of how failed_setup_printers is managed is > > > much > > > > > more related to this change than the followon; I think it will be easier > > to > > > > > understand history with this change here instead of punting it to the > next > > > cl. > > > > > > > > > > If you really want to shift this to the next change, I will do that. > > > > > > > > > > > > > I would strongly encourage moving this to the next CL. My current thinking > > is > > > > that storing |Printer| may be expensive and unnecessary, but since we > don't > > > > actually use the value there is no way for me to tell. It just involves > > moving > > > a > > > > few lines to the CL where they are actually used. > > > > > > I think what you're worried about here is a micro-optimization at best; the > > > structure involved is around 100-200 bytes, and we only have one of them per > > USB > > > printer plugged into the device that we haven't been able to configure. > > > > > > But I'm more interested in getting this in than trying to win this argument, > > so > > > done. > > > > I most likely agree, but my point is since this is unused in this CL it is > > impossible for me to tell, which is why we tend to strongly discourage > > committing commented out or unused code. Thanks for removing it. > > I appreciate the attention to detail, even if I disagree with some of the > conclusions :). > > I can haz lg? I already did, but still lgtm :)
On 2017/03/17 21:30:34, stevenjb wrote: > On 2017/03/17 21:29:14, Carlson wrote: > > On 2017/03/17 21:21:13, stevenjb wrote: > > > > > > https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... > > > File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc > > (right): > > > > > > > > > https://codereview.chromium.org/2738323003/diff/160001/chrome/browser/chromeo... > > > chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:244: > > > std::unordered_map<std::string, Printer> failed_setup_printers_; > > > On 2017/03/17 21:08:17, Carlson wrote: > > > > On 2017/03/17 20:49:54, stevenjb wrote: > > > > > On 2017/03/17 18:47:34, Carlson wrote: > > > > > > On 2017/03/17 17:34:10, stevenjb wrote: > > > > > > > having a chromeos::Printer class BTW is super confusing; a little > > > > > namespacing > > > > > > > (e.g. chromeos::printing) would go a long way to help devs figure > out > > > > where > > > > > > > 'Printer' is defined. It doesn't help that it is in > > > > printer_configuration.h > > > > > > not > > > > > > > printer.h; the file or the class should be renamed. (Not in this CL > of > > > > > course, > > > > > > > but something to consider as a cleanup pass). > > > > > > > > > > > > Yeah, this sort of cleanup is probably a bit overdue at this point. > > filed > > > > > > crbug/702710 > > > > > > > > > > > > > > > > > > > > Printer is a non trivial class that we don't appear to actually use > > yet. > > > > > Let's > > > > > > > not add it until we need it. > > > > > > > > > > > > Disagree with this, because the next cl in this series directly > depends > > on > > > > > this > > > > > > structure and the entire flow of how failed_setup_printers is managed > is > > > > much > > > > > > more related to this change than the followon; I think it will be > easier > > > to > > > > > > understand history with this change here instead of punting it to the > > next > > > > cl. > > > > > > > > > > > > If you really want to shift this to the next change, I will do that. > > > > > > > > > > > > > > > > I would strongly encourage moving this to the next CL. My current > thinking > > > is > > > > > that storing |Printer| may be expensive and unnecessary, but since we > > don't > > > > > actually use the value there is no way for me to tell. It just involves > > > moving > > > > a > > > > > few lines to the CL where they are actually used. > > > > > > > > I think what you're worried about here is a micro-optimization at best; > the > > > > structure involved is around 100-200 bytes, and we only have one of them > per > > > USB > > > > printer plugged into the device that we haven't been able to configure. > > > > > > > > But I'm more interested in getting this in than trying to win this > argument, > > > so > > > > done. > > > > > > I most likely agree, but my point is since this is unused in this CL it is > > > impossible for me to tell, which is why we tend to strongly discourage > > > committing commented out or unused code. Thanks for removing it. > > > > I appreciate the attention to detail, even if I disagree with some of the > > conclusions :). > > > > I can haz lg? > > I already did, but still lgtm :) You mean I missed my opportunity to passive-aggressively ignore this suggestion? <sigh> :)
The CQ bit was checked by justincarlson@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org, tbarzic@chromium.org, stevenjb@chromium.org Link to the patchset: https://codereview.chromium.org/2738323003/#ps220001 (title: "Fix ppd provider lifetime. It was being inappropriately deleted which meant it caused USB resoluti…")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1490121271096420,
"parent_rev": "6d7c10b353826bfdeb01dcb6c3a41546507b8d4d", "commit_rev":
"506ac3f696915a9cf1e8da551e9f68b8a6408b45"}
Message was sent while issue was closed.
Description was changed from ========== Implement basic USB setup in the cups printer detector. Factor out some utility usb functions into another library (these will be reused in discovery). With this change, if you plug in a USB printer that we handle while connected to the network, it should automatically get set up. While I'm in there, rename chromeos::SetupResult to chromeos::PrinterSetupResult to prevent polluting the chromeos namespace with a relatively generic name. BUG=616866 ========== to ========== Implement basic USB setup in the cups printer detector. Factor out some utility usb functions into another library (these will be reused in discovery). With this change, if you plug in a USB printer that we handle while connected to the network, it should automatically get set up. While I'm in there, rename chromeos::SetupResult to chromeos::PrinterSetupResult to prevent polluting the chromeos namespace with a relatively generic name. BUG=616866 Review-Url: https://codereview.chromium.org/2738323003 Cr-Commit-Position: refs/heads/master@{#458536} Committed: https://chromium.googlesource.com/chromium/src/+/506ac3f696915a9cf1e8da551e9f... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/506ac3f696915a9cf1e8da551e9f... |
