Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2618313004:
[CUPS] Implement the enterprise icon for printers in Print Preview Dialog. (Closed)
Description[CUPS] Implement the enterprise icon for printers in Print Preview Dialog.
BUG=669293
TEST=Define the enterpise printers policy in DMServer and open Print Preview
dialog (Ctrl+P), test that the enterprise printer shows up with the correct icon.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2618313004
Cr-Commit-Position: refs/heads/master@{#444906}
Committed: https://chromium.googlesource.com/chromium/src/+/b4ceb8d03a240ef6399b09711eea8518c838d141
Patch Set 1 #
Total comments: 1
Patch Set 2 : Add the missing image. #
Total comments: 3
Patch Set 3 : Address skau@'s comments. #
Total comments: 2
Patch Set 4 : Update the tests. #Patch Set 5 : Address dbeam@'s comment. Rebase #
Total comments: 9
Patch Set 6 : Address skau@'s comment. #
Total comments: 2
Patch Set 7 : Address skau@'s comment. #
Total comments: 2
Patch Set 8 : Address dpapad@'s comments. #Messages
Total messages: 55 (30 generated)
Description was changed from ========== [CUPS] Implement the enterprise icon for printers in Print Preview Dialog. BUG=669293 TEST=Define the enterpise printers policy in DMServer and open Print Preview dialog (Ctrl+P), test that the enterprise printer shows up with the correct icon. ========== to ========== [CUPS] Implement the enterprise icon for printers in Print Preview Dialog. BUG=669293 TEST=Define the enterpise printers policy in DMServer and open Print Preview dialog (Ctrl+P), test that the enterprise printer shows up with the correct icon. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
xdai@chromium.org changed reviewers: + skau@chromium.org
skau@, could you help take a look at this CL to see if this approach looks good to you? I'll request the owner's review after your review. https://codereview.chromium.org/2618313004/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2618313004/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:111: prefs_->IsEnterprisePrinter(printer.id()) ? kValueTrue : kValueFalse; The reason I put the functions in the anonymous namespace inside of the class is line 110-111. I need the reference of chromeos::PrinterPrefManager and if these functions remain in the anonymous namespace, I'll have to pass the pointer as an argument in most of these functions.
The CQ bit was checked by xdai@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...
https://codereview.chromium.org/2618313004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2618313004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:144: return std::find(recommended_printer_ids_.begin(), Use base::ContainsValue(), it's more straightforward. https://codereview.chromium.org/2618313004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.h (right): https://codereview.chromium.org/2618313004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:44: bool IsEnterprisePrinter(const std::string& printer_id) const; I would make this a property of printer rather than lookup it up by id. It'll make it much simpler to work with and we know if it's an enterprise printer when we call GetPrinter.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
The CQ bit was checked by xdai@chromium.org to run a CQ dry run
skau@, I've addressed your comments. Please take another look, thanks for the review. https://codereview.chromium.org/2618313004/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.h (right): https://codereview.chromium.org/2618313004/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.h:44: bool IsEnterprisePrinter(const std::string& printer_id) const; On 2017/01/10 19:55:52, skau wrote: > I would make this a property of printer rather than lookup it up by id. It'll > make it much simpler to work with and we know if it's an enterprise printer when > we call GetPrinter. Done. Thanks for the suggestion!
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2618313004/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2618313004/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:145: printer->set_source(Printer::SRC_POLICY); Can you update the unittest to check for this?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
xdai@chromium.org changed reviewers: + dpapad@chromium.org
dpapad@, could you help do owner review for this CL please? Thanks! skau@, I've addressed your comments. Thanks for your review. https://codereview.chromium.org/2618313004/diff/100001/chromeos/printing/prin... File chromeos/printing/printer_translator.cc (right): https://codereview.chromium.org/2618313004/diff/100001/chromeos/printing/prin... chromeos/printing/printer_translator.cc:145: printer->set_source(Printer::SRC_POLICY); On 2017/01/10 22:10:38, skau wrote: > Can you update the unittest to check for this? Done.
The CQ bit was checked by xdai@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: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
+dbeam for enterprise_printer.png. This icon is identical to the enterprise-controlled icon used in MD Settings. Do we really need to have multiple copies of this in the codebase?
On 2017/01/11 22:09:28, dpapad wrote: > +dbeam for enterprise_printer.png. This icon is identical to the > enterprise-controlled icon used in MD Settings. Do we really need to have > multiple copies of this in the codebase? wellllllllllll a) we shouldn't use png, we should use svg. skau@: have you tried the UI at 2x DPI, i.e. --force-device-scale-factor=2? b) yes, this icon is already used, but it's in the middle of a big bunch of <g>[1] tags and my SVG fu is low. maybe we could pull it to a separate svg file (like domain.svg) and replace <g id="domain"> with <include src="domain.svg"> inside of icons.html or something? [1] https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/icons.htm...
On 2017/01/11 22:36:00, Dan Beam wrote: > On 2017/01/11 22:09:28, dpapad wrote: > > +dbeam for enterprise_printer.png. This icon is identical to the > > enterprise-controlled icon used in MD Settings. Do we really need to have > > multiple copies of this in the codebase? > > wellllllllllll > > a) we shouldn't use png, we should use svg. skau@: have you tried the UI at 2x > DPI, i.e. --force-device-scale-factor=2? > > b) yes, this icon is already used, but it's in the middle of a big bunch of > <g>[1] tags and my SVG fu is low. maybe we could pull it to a separate svg file > (like domain.svg) and replace <g id="domain"> with <include src="domain.svg"> > inside of icons.html or something? > > [1] > https://cs.chromium.org/chromium/src/ui/webui/resources/cr_elements/icons.htm... I just tried the UI at different DPI and uploaded the screenshots in the bug (crbug.com/669293), they seems look fine. We probably can pull the svg file to a separate file and use svg here instead but since there are many other similar destination icons in png format in the Print Preview UI [1], should we update all of them? [1] https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/d...
dbeam@ I had not checked the UI at different scale factors. But it looks like it should be okay since the image doesn't have any curves in it.
dbeam@, dpapad@, kingly ping?
On 2017/01/13 23:09:33, xdai1 wrote: > dbeam@, dpapad@, kingly ping? https://bugs.chromium.org/p/chromium/issues/detail?id=669293#c4
On 2017/01/14 00:22:41, Dan Beam wrote: > On 2017/01/13 23:09:33, xdai1 wrote: > > dbeam@, dpapad@, kingly ping? > > https://bugs.chromium.org/p/chromium/issues/detail?id=669293#c4 dbeam@, I changed the icon to be the svg icon. Please take another look, thanks for your review!
The CQ bit was checked by xdai@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.
https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/browser... chrome/browser/browser_resources.grd:407: file="../../ui/webui/resources/images/business.svg" type="BINDATA" /> I don't see this file being added in this CL. Did you forget to git add? Also, should we rename to enterprise.svg? https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:581: return arrayContains(this.tags_, Destination.EnterprisePrinterTag_); Is this linear search going to be costly? What is the size of |tags_| and how often is this function called?
dpapad@, I've addressed your comments. Please take another look, thanks for your review. https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/browser... File chrome/browser/browser_resources.grd (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/browser... chrome/browser/browser_resources.grd:407: file="../../ui/webui/resources/images/business.svg" type="BINDATA" /> On 2017/01/18 02:56:04, dpapad wrote: > I don't see this file being added in this CL. Did you forget to git add? Also, > should we rename to enterprise.svg? This is an existing file in the code base so I just use it directly. https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:581: return arrayContains(this.tags_, Destination.EnterprisePrinterTag_); On 2017/01/18 02:56:04, dpapad wrote: > Is this linear search going to be costly? What is the size of |tags_| and how > often is this function called? For a single printer, the size of |tags_| equals to the number of the cups printer's options (https://cs.chromium.org/chromium/src/build/linux/debian_wheezy_amd64-sysroot/...). But I don't know how many options a cups printer can have (maybe skau@ knows?) . This function is called when we initialize the printers list and when the printer state changes.
https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:581: return arrayContains(this.tags_, Destination.EnterprisePrinterTag_); On 2017/01/18 18:45:42, xdai1 wrote: > On 2017/01/18 02:56:04, dpapad wrote: > > Is this linear search going to be costly? What is the size of |tags_| and how > > often is this function called? > > For a single printer, the size of |tags_| equals to the number of the cups > printer's options > (https://cs.chromium.org/chromium/src/build/linux/debian_wheezy_amd64-sysroot/...). > But I don't know how many options a cups printer can have (maybe skau@ knows?) . > This function is called when we initialize the printers list and when the > printer state changes. FYI: We expect around 20 printer properties for any given printer. It could be more but won't exceed 100.
https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:581: return arrayContains(this.tags_, Destination.EnterprisePrinterTag_); On 2017/01/18 at 20:59:06, skau wrote: > On 2017/01/18 18:45:42, xdai1 wrote: > > On 2017/01/18 02:56:04, dpapad wrote: > > > Is this linear search going to be costly? What is the size of |tags_| and how > > > often is this function called? > > > > For a single printer, the size of |tags_| equals to the number of the cups > > printer's options > > (https://cs.chromium.org/chromium/src/build/linux/debian_wheezy_amd64-sysroot/...). > > But I don't know how many options a cups printer can have (maybe skau@ knows?) . > > This function is called when we initialize the printers list and when the > > printer state changes. > FYI: We expect around 20 printer properties for any given printer. It could be more but won't exceed 100. So if I have hundreds of printers (like in a Google office), is this going to be a problem? In other words, is there a noticeable slow down in startup (when the printer list gets initialized)? Can/should |this.tags_| be converted to a native Set instead, see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:581: return arrayContains(this.tags_, Destination.EnterprisePrinterTag_); On 2017/01/18 21:09:57, dpapad wrote: > On 2017/01/18 at 20:59:06, skau wrote: > > On 2017/01/18 18:45:42, xdai1 wrote: > > > On 2017/01/18 02:56:04, dpapad wrote: > > > > Is this linear search going to be costly? What is the size of |tags_| and > how > > > > often is this function called? > > > > > > For a single printer, the size of |tags_| equals to the number of the cups > > > printer's options > > > > (https://cs.chromium.org/chromium/src/build/linux/debian_wheezy_amd64-sysroot/...). > > > But I don't know how many options a cups printer can have (maybe skau@ > knows?) . > > > This function is called when we initialize the printers list and when the > > > printer state changes. > > FYI: We expect around 20 printer properties for any given printer. It could > be more but won't exceed 100. > > So if I have hundreds of printers (like in a Google office), is this going to be > a problem? In other words, is there a noticeable slow down in startup (when the > printer list gets initialized)? Can/should |this.tags_| be converted to a native > Set instead, see > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... xdai@ We should probably make isEnterprise a separate field on the destination. Looks like you could extract the value from the options map in print_preview_handler when converting from a PrintBasicInfo to the dictionary.
dpapad@, skau@, I've addressed your comments. Please take another look, thanks for your review! https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:581: return arrayContains(this.tags_, Destination.EnterprisePrinterTag_); On 2017/01/18 21:09:57, dpapad wrote: > On 2017/01/18 at 20:59:06, skau wrote: > > On 2017/01/18 18:45:42, xdai1 wrote: > > > On 2017/01/18 02:56:04, dpapad wrote: > > > > Is this linear search going to be costly? What is the size of |tags_| and > how > > > > often is this function called? > > > > > > For a single printer, the size of |tags_| equals to the number of the cups > > > printer's options > > > > (https://cs.chromium.org/chromium/src/build/linux/debian_wheezy_amd64-sysroot/...). > > > But I don't know how many options a cups printer can have (maybe skau@ > knows?) . > > > This function is called when we initialize the printers list and when the > > > printer state changes. > > FYI: We expect around 20 printer properties for any given printer. It could > be more but won't exceed 100. > > So if I have hundreds of printers (like in a Google office), is this going to be > a problem? In other words, is there a noticeable slow down in startup (when the > printer list gets initialized)? Can/should |this.tags_| be converted to a native > Set instead, see > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... the format of|this.tags_| (https://cs.chromium.org/chromium/src/chrome/browser/resources/print_preview/d...) is defined to be consistent with cloud print tags format. So it might be better to leave the format as is for now... https://codereview.chromium.org/2618313004/diff/140001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:581: return arrayContains(this.tags_, Destination.EnterprisePrinterTag_); On 2017/01/18 22:05:40, skau wrote: > On 2017/01/18 21:09:57, dpapad wrote: > > On 2017/01/18 at 20:59:06, skau wrote: > > > On 2017/01/18 18:45:42, xdai1 wrote: > > > > On 2017/01/18 02:56:04, dpapad wrote: > > > > > Is this linear search going to be costly? What is the size of |tags_| > and > > how > > > > > often is this function called? > > > > > > > > For a single printer, the size of |tags_| equals to the number of the cups > > > > printer's options > > > > > > > (https://cs.chromium.org/chromium/src/build/linux/debian_wheezy_amd64-sysroot/...). > > > > But I don't know how many options a cups printer can have (maybe skau@ > > knows?) . > > > > This function is called when we initialize the printers list and when the > > > > printer state changes. > > > FYI: We expect around 20 printer properties for any given printer. It could > > be more but won't exceed 100. > > > > So if I have hundreds of printers (like in a Google office), is this going to > be > > a problem? In other words, is there a noticeable slow down in startup (when > the > > printer list gets initialized)? Can/should |this.tags_| be converted to a > native > > Set instead, see > > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... > > xdai@ We should probably make isEnterprise a separate field on the destination. > Looks like you could extract the value from the options map in > print_preview_handler when converting from a PrintBasicInfo to the dictionary. I made isEnterprise_ a separate field on the destination object as you suggested. PrintersToValues() has already extracted the value and saved it in the dictionary with the key printing::kSettingPrinterOptions (printerOptions).
https://codereview.chromium.org/2618313004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:191: arrayContains(opt_params.tags, Destination.EnterprisePrinterTag_)) || Can we add an out_params.isEnterprise instead? We have the same problems as long as we call arrayContains.
skau@, dpapad@, please take another look at this CL, thanks for your review! https://codereview.chromium.org/2618313004/diff/160001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination.js (right): https://codereview.chromium.org/2618313004/diff/160001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination.js:191: arrayContains(opt_params.tags, Destination.EnterprisePrinterTag_)) || On 2017/01/19 03:30:08, skau wrote: > Can we add an out_params.isEnterprise instead? We have the same problems as > long as we call arrayContains. Done.
The CQ bit was checked by xdai@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.
lgtm
LGTM with nit. https://codereview.chromium.org/2618313004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/local_parsers.js (right): https://codereview.chromium.org/2618313004/diff/180001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/local_parsers.js:19: options.isEnterprisePrinter = destinationInfo.cupsEnterprisePrinter; Nit: var options = { description: destinationInfo.printerDescription, isEnterprisePrinter: destinationInfo.cupsEnterprisePrinter, };
Thanks for your review! https://codereview.chromium.org/2618313004/diff/180001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/local_parsers.js (right): https://codereview.chromium.org/2618313004/diff/180001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/local_parsers.js:19: options.isEnterprisePrinter = destinationInfo.cupsEnterprisePrinter; On 2017/01/19 22:44:27, dpapad wrote: > Nit: > > var options = { > description: destinationInfo.printerDescription, > isEnterprisePrinter: destinationInfo.cupsEnterprisePrinter, > }; Done.
The CQ bit was checked by xdai@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skau@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2618313004/#ps190013 (title: "Address dpapad@'s 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": 190013, "attempt_start_ts": 1484867218291950,
"parent_rev": "e05eb07594ff5711be1777818fd849bb8aae9c6b", "commit_rev":
"b4ceb8d03a240ef6399b09711eea8518c838d141"}
Message was sent while issue was closed.
Description was changed from ========== [CUPS] Implement the enterprise icon for printers in Print Preview Dialog. BUG=669293 TEST=Define the enterpise printers policy in DMServer and open Print Preview dialog (Ctrl+P), test that the enterprise printer shows up with the correct icon. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== [CUPS] Implement the enterprise icon for printers in Print Preview Dialog. BUG=669293 TEST=Define the enterpise printers policy in DMServer and open Print Preview dialog (Ctrl+P), test that the enterprise printer shows up with the correct icon. CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2618313004 Cr-Commit-Position: refs/heads/master@{#444906} Committed: https://chromium.googlesource.com/chromium/src/+/b4ceb8d03a240ef6399b09711eea... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:190013) as https://chromium.googlesource.com/chromium/src/+/b4ceb8d03a240ef6399b09711eea... |
