Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2790603003:
Make CUPS USB printing play better with the settings page. This change does several things: (Closed)
|
Created:
3 years, 8 months ago by Carlson Modified:
3 years, 8 months ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, arv+watch_chromium.org, oshima+watch_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake CUPS USB printing play better with the settings page. This change does several things:
* Provides a real implementation of PrinterDiscoverer, and re-enables
it.
* Adds support for observers to CupsPrinterDetector,
* Connects PrinterDiscoverer to CupsPrinterDetector, so
PrinterDiscoverer knows about USB printers
BUGS=616866,706582
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2790603003
Cr-Commit-Position: refs/heads/master@{#463738}
Committed: https://chromium.googlesource.com/chromium/src/+/e5243e7446e4bae1bdeb43f1d4c526de3bf939f5
Patch Set 1 #Patch Set 2 : Pre-review cleanups #
Total comments: 8
Patch Set 3 : Address skau@ comments #
Total comments: 7
Patch Set 4 : Address xdai@ comments #
Total comments: 35
Patch Set 5 : Address stevenjb@ comments, revert ui changes #Patch Set 6 : stop latching PrintersManager, add comments #Messages
Total messages: 29 (11 generated)
Description was changed from ========== Make CUPS USB printing play better with the settings page. This change does several things: * Provides a real implementation of PrinterDiscoverer, and re-enables it. * Adds support for observers to CupsPrinterDetector, * Connects PrinterDiscoverer to CupsPrinterDetector, so PrinterDiscoverer knows about USB printers * Updates the settings UI so that, when you click "add" from the Discovered printers list, it takes you to the manu/model dropdown dialog to select a driver. (This is sane behavior for USB, but will probably need to be tweaked when auto-discovered network printers are also enabled). Note that the UI changes, in particular, are done by someone who is *not* a UI person. Please feel (extra) free to tell me I've done it all wrong, but hopefully with a suggestion of how to do it in a more appropriate way. BUGS=616866,706582 BUG= ========== to ========== Make CUPS USB printing play better with the settings page. This change does several things: * Provides a real implementation of PrinterDiscoverer, and re-enables it. * Adds support for observers to CupsPrinterDetector, * Connects PrinterDiscoverer to CupsPrinterDetector, so PrinterDiscoverer knows about USB printers * Updates the settings UI so that, when you click "add" from the Discovered printers list, it takes you to the manu/model dropdown dialog to select a driver. (This is sane behavior for USB, but will probably need to be tweaked when auto-discovered network printers are also enabled). Note that the UI changes, in particular, are done by someone who is *not* a UI person. Please feel (extra) free to tell me I've done it all wrong, but hopefully with a suggestion of how to do it in a more appropriate way. BUGS=616866,706582 BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
justincarlson@chromium.org changed reviewers: + skau@chromium.org, stevenjb@chromium.org, xdai@chromium.org
Requesting: skau: all the c++ parts, owners signoff on /c/b/chromeos/printing/* xdai: c/b/resources/* changes stevenjb: c/b/chromeos/printer_detector/*, /c/b/resources/* (suggest holding off on the latter until after xdai has a pass to check sanity of approach). Certainly open to feedback on other parts as well, if you feel so inclined. :)
lgtm https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (left): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:1231: "printing/fake_printer_discoverer.h", I know I told you to keep these, but can you delete them? We can always resurrect them from the git history. If they're not compiled, they'll just get out of date anyway. https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:147: FROM_HERE, &PrinterDetector::Observer::OnAvailableUsbPrintersChanged, Add a note to this method that calling GetPrinters() will cause a deadlock. https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:65: // This wrapper buys us weak_ptr semantics on the Discoverer object, and also Do you mean you get weak_ptr semantics on the Observer? I'm unclear what object you're protecting against here.
https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/BUILD.gn (left): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/BUILD.gn:1231: "printing/fake_printer_discoverer.h", On 2017/03/31 17:50:11, skau wrote: > I know I told you to keep these, but can you delete them? We can always > resurrect them from the git history. If they're not compiled, they'll just get > out of date anyway. Done. https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:147: FROM_HERE, &PrinterDetector::Observer::OnAvailableUsbPrintersChanged, On 2017/03/31 17:50:11, skau wrote: > Add a note to this method that calling GetPrinters() will cause a deadlock. Done. (I really, really wish we had lock annotations. :P) https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:65: // This wrapper buys us weak_ptr semantics on the Discoverer object, and also On 2017/03/31 17:50:11, skau wrote: > Do you mean you get weak_ptr semantics on the Observer? I'm unclear what object > you're protecting against here. Long comment is insufficiently long. :) This is ultimately motivated by the fact that we want to ensure that a new observer of this class gets an OnPrintersFound callback with the current state, not just when the state changes. So when the observer is added, I want to schedule an OnPrintersFound callback. But in between the time that I post that task and the time it executes, a couple things could happen. 1) The PrinterDiscoverer instance could be destroyed. In which case we don't want to execute the callback 2) The observer that was added could be destroyed (or, equivalently for our purposes, removed as an observer of this class). In that case we also don't want to do the callback. The PrinterDiscoverer weak_ptr stuff protects us from the first scenario. The HasObserver call protects us from the second. Doing these things requires the callback that we schedule at line 41 in AddObserver calls back to this wrapper instead of directly to the observer itself. Does that make sense? If so, any suggestions on how to make the comment clearer?
https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:65: // This wrapper buys us weak_ptr semantics on the Discoverer object, and also On 2017/03/31 18:20:07, Carlson wrote: > On 2017/03/31 17:50:11, skau wrote: > > Do you mean you get weak_ptr semantics on the Observer? I'm unclear what > object > > you're protecting against here. > > Long comment is insufficiently long. :) > > This is ultimately motivated by the fact that we want to ensure that a new > observer of this class gets an OnPrintersFound callback with the current state, > not just when the state changes. So when the observer is added, I want to > schedule an OnPrintersFound callback. > > But in between the time that I post that task and the time it executes, a couple > things could happen. > > 1) The PrinterDiscoverer instance could be destroyed. In which case we don't > want to execute the callback > 2) The observer that was added could be destroyed (or, equivalently for our > purposes, removed as an observer of this class). In that case we also don't > want to do the callback. > > The PrinterDiscoverer weak_ptr stuff protects us from the first scenario. The > HasObserver call protects us from the second. Doing these things requires the > callback that we schedule at line 41 in AddObserver calls back to this wrapper > instead of directly to the observer itself. > > Does that make sense? If so, any suggestions on how to make the comment > clearer? That makes more sense. You should probably mention where the method is called since that informs the requirements. I'm assuming this method does't need to be thread safe because it's sequenced?
See inline comments. https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (left): https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:320: selectedPrinter: { Could you try to see the following scenario: Open the discovery dialog, click and select one printer from the discovered list, do nothing, and click on the manually add printer button, observe all the fields. Does the printer from the discovered list influence the fields in the manually add printer flow? https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:78: // This update happened after we already finished the initial nit: comment indent off: 'scan' should be in the previous line. https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:112: switchToConfiguringDialog_: function() { I think you don't need this function any more?
https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:65: // This wrapper buys us weak_ptr semantics on the Discoverer object, and also On 2017/03/31 21:03:19, skau wrote: > On 2017/03/31 18:20:07, Carlson wrote: > > On 2017/03/31 17:50:11, skau wrote: > > > Do you mean you get weak_ptr semantics on the Observer? I'm unclear what > > object > > > you're protecting against here. > > > > Long comment is insufficiently long. :) > > > > This is ultimately motivated by the fact that we want to ensure that a new > > observer of this class gets an OnPrintersFound callback with the current > state, > > not just when the state changes. So when the observer is added, I want to > > schedule an OnPrintersFound callback. > > > > But in between the time that I post that task and the time it executes, a > couple > > things could happen. > > > > 1) The PrinterDiscoverer instance could be destroyed. In which case we don't > > want to execute the callback > > 2) The observer that was added could be destroyed (or, equivalently for our > > purposes, removed as an observer of this class). In that case we also don't > > want to do the callback. > > > > The PrinterDiscoverer weak_ptr stuff protects us from the first scenario. The > > HasObserver call protects us from the second. Doing these things requires the > > callback that we schedule at line 41 in AddObserver calls back to this wrapper > > instead of directly to the observer itself. > > > > Does that make sense? If so, any suggestions on how to make the comment > > clearer? > > That makes more sense. You should probably mention where the method is called > since that informs the requirements. I'm assuming this method does't need to be > thread safe because it's sequenced? Moved the bulk of the comment to the callsite and reworded a bit. Yes to the question (the sequencing requirement is in a comment at the interface level) https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (left): https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:320: selectedPrinter: { On 2017/03/31 22:08:38, xdai1 wrote: > Could you try to see the following scenario: > > Open the discovery dialog, click and select one printer from the discovered > list, do nothing, and click on the manually add printer button, observe all the > fields. Does the printer from the discovered list influence the fields in the > manually add printer flow? Oh, you're good. Yes, indeed, this is what happens, we pre-populate the manual printer fields inappropriately, and I see why. Clearing newPrinter on the switch from discovery to manual fixes this, and I've done that for now, but is that the right way to do this? I could restore the "selected-printer" changes and copy over selectedPrinter to newPrinter on a discovery->manufacturer transition instead. However, I do like keeping things as newPrinter generally as it simplified some other things below. Let me know which sort of fix you prefer. https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (right): https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:78: // This update happened after we already finished the initial On 2017/03/31 22:08:37, xdai1 wrote: > nit: comment indent off: 'scan' should be in the previous line. Done. https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:112: switchToConfiguringDialog_: function() { On 2017/03/31 22:08:38, xdai1 wrote: > I think you don't need this function any more? Done.
Please see my inline comments. https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js (left): https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:320: selectedPrinter: { On 2017/03/31 22:58:41, Carlson wrote: > On 2017/03/31 22:08:38, xdai1 wrote: > > Could you try to see the following scenario: > > > > Open the discovery dialog, click and select one printer from the discovered > > list, do nothing, and click on the manually add printer button, observe all > the > > fields. Does the printer from the discovered list influence the fields in the > > manually add printer flow? > > Oh, you're good. > > Yes, indeed, this is what happens, we pre-populate the manual printer fields > inappropriately, and I see why. > > Clearing newPrinter on the switch from discovery to manual fixes this, and I've > done that for now, but is that the right way to do this? I could restore the > "selected-printer" changes and copy over selectedPrinter to newPrinter on a > discovery->manufacturer transition instead. > > However, I do like keeping things as newPrinter generally as it simplified some > other things below. > > Let me know which sort of fix you prefer. I would prefer to restore the 'selectedPrinter' change. There are two reasons. The first reason please refer my comment in cups_add_printer_dialog.html. The second reason is the user may need to re-type the information he just entered unnecessarily. Suppose the following scenario: Open the discovery dialog, then click on manual add printer button, enter the printer information, then click on add nearby printer button, do nothing, then click on manual add printer button again. I would expect all the information still remains here. But I don't have strong opinions here. If you really want to remove selectedPrinter variable, make sure you also fix the case I described in cups_add_printer_dialog.html. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:307: on-tap="switchToManualAddDialog_"> If you want to remove selectedPrinter variable, you might also need to modify the function switchToManualAddDialog() here. Suppose the scenario like this: Open discovery dialog, select one printer from the list, click Add, then the manufacturer-model dialog shows up, select the manufacturer and model from the drop down list, do nothing and then click on this manual add printer button. I think the same problem as I described in previous comment happens. Note you don't want to simply reset the newPrinter here because if manufacture-model dialog was opened by the manual-add-printer dialog, resetting the newPrinter will make you lost all the fields you just entered...
On 2017/03/31 22:58:41, Carlson wrote: > https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/printing/printer_discoverer.cc (right): > > https://codereview.chromium.org/2790603003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/printing/printer_discoverer.cc:65: // This wrapper buys > us weak_ptr semantics on the Discoverer object, and also > On 2017/03/31 21:03:19, skau wrote: > > On 2017/03/31 18:20:07, Carlson wrote: > > > On 2017/03/31 17:50:11, skau wrote: > > > > Do you mean you get weak_ptr semantics on the Observer? I'm unclear what > > > object > > > > you're protecting against here. > > > > > > Long comment is insufficiently long. :) > > > > > > This is ultimately motivated by the fact that we want to ensure that a new > > > observer of this class gets an OnPrintersFound callback with the current > > state, > > > not just when the state changes. So when the observer is added, I want to > > > schedule an OnPrintersFound callback. > > > > > > But in between the time that I post that task and the time it executes, a > > couple > > > things could happen. > > > > > > 1) The PrinterDiscoverer instance could be destroyed. In which case we > don't > > > want to execute the callback > > > 2) The observer that was added could be destroyed (or, equivalently for our > > > purposes, removed as an observer of this class). In that case we also don't > > > want to do the callback. > > > > > > The PrinterDiscoverer weak_ptr stuff protects us from the first scenario. > The > > > HasObserver call protects us from the second. Doing these things requires > the > > > callback that we schedule at line 41 in AddObserver calls back to this > wrapper > > > instead of directly to the observer itself. > > > > > > Does that make sense? If so, any suggestions on how to make the comment > > > clearer? > > > > That makes more sense. You should probably mention where the method is called > > since that informs the requirements. I'm assuming this method does't need to > be > > thread safe because it's sequenced? > > Moved the bulk of the comment to the callsite and reworded a bit. > > Yes to the question (the sequencing requirement is in a comment at the interface > level) > > https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js > (left): > > https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:320: > selectedPrinter: { > On 2017/03/31 22:08:38, xdai1 wrote: > > Could you try to see the following scenario: > > > > Open the discovery dialog, click and select one printer from the discovered > > list, do nothing, and click on the manually add printer button, observe all > the > > fields. Does the printer from the discovered list influence the fields in the > > manually add printer flow? > > Oh, you're good. > > Yes, indeed, this is what happens, we pre-populate the manual printer fields > inappropriately, and I see why. > > Clearing newPrinter on the switch from discovery to manual fixes this, and I've > done that for now, but is that the right way to do this? I could restore the > "selected-printer" changes and copy over selectedPrinter to newPrinter on a > discovery->manufacturer transition instead. > > However, I do like keeping things as newPrinter generally as it simplified some > other things below. > > Let me know which sort of fix you prefer. > > https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... > File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js > (right): > > https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:78: > // This update happened after we already finished the initial > On 2017/03/31 22:08:37, xdai1 wrote: > > nit: comment indent off: 'scan' should be in the previous line. > > Done. > > https://codereview.chromium.org/2790603003/diff/40001/chrome/browser/resource... > chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.js:112: > switchToConfiguringDialog_: function() { > On 2017/03/31 22:08:38, xdai1 wrote: > > I think you don't need this function any more? > > Done. Alas, I'm not going to get this in today. Clearly I need to understand more about how polymer data bindings work to make sure I'm not doing something unexpected in the UI. I'm off next week, but will pick this up again a week from Monday.
https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:107: return ret; local not needed. Also, we could avoid an extra copy by passing a vector* to GetPrinters() instead of returning a vector. Alternately, return a scoped_ptr and pass ownership. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:150: GetPrintersLocked()); Each call to Notify is going to build a new vector. Instead call GetPrintersLocked() once and pass the result to each observer. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:185: auto existing_printer_configuration = Avoid using auto when the return type is not obvious to someone not super familiar with the code (like me). auto is good for iterators and things like MakeUnique. I assume this gets a Printer, but is it a *, scoped_pt, or &? Better to be explicit here. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:258: *printer_copy = *data->printer; I think this would be more clear as: auto printer_copy = base::MakeUnique<Printer>(new Printer(*data->printer)); https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/printer_detector.h (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/printer_detector.h:13: #include "base/memory/ref_counted.h" needed? https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/printer_detector.h:44: // The set of available printers has changed nit: . https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:30: PrintersManagerFactory::GetForBrowserContext(profile)), Even though this class is associated with |profile|, it is not a keyed service so there is no obvious guarantee that it will outlive this keyed service (and there is no explicit dependency). Rather than store this in |printers_manager_|, it would be better to get the pointer as needed. That way at least to code will crash on a CHECK or a null pointer dereference rather than accessing a potentially invalid pointer. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:33: PrinterDetectorFactory::GetInstance()->Get(profile); DCHECK(detector); https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:50: GetAvailablePrinters())); This is awkward. I would suggest instead providing a GetAllPrinters() method that can be called explicitly instead. e.g. SomeClass::Init() { printer_discoverer_->AddObserver(this); UpdatePrinters(printer_discoverer_->GetAllPrinters()); } SomeClass::OnPrintersFound(printers) { UpdatePrinters(printers); } https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:99: ScopedObserver<PrinterDetector, PrinterDetector::Observer> observer_; nit: detector_observer_ would be a little more clear since this class also has its own observer list. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.h (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.h:44: static std::unique_ptr<PrinterDiscoverer> Create(Profile* profile); Maybe rename this CreateForProfile for clarity? https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.h:50: // oberserver more than once. Callbacks to the observer will take place nit: 'Calls to |observer| methods' https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:52: new-printer="{{newPrinter}}"> previous indent was correct
Also, can we move the UI changes to a separate CL? They do not appear to be necessary for the non UI changes?
Thanks for the review. Removed the UI changes from this cl. Also I could use clarification on a few of your comments in printer_discoverer.cc PTAL? https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:107: return ret; On 2017/04/10 19:49:55, stevenjb wrote: > local not needed. > > Also, we could avoid an extra copy by passing a vector* to GetPrinters() instead > of returning a vector. > Alternately, return a scoped_ptr and pass ownership. > Removed temporary. RE: efficiency, is there a reason we expect RVO and/or rvalue reference assignment/construction to not work here? I would expect basically no efficiency gains by going down the pass-a-pointer or return-a-unique-ptr routes. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:150: GetPrintersLocked()); On 2017/04/10 19:49:55, stevenjb wrote: > Each call to Notify is going to build a new vector. Instead call > GetPrintersLocked() once and pass the result to each observer. I don't understand your concern here, sorry. There is only one call here, not a looped construct. What am I missing? https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:185: auto existing_printer_configuration = On 2017/04/10 19:49:55, stevenjb wrote: > Avoid using auto when the return type is not obvious to someone not super > familiar with the code (like me). auto is good for iterators and things like > MakeUnique. I assume this gets a Printer, but is it a *, scoped_pt, or &? > Better to be explicit here. > Done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:258: *printer_copy = *data->printer; On 2017/04/10 19:49:55, stevenjb wrote: > I think this would be more clear as: > auto printer_copy = base::MakeUnique<Printer>(new Printer(*data->printer)); > Don't need the "new Printer" part, but otherwise, done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/printer_detector.h (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/printer_detector.h:13: #include "base/memory/ref_counted.h" On 2017/04/10 19:49:55, stevenjb wrote: > needed? Removed https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/printer_detector.h:44: // The set of available printers has changed On 2017/04/10 19:49:55, stevenjb wrote: > nit: . Done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:30: PrintersManagerFactory::GetForBrowserContext(profile)), On 2017/04/10 19:49:55, stevenjb wrote: > Even though this class is associated with |profile|, it is not a keyed service > so there is no obvious guarantee that it will outlive this keyed service (and > there is no explicit dependency). > Rather than store this in |printers_manager_|, it would be better to get the > pointer as needed. That way at least to code will crash on a CHECK or a null > pointer dereference rather than accessing a potentially invalid pointer. Hmm, this is a good point. Practically speaking, the limited lifetime of the Discoverer means I don't think it's possible to run into this issue, but that's an ugly assumption to embed into the class in the long term. I would happily turn this into a keyed service itself, if I could figure out how to limit the lifetime appropriately. My impression is that keyed services, once instantiated, don't go away until the associated profile does, and I don't want an instance of this class hanging around chewing up memory all the time when we only care about printer discovery in very limited circumstances. Is there any reasonably elegant way to do limited lifetime *and* have this be a keyed service? https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:33: PrinterDetectorFactory::GetInstance()->Get(profile); On 2017/04/10 19:49:55, stevenjb wrote: > DCHECK(detector); Done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:50: GetAvailablePrinters())); On 2017/04/10 19:49:55, stevenjb wrote: > This is awkward. I would suggest instead providing a GetAllPrinters() method > that can be called explicitly instead. > > e.g. > > SomeClass::Init() { > printer_discoverer_->AddObserver(this); > UpdatePrinters(printer_discoverer_->GetAllPrinters()); > } > > SomeClass::OnPrintersFound(printers) { > UpdatePrinters(printers); > } I don't think I understand what you're suggesting here. Are you suggesting that we shift responsibility for doing the initial query to the observer, and expose a PrinterDiscoverer method for the observer to make that query? If so, I'd really argue against that, because I'd rather have a simpler external interface with an uglier implementation than complicate the interface to buy a little bit simpler implementation. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:99: ScopedObserver<PrinterDetector, PrinterDetector::Observer> observer_; On 2017/04/10 19:49:55, stevenjb wrote: > nit: detector_observer_ would be a little more clear since this class also has > its own observer list. Done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.h (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.h:44: static std::unique_ptr<PrinterDiscoverer> Create(Profile* profile); On 2017/04/10 19:49:55, stevenjb wrote: > Maybe rename this CreateForProfile for clarity? Done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.h:50: // oberserver more than once. Callbacks to the observer will take place On 2017/04/10 19:49:55, stevenjb wrote: > nit: 'Calls to |observer| methods' Done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/resource... File chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/resource... chrome/browser/resources/settings/printing_page/cups_add_printer_dialog.html:52: new-printer="{{newPrinter}}"> On 2017/04/10 19:49:55, stevenjb wrote: > previous indent was correct Removed from this changelist
Description was changed from ========== Make CUPS USB printing play better with the settings page. This change does several things: * Provides a real implementation of PrinterDiscoverer, and re-enables it. * Adds support for observers to CupsPrinterDetector, * Connects PrinterDiscoverer to CupsPrinterDetector, so PrinterDiscoverer knows about USB printers * Updates the settings UI so that, when you click "add" from the Discovered printers list, it takes you to the manu/model dropdown dialog to select a driver. (This is sane behavior for USB, but will probably need to be tweaked when auto-discovered network printers are also enabled). Note that the UI changes, in particular, are done by someone who is *not* a UI person. Please feel (extra) free to tell me I've done it all wrong, but hopefully with a suggestion of how to do it in a more appropriate way. BUGS=616866,706582 BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make CUPS USB printing play better with the settings page. This change does several things: * Provides a real implementation of PrinterDiscoverer, and re-enables it. * Adds support for observers to CupsPrinterDetector, * Connects PrinterDiscoverer to CupsPrinterDetector, so PrinterDiscoverer knows about USB printers BUGS=616866,706582 BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
So the only blocker for me is storing printers_manager_. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printer_detector/cups_printer_detector.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:150: GetPrintersLocked()); On 2017/04/10 22:16:41, Carlson wrote: > On 2017/04/10 19:49:55, stevenjb wrote: > > Each call to Notify is going to build a new vector. Instead call > > GetPrintersLocked() once and pass the result to each observer. > > I don't understand your concern here, sorry. There is only one call here, not a > looped construct. What am I missing? Apologies I was somehow thinking this was inside a loop. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printer_detector/cups_printer_detector.cc:258: *printer_copy = *data->printer; On 2017/04/10 22:16:41, Carlson wrote: > On 2017/04/10 19:49:55, stevenjb wrote: > > I think this would be more clear as: > > auto printer_copy = base::MakeUnique<Printer>(new Printer(*data->printer)); > > > > Don't need the "new Printer" part, but otherwise, done. Acknowledged. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:30: PrintersManagerFactory::GetForBrowserContext(profile)), On 2017/04/10 22:16:42, Carlson wrote: > On 2017/04/10 19:49:55, stevenjb wrote: > > Even though this class is associated with |profile|, it is not a keyed service > > so there is no obvious guarantee that it will outlive this keyed service (and > > there is no explicit dependency). > > Rather than store this in |printers_manager_|, it would be better to get the > > pointer as needed. That way at least to code will crash on a CHECK or a null > > pointer dereference rather than accessing a potentially invalid pointer. > > Hmm, this is a good point. > > Practically speaking, the limited lifetime of the Discoverer means I don't think > it's possible to run into this issue, but that's an ugly assumption to embed > into the class in the long term. > > I would happily turn this into a keyed service itself, if I could figure out how > to limit the lifetime appropriately. My impression is that keyed services, once > instantiated, don't go away until the associated profile does, and I don't want > an instance of this class hanging around chewing up memory all the time when we > only care about printer discovery in very limited circumstances. > > Is there any reasonably elegant way to do limited lifetime *and* have this be a > keyed service? There may be but I don't know off the top of my head. You would have to examine the code. Otherwise, the overhead for calling PrintersManagerFactory::GetForBrowserContext as needed isn't significant. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:50: GetAvailablePrinters())); On 2017/04/10 22:16:41, Carlson wrote: > On 2017/04/10 19:49:55, stevenjb wrote: > > This is awkward. I would suggest instead providing a GetAllPrinters() method > > that can be called explicitly instead. > > > > e.g. > > > > SomeClass::Init() { > > printer_discoverer_->AddObserver(this); > > UpdatePrinters(printer_discoverer_->GetAllPrinters()); > > } > > > > SomeClass::OnPrintersFound(printers) { > > UpdatePrinters(printers); > > } > > I don't think I understand what you're suggesting here. Are you suggesting that > we shift responsibility for doing the initial query to the observer, and expose > a PrinterDiscoverer method for the observer to make that query? > > If so, I'd really argue against that, because I'd rather have a simpler external > interface with an uglier implementation than complicate the interface to buy a > little bit simpler implementation. So, that is what I was suggesting, it's a pretty common pattern, but I realize now that I was more confused than I realized. I see now that OnDiscoveryInitialScanDone() does not return the list, it calls OnPrintersFound() with the list and then it calls OnDiscoveryInitialScanDone() separately. I find personally this API confusing, especially since GetAvailablePrinters() is synchronous, but if the other reviewers are OK with it I won't push back against it.
https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:50: GetAvailablePrinters())); On 2017/04/11 16:27:42, stevenjb wrote: > On 2017/04/10 22:16:41, Carlson wrote: > > On 2017/04/10 19:49:55, stevenjb wrote: > > > This is awkward. I would suggest instead providing a GetAllPrinters() method > > > that can be called explicitly instead. > > > > > > e.g. > > > > > > SomeClass::Init() { > > > printer_discoverer_->AddObserver(this); > > > UpdatePrinters(printer_discoverer_->GetAllPrinters()); > > > } > > > > > > SomeClass::OnPrintersFound(printers) { > > > UpdatePrinters(printers); > > > } > > > > I don't think I understand what you're suggesting here. Are you suggesting > that > > we shift responsibility for doing the initial query to the observer, and > expose > > a PrinterDiscoverer method for the observer to make that query? > > > > If so, I'd really argue against that, because I'd rather have a simpler > external > > interface with an uglier implementation than complicate the interface to buy a > > little bit simpler implementation. > > So, that is what I was suggesting, it's a pretty common pattern, but I realize > now that I was more confused than I realized. > > I see now that OnDiscoveryInitialScanDone() does not return the list, it calls > OnPrintersFound() with the list and then it calls OnDiscoveryInitialScanDone() > separately. > > I find personally this API confusing, especially since GetAvailablePrinters() is > synchronous, but if the other reviewers are OK with it I won't push back against > it. The behavior for detection is a little abnormal. There are two phases of detection. First we actively scan for devices and emit OnDiscoveryInitialScanDone(). After that, we continue to listen for devices as we respond to a device being plugged in or turned on.
https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:50: GetAvailablePrinters())); On 2017/04/11 17:12:49, skau wrote: > On 2017/04/11 16:27:42, stevenjb wrote: > > On 2017/04/10 22:16:41, Carlson wrote: > > > On 2017/04/10 19:49:55, stevenjb wrote: > > > > This is awkward. I would suggest instead providing a GetAllPrinters() > method > > > > that can be called explicitly instead. > > > > > > > > e.g. > > > > > > > > SomeClass::Init() { > > > > printer_discoverer_->AddObserver(this); > > > > UpdatePrinters(printer_discoverer_->GetAllPrinters()); > > > > } > > > > > > > > SomeClass::OnPrintersFound(printers) { > > > > UpdatePrinters(printers); > > > > } > > > > > > I don't think I understand what you're suggesting here. Are you suggesting > > that > > > we shift responsibility for doing the initial query to the observer, and > > expose > > > a PrinterDiscoverer method for the observer to make that query? > > > > > > If so, I'd really argue against that, because I'd rather have a simpler > > external > > > interface with an uglier implementation than complicate the interface to buy > a > > > little bit simpler implementation. > > > > So, that is what I was suggesting, it's a pretty common pattern, but I realize > > now that I was more confused than I realized. > > > > I see now that OnDiscoveryInitialScanDone() does not return the list, it calls > > OnPrintersFound() with the list and then it calls OnDiscoveryInitialScanDone() > > separately. > > > > I find personally this API confusing, especially since GetAvailablePrinters() > is > > synchronous, but if the other reviewers are OK with it I won't push back > against > > it. > > The behavior for detection is a little abnormal. There are two phases of > detection. First we actively scan for devices and emit > OnDiscoveryInitialScanDone(). After that, we continue to listen for devices as > we respond to a device being plugged in or turned on. What confuses me is that "actively scan" appears to involve "call GetAvailablePrinters()" which is synchronous and doesn't appear to ensure that the active scan completed? If the intention is that this will eventually wait for a scan, but that isn't implemented yet, it might be nicer to set that up initially, i.e. use PostTaskAndReplyWithResult(... PrinterDiscovererImpl::ScanForPrinters, ... PrinterDiscovererImpl::WrappedOnPrintersFound). That would certainly make the intention more clear. You can comment ScanForPrinters with a TODO.
Wall o' text reply, sorry. :) PTAL? https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_discoverer.cc (right): https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:30: PrintersManagerFactory::GetForBrowserContext(profile)), On 2017/04/11 16:27:42, stevenjb wrote: > On 2017/04/10 22:16:42, Carlson wrote: > > On 2017/04/10 19:49:55, stevenjb wrote: > > > Even though this class is associated with |profile|, it is not a keyed > service > > > so there is no obvious guarantee that it will outlive this keyed service > (and > > > there is no explicit dependency). > > > Rather than store this in |printers_manager_|, it would be better to get the > > > pointer as needed. That way at least to code will crash on a CHECK or a null > > > pointer dereference rather than accessing a potentially invalid pointer. > > > > Hmm, this is a good point. > > > > Practically speaking, the limited lifetime of the Discoverer means I don't > think > > it's possible to run into this issue, but that's an ugly assumption to embed > > into the class in the long term. > > > > I would happily turn this into a keyed service itself, if I could figure out > how > > to limit the lifetime appropriately. My impression is that keyed services, > once > > instantiated, don't go away until the associated profile does, and I don't > want > > an instance of this class hanging around chewing up memory all the time when > we > > only care about printer discovery in very limited circumstances. > > > > Is there any reasonably elegant way to do limited lifetime *and* have this be > a > > keyed service? > > There may be but I don't know off the top of my head. You would have to examine > the code. Otherwise, the overhead for calling > PrintersManagerFactory::GetForBrowserContext as needed isn't significant. Done. https://codereview.chromium.org/2790603003/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_discoverer.cc:50: GetAvailablePrinters())); On 2017/04/11 16:27:42, stevenjb wrote: > On 2017/04/10 22:16:41, Carlson wrote: > > On 2017/04/10 19:49:55, stevenjb wrote: > > > This is awkward. I would suggest instead providing a GetAllPrinters() method > > > that can be called explicitly instead. > > > > > > e.g. > > > > > > SomeClass::Init() { > > > printer_discoverer_->AddObserver(this); > > > UpdatePrinters(printer_discoverer_->GetAllPrinters()); > > > } > > > > > > SomeClass::OnPrintersFound(printers) { > > > UpdatePrinters(printers); > > > } > > > > I don't think I understand what you're suggesting here. Are you suggesting > that > > we shift responsibility for doing the initial query to the observer, and > expose > > a PrinterDiscoverer method for the observer to make that query? > > > > If so, I'd really argue against that, because I'd rather have a simpler > external > > interface with an uglier implementation than complicate the interface to buy a > > little bit simpler implementation. > > So, that is what I was suggesting, it's a pretty common pattern, but I realize > now that I was more confused than I realized. > > I see now that OnDiscoveryInitialScanDone() does not return the list, it calls > OnPrintersFound() with the list and then it calls OnDiscoveryInitialScanDone() > separately. > > I find personally this API confusing, especially since GetAvailablePrinters() is > synchronous, but if the other reviewers are OK with it I won't push back against > it. I agree the API seems weird given the current implementation, but I think that's mostly because it's also intended to handle the (as of yet unimplemented) network discovery of printers in this class. *That* discovery will be asynchronous, which is why the InitialScanDone hook exists. Since right now all we have is synchronous printers, we can just call both API functions at once. When we add networked printers, the observer will typically get OnPrintersFound(usb printers) posted immediately when observation starts OnPrintersFound(network printers) sometime later (possibly repeatedly, depending on how we get data back) OnDiscoveryInitialScanDone() - when we believe we've found all the printers OnPrintersFound(other stuff) if printers appear/disappear while still observing. and the implementation will change accordingly -- we'll no longer call OnDiscoveryInitialScanDone() from WrappedOnPrintersFound, it will have to wait until the network stuff is done as well. The corresponding UI piece is supposed to be a live-updating list of printers with a spinner showing we're still looking (which might take several seconds for network discovery), the spinner goes away when we think we've discovered everything that exists at the point the scan started, but the list still can be updated if the state of the world changes (e.g. the user plugs in a new USB printer, or something new shows up on the network). Does that make more sense to you? If there is a better API for this, I'd like to get that right. As for the suggestion of using PostTaskAndReplyWithResult, I think that gets the observer-removed case wrong, doesn't it? If someone goes and does discoverer->AddObserver(some_observer); discoverer->RemoveObserver(some_observer); and AddObserver has gone and posted a task against the observer, there is (AFAICT) nothing that prevents the observer callback from being invoked even though the observer is no longer observing the discoverer (also, the observer may no longer exist!). That's why I'm doing the WrappedOnPrintersFound two-step here. If I can get rid of it and still provide a surprise-free API for observers, I'd love to do that. (We could avoid this trouble by doing the OnPrintersFound() call directly from AddObserver(), but that doesn't seem to be the common idiom for observer-based patterns in Chrome, so I've avoided that).
owner lgtm
The CQ bit was checked by justincarlson@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
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 Link to the patchset: https://codereview.chromium.org/2790603003/#ps100001 (title: "stop latching PrintersManager, add comments")
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": 100001, "attempt_start_ts": 1491940008685730,
"parent_rev": "9fe3bc8107285979782a49dbcf0a37cfe0187696", "commit_rev":
"e5243e7446e4bae1bdeb43f1d4c526de3bf939f5"}
Message was sent while issue was closed.
Description was changed from ========== Make CUPS USB printing play better with the settings page. This change does several things: * Provides a real implementation of PrinterDiscoverer, and re-enables it. * Adds support for observers to CupsPrinterDetector, * Connects PrinterDiscoverer to CupsPrinterDetector, so PrinterDiscoverer knows about USB printers BUGS=616866,706582 BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Make CUPS USB printing play better with the settings page. This change does several things: * Provides a real implementation of PrinterDiscoverer, and re-enables it. * Adds support for observers to CupsPrinterDetector, * Connects PrinterDiscoverer to CupsPrinterDetector, so PrinterDiscoverer knows about USB printers BUGS=616866,706582 BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2790603003 Cr-Commit-Position: refs/heads/master@{#463738} Committed: https://chromium.googlesource.com/chromium/src/+/e5243e7446e4bae1bdeb43f1d4c5... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/e5243e7446e4bae1bdeb43f1d4c5... |
