Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1153173002:
Include USB printers in printer list as "provisional" devices. (Closed)
|
Created:
5 years, 6 months ago by Reilly Grant (use Gerrit) Modified:
5 years, 6 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionInclude USB printers in printer list as "provisional" devices.
This patch starts including a list of connected printers supported by
printerProvider apps in the normal list of extension printers returned
to the print preview WebUI. These printers are given "provisional" IDs
because the devices are not yet known to the apps themselves.
BUG=468955
Committed: https://crrev.com/6c3c36136b4567f2225550f15e27d2720e1930d8
Cr-Commit-Position: refs/heads/master@{#331886}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Reintegrate USB printer list into normal printer list. #
Total comments: 12
Patch Set 3 : Addressed comments. #
Total comments: 21
Patch Set 4 : No {}. :( #
Total comments: 2
Patch Set 5 : Addressed more comments. #
Messages
Total messages: 36 (11 generated)
reillyg@chromium.org changed reviewers: + vitalybuka@chromium.org
As requested this change splits out USB device enumeration into its own function. Please take a look.
Please include Toni in related changes.
vitalybuka@chromium.org changed reviewers: + tbarzic@chromium.org
https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:755: extension_printer_handler_->StartGetUsbPrinters(base::Bind( Can you avoid adding this function and remove as mach as possible USB related logic from PrintPreview. I guess you can return combined list from extension_printer_handler_->StartGetPrinters https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1698: if (!extension_printer_handler_.get()) { if (extension_printer_handler_) https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.h:384: scoped_ptr<ExtensionPrinterHandler> extension_printer_handler_; Idea was to have common interface for different print system. So here should be PrinterHandler.
https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:755: extension_printer_handler_->StartGetUsbPrinters(base::Bind( On 2015/05/27 16:58:40, Vitaly Buka wrote: > Can you avoid adding this function and remove as mach as possible > USB related logic from PrintPreview. > I guess you can return combined list from > extension_printer_handler_->StartGetPrinters I am asking because every new type of printers cause a lot of small changes in different places of the preview JS. I guess the only USB specific thing you need is a message for printers what where never selected for users. Also call you call that message some how generic? Something like "Printer selection confirmation" So if message is present, preview will ask user for any type of printers. If HandleGetExtensionPrinterCapabilities is called after that, extension_printer_handler_ will knows that user allowed access.
https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:124: extensions::APIPermission::kUsb)) { Maybe check that the extension has required event listener (otherwise, these filters are irrelevant)? https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:260: DevicePermissionsManager* permissions_manager = can this logic be moved somewhere under extensions/browser/api/printer_provider/? Maybe create a new file, supported_usb_printers_enumerator or something?
Please take a look.
can you also respond to comments from the previous patchset :) https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:121: wait_for_usb_enumeration_ = true; break; https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:239: printer_enumeration_complete_ = true; nit: no {} https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:241: if (wait_for_usb_enumeration_) { remove this and invoke callback as callback.Run(printers, printer_enumeration_done_ && !wait_for_usb_enumeration_) https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:278: extensions::APIPermission::kPrinterProvider) || extract to a separate method (you have the same condition in StartGetPrinters) https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:307: .Set("extensionName", extension->name())); set a flag "isProvisional" (so it can be distinguished from regular extension printer) https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.h (right): https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.h:118: bool wait_for_usb_enumeration_ = false; nit: usb_enumeration_complete_ to be consistent
https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:260: DevicePermissionsManager* permissions_manager = On 2015/05/27 18:24:33, tbarzic wrote: > can this logic be moved somewhere under > extensions/browser/api/printer_provider/? Maybe create a new file, > supported_usb_printers_enumerator or something? you can ignore this one :)
https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:238: if (done) { maybe simpler is just to have integer counters: int enumerations_in_progress_ = 0; Increment when start decrement when finish done if zero
https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:124: extensions::APIPermission::kUsb)) { On 2015/05/27 18:24:33, tbarzic wrote: > Maybe check that the extension has required event listener (otherwise, these > filters are irrelevant)? I think that starts to get to be a lot of properties to check. Should we just look for an onGetUsbPrinterInfoRequested handler, or onGetPrinterCapabilitiesRequested and onPrintRequested as well? I think these two permissions are the relevant ones for an app being able to declare a set of supported USB printers. https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:755: extension_printer_handler_->StartGetUsbPrinters(base::Bind( On 2015/05/27 17:14:16, Vitaly Buka wrote: > On 2015/05/27 16:58:40, Vitaly Buka wrote: > > Can you avoid adding this function and remove as mach as possible > > USB related logic from PrintPreview. > > I guess you can return combined list from > > extension_printer_handler_->StartGetPrinters > > I am asking because every new type of printers cause a lot of small changes in > different places of the preview JS. > I guess the only USB specific thing you need is a message for printers what > where never selected for users. > > Also call you call that message some how generic? Something like "Printer > selection confirmation" > So if message is present, preview will ask user for any type of printers. > > If HandleGetExtensionPrinterCapabilities is called after that, > extension_printer_handler_ will knows that user allowed access. > > Done. https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1698: if (!extension_printer_handler_.get()) { On 2015/05/27 16:58:40, Vitaly Buka wrote: > if (extension_printer_handler_) Done. https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/1153173002/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.h:384: scoped_ptr<ExtensionPrinterHandler> extension_printer_handler_; On 2015/05/27 16:58:40, Vitaly Buka wrote: > Idea was to have common interface for different print system. > So here should be PrinterHandler. Done. https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:121: wait_for_usb_enumeration_ = true; On 2015/05/28 00:20:00, tbarzic wrote: > break; Done. https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:238: if (done) { On 2015/05/28 16:34:12, Vitaly Buka wrote: > maybe simpler is just to have integer counters: > > int enumerations_in_progress_ = 0; > Increment when start > decrement when finish > done if zero > Done. https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:241: if (wait_for_usb_enumeration_) { On 2015/05/28 00:19:59, tbarzic wrote: > remove this and invoke callback as > > callback.Run(printers, printer_enumeration_done_ && !wait_for_usb_enumeration_) Done. https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:278: extensions::APIPermission::kPrinterProvider) || On 2015/05/28 00:20:00, tbarzic wrote: > extract to a separate method (you have the same condition in StartGetPrinters) Done. https://codereview.chromium.org/1153173002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:307: .Set("extensionName", extension->name())); On 2015/05/28 00:20:00, tbarzic wrote: > set a flag "isProvisional" (so it can be distinguished from regular extension > printer) Done.
lgtm https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:118: pending_enumeration_count_ = 1; nit: Put this next to call to DispatchGetPrintersRequested https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:245: if (done) { no {} https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:280: if (!manifest_data || !HasUsbPrinterProviderPermissions(extension.get())) { no { }
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbarzic@chromium.org Link to the patchset: https://codereview.chromium.org/1153173002/#ps60001 (title: "No {}. :(")
https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:118: pending_enumeration_count_ = 1; On 2015/05/28 21:57:43, tbarzic wrote: > nit: Put this next to call to DispatchGetPrintersRequested This has to happen before we start the USB enumeration because it could complete synchronously. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:245: if (done) { On 2015/05/28 21:57:43, tbarzic wrote: > no {} Done. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:280: if (!manifest_data || !HasUsbPrinterProviderPermissions(extension.get())) { On 2015/05/28 21:57:43, tbarzic wrote: > no { } Done.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153173002/60001
reillyg@chromium.org changed reviewers: + rockot@chromium.org
The CQ bit was unchecked by reillyg@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Ken, please take a look at //extensions/common/api/printer_provider.
lgtm
The CQ bit was checked by reillyg@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153173002/60001
The CQ bit was unchecked by tbarzic@chromium.org
On 2015/05/28 22:41:43, commit-bot: I haz the power wrote: > CQ is trying da patch. Follow status at > https://chromium-cq-status.appspot.com/patch-status/1153173002/60001 I'm not an owner in chrome/browser/ui/webui/print_preview :)
https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:116: // Assume that there can only be one printer enumeration occuring at once. Do we have code outside that guaranty that this will not be called twice? https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:117: DCHECK(pending_enumeration_count_ == 0); DCHECK_EQ https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:132: DCHECK(service); We don't set DCHECKs before deference It will crash on like 134 anyway https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:244: DCHECK(pending_enumeration_count_ > 0); DCHECK_GT https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:278: permissions_manager->GetForExtension(extension->id()); You can put device_permissions declaration after !manifest_data check https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:313: DCHECK(pending_enumeration_count_ > 0); DCHECK_GT https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:315: callback.Run(*printer_list.Build().get(), pending_enumeration_count_ == 0); do you need get() here? https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc (right): https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler_unittest.cc:460: scoped_ptr<TestExtensionEnvironment> env_; maybe just TestExtensionEnvironment env_; https://codereview.chromium.org/1153173002/diff/60001/extensions/common/api/p... File extensions/common/api/printer_provider/usb_printer_manifest_data.h (right): https://codereview.chromium.org/1153173002/diff/60001/extensions/common/api/p... extensions/common/api/printer_provider/usb_printer_manifest_data.h:34: bool SupportsDevice(scoped_refptr<device::UsbDevice> device) const; const scoped_refptr<device::UsbDevice>&
lgtm with comments
https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/extension_printer_handler.cc (right): https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:116: // Assume that there can only be one printer enumeration occuring at once. On 2015/05/28 22:50:55, Vitaly Buka wrote: > Do we have code outside that guaranty that this will not be called twice? At the PrinterProviderAPI layer we support multiple GetPrinters calls but at the PrinterHandler layer we expect Reset to always be called before running another enumeration. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:117: DCHECK(pending_enumeration_count_ == 0); On 2015/05/28 22:50:55, Vitaly Buka wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:132: DCHECK(service); On 2015/05/28 22:50:54, Vitaly Buka wrote: > We don't set DCHECKs before deference > It will crash on like 134 anyway Done. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:244: DCHECK(pending_enumeration_count_ > 0); On 2015/05/28 22:50:54, Vitaly Buka wrote: > DCHECK_GT Done. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:278: permissions_manager->GetForExtension(extension->id()); On 2015/05/28 22:50:55, Vitaly Buka wrote: > You can put device_permissions declaration after !manifest_data check Done. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:313: DCHECK(pending_enumeration_count_ > 0); On 2015/05/28 22:50:55, Vitaly Buka wrote: > DCHECK_GT Done. https://codereview.chromium.org/1153173002/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/extension_printer_handler.cc:315: callback.Run(*printer_list.Build().get(), pending_enumeration_count_ == 0); On 2015/05/28 22:50:55, Vitaly Buka wrote: > do you need get() here? Yes, to turn a scoped_ptr<base::ListValue> into a const base::ListValue&. The value itself will be freed after the statement is evaluated. https://codereview.chromium.org/1153173002/diff/60001/extensions/common/api/p... File extensions/common/api/printer_provider/usb_printer_manifest_data.h (right): https://codereview.chromium.org/1153173002/diff/60001/extensions/common/api/p... extensions/common/api/printer_provider/usb_printer_manifest_data.h:34: bool SupportsDevice(scoped_refptr<device::UsbDevice> device) const; On 2015/05/28 22:50:55, Vitaly Buka wrote: > const scoped_refptr<device::UsbDevice>& Done.
The CQ bit was checked by reillyg@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org, tbarzic@chromium.org, vitalybuka@chromium.org Link to the patchset: https://codereview.chromium.org/1153173002/#ps80001 (title: "Addressed more comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1153173002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6c3c36136b4567f2225550f15e27d2720e1930d8 Cr-Commit-Position: refs/heads/master@{#331886}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1163743002/ by thakis@chromium.org. The reason for reverting is: Looks like this broke at least one test on the official builders (http://crbug.com/493584). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
