Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2463473002:
Present the printer list from preferences for Chrome OS. (Closed)
DescriptionPresent the printer list from preferences for Chrome OS.
Chrome OS keeps the canonical printer listing in preferences rather than
in the printing backend. Add a new codepath on this platform.
CL 1 of 3 to enable printer selection on Chrome OS
BUG=658461
TEST=Verified that printers from preferences show up in printer selection.
Committed: https://crrev.com/f7314c0fda0d093c22ceeba08ad00e88695e9d64
Cr-Commit-Position: refs/heads/master@{#430072}
Patch Set 1 #Patch Set 2 : tidy #
Total comments: 10
Patch Set 3 : address comments #Patch Set 4 : fix display #Patch Set 5 : add bug id #
Total comments: 7
Patch Set 6 : present display name as mac does #
Total comments: 4
Patch Set 7 : rebase #Patch Set 8 : update comments #Patch Set 9 : thread restrictions #
Total comments: 4
Patch Set 10 : done #
Dependent Patchsets: Messages
Total messages: 33 (15 generated)
Description was changed from ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. NOTE: This currently breaks printing for these printers. Two follow up CLs complete the integration. BUG=658461 TEST=Verified that printers from preferences show up in printer selection. ========== to ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. CQ-DEPEND=2457933004 BUG=658461 TEST=Verified that printers from preferences show up in printer selection. ==========
Description was changed from ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. CQ-DEPEND=2457933004 BUG=658461 TEST=Verified that printers from preferences show up in printer selection. ========== to ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. CL 1 of 3 to enable printer selection on Chrome OS BUG=658461 TEST=Verified that printers from preferences show up in printer selection. ==========
skau@chromium.org changed reviewers: + thestig@chromium.org
This is a first in a series of CLs to integrate Chrome OS printers with the print dialog. They are intended to be committed as a group. I am introducing a new group of functions called printer_backend_proxy to switch behavior on Chrome OS. Feedback on this approach is appreciated.
Just a few questions. https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:20: printing::PrintBackend::CreateInstance(nullptr)); nit: you can omit printing:: in namespace printing. https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:22: return ""; Is this intentional or will it be filled in with some other value in a follow-up CL? https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... File printing/backend/print_backend.h (right): https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... printing/backend/print_backend.h:31: std::string printer_id; Is this field going to be ChromeOS only?
I've answered the questions and done a little cleanup. One note, printer_name looks to be the name of the printer in CUPS. For CrOS we'll be using our UUIDs in CUPS so that there's no chance of ambiguity since printer names are mutable. https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:20: printing::PrintBackend::CreateInstance(nullptr)); On 2016/10/28 21:55:51, Lei Zhang wrote: > nit: you can omit printing:: in namespace printing. Done. https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:22: return ""; On 2016/10/28 21:55:51, Lei Zhang wrote: > Is this intentional or will it be filled in with some other value in a follow-up > CL? We don't have default printers in Chrome OS right now. It will be added in the near future. https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... File printing/backend/print_backend.h (right): https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... printing/backend/print_backend.h:31: std::string printer_id; On 2016/10/28 21:55:51, Lei Zhang wrote: > Is this field going to be ChromeOS only? I've added a guard for now. It's probably useful in removing this nice bit of code https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr... but I'm not 100% sure what it does so I don't want to migrate it right now.
I've answered the questions and done a little cleanup. One note, printer_name looks to be the name of the printer in CUPS. For CrOS we'll be using our UUIDs in CUPS so that there's no chance of ambiguity since printer names are mutable. https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:20: printing::PrintBackend::CreateInstance(nullptr)); On 2016/10/28 21:55:51, Lei Zhang wrote: > nit: you can omit printing:: in namespace printing. Done. https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:22: return ""; On 2016/10/28 21:55:51, Lei Zhang wrote: > Is this intentional or will it be filled in with some other value in a follow-up > CL? We don't have default printers in Chrome OS right now. It will be added in the near future. https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... File printing/backend/print_backend.h (right): https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... printing/backend/print_backend.h:31: std::string printer_id; On 2016/10/28 21:55:51, Lei Zhang wrote: > Is this field going to be ChromeOS only? I've added a guard for now. It's probably useful in removing this nice bit of code https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr... but I'm not 100% sure what it does so I don't want to migrate it right now.
https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:22: return ""; On 2016/11/02 22:00:03, skau wrote: > On 2016/10/28 21:55:51, Lei Zhang wrote: > > Is this intentional or will it be filled in with some other value in a > follow-up > > CL? > > We don't have default printers in Chrome OS right now. It will be added in the > near future. If there is a bug # for that, please include it for reference. https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... File printing/backend/print_backend.h (right): https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... printing/backend/print_backend.h:31: std::string printer_id; On 2016/11/02 22:00:03, skau wrote: > On 2016/10/28 21:55:51, Lei Zhang wrote: > > Is this field going to be ChromeOS only? > > I've added a guard for now. It's probably useful in removing this nice bit of > code > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr... > but I'm not 100% sure what it does so I don't want to migrate it right now. Ok, so in patch set 2 you had [printer_id, printer_name, printer_description] and in patch set 4 its now [printer_display_name, printer_name, printer_description]. What was wrong with the way you had it in patch set 2?
Updated patch and responses. https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:22: return ""; On 2016/11/02 22:40:59, Lei Zhang wrote: > On 2016/11/02 22:00:03, skau wrote: > > On 2016/10/28 21:55:51, Lei Zhang wrote: > > > Is this intentional or will it be filled in with some other value in a > > follow-up > > > CL? > > > > We don't have default printers in Chrome OS right now. It will be added in > the > > near future. > > If there is a bug # for that, please include it for reference. Done. https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... File printing/backend/print_backend.h (right): https://codereview.chromium.org/2463473002/diff/20001/printing/backend/print_... printing/backend/print_backend.h:31: std::string printer_id; On 2016/11/02 22:40:59, Lei Zhang wrote: > On 2016/11/02 22:00:03, skau wrote: > > On 2016/10/28 21:55:51, Lei Zhang wrote: > > > Is this field going to be ChromeOS only? > > > > I've added a guard for now. It's probably useful in removing this nice bit of > > code > > > https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/print_preview/pr... > > but I'm not 100% sure what it does so I don't want to migrate it right now. > > Ok, so in patch set 2 you had [printer_id, printer_name, printer_description] > and in patch set 4 its now [printer_display_name, printer_name, > printer_description]. What was wrong with the way you had it in patch set 2? Sorry about the thrashing. display_name is more consistent with how the field is being used in the other platforms. In patch set 2, I thought printer_name was used to represent the display name rather than the name in CUPS.
https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:387: // For Chrome OS |printer.printer_display_name| is the human readable name and So is ChromeOS more in line with Mac then? Maybe don't add a ChromeOS-only "printer_display_name" field to PrinterBasicInfo, and just create the info the Mac way in ToBasicInfo() ? Having 2 different ways to interpret PrinterBasicInfo is already a bit confusing. Let's not expand it to 3 ways. https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right): https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:29: PrinterList printer_list; nit: move this down to line 34, and make line 34 take the blank space in line 35, so the two functions are consistent in their formatting. https://codereview.chromium.org/2463473002/diff/80001/printing/backend/print_... File printing/backend/print_backend.h (right): https://codereview.chromium.org/2463473002/diff/80001/printing/backend/print_... printing/backend/print_backend.h:25: // require further interpretation on Mac. See existing callers for examples. Update comment.
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Comments addressed. PTAL. https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:387: // For Chrome OS |printer.printer_display_name| is the human readable name and On 2016/11/03 07:39:50, Lei Zhang wrote: > So is ChromeOS more in line with Mac then? Maybe don't add a ChromeOS-only > "printer_display_name" field to PrinterBasicInfo, and just create the info the > Mac way in ToBasicInfo() ? > > Having 2 different ways to interpret PrinterBasicInfo is already a bit > confusing. Let's not expand it to 3 ways. Per discussion, I'm going to try to align all the platforms around printer_display_name. But the interest of time, ChromeOS will take the OS X code path as well. https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right): https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:29: PrinterList printer_list; On 2016/11/03 07:39:50, Lei Zhang wrote: > nit: move this down to line 34, and make line 34 take the blank space in line > 35, so the two functions are consistent in their formatting. Done. https://codereview.chromium.org/2463473002/diff/80001/printing/backend/print_... File printing/backend/print_backend.h (right): https://codereview.chromium.org/2463473002/diff/80001/printing/backend/print_... printing/backend/print_backend.h:25: // require further interpretation on Mac. See existing callers for examples. On 2016/11/03 07:39:50, Lei Zhang wrote: > Update comment. Done.
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/2463473002/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2463473002/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:387: // For Chrome OS |printer.printer_display_name| is the human readable name and On 2016/11/03 21:58:27, skau wrote: > On 2016/11/03 07:39:50, Lei Zhang wrote: > > So is ChromeOS more in line with Mac then? Maybe don't add a ChromeOS-only > > "printer_display_name" field to PrinterBasicInfo, and just create the info the > > Mac way in ToBasicInfo() ? > > > > Having 2 different ways to interpret PrinterBasicInfo is already a bit > > confusing. Let's not expand it to 3 ways. > > Per discussion, I'm going to try to align all the platforms around > printer_display_name. But the interest of time, ChromeOS will take the OS X > code path as well. Agreed, I think that's a safer bet.
lgtm with some nits to just keep comments up to date. BTW, if may be helpful to add "rebase" or "sync to ToT" to the patch set descriptions when you do. In the patch set 5 to patch set 6 diff, the BUILD.gn changes are not actually related to this CL at all. It'll help for reviewers to know to expect that. Some people do changes and rebasing as different patch sets altogether. https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:379: // |printer.printer_name| specifies the device name / printer queue name. Add a comment to say Chrome OS emulates the Mac behavior. https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:25: // Store the name used in CUPS, Printer#id in printer_name, the description |printer_name|
The CQ bit was checked by skau@chromium.org to run a CQ dry run
thanks for the review. Please note, I'm going to try to land the series together so this will stay open for a little longer than usual. https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:379: // |printer.printer_name| specifies the device name / printer queue name. On 2016/11/03 22:12:29, Lei Zhang wrote: > Add a comment to say Chrome OS emulates the Mac behavior. Done. https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:25: // Store the name used in CUPS, Printer#id in printer_name, the description On 2016/11/03 22:12:29, Lei Zhang wrote: > |printer_name| Done.
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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thestig@: I've had to revise this CL because PrinterPrefManager shouldn't be called off the UI thread. Please take another look. Sorry for the inconvenience.
lgtm https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.h (right): https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.h:25: // list. Mention this must be called on the UI thread. https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:66: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, Not sure if you need to PostTask(), or if you can just do cb.Run(printer_list) right here, since you are already on the right thread.
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org Link to the patchset: https://codereview.chromium.org/2463473002/#ps180001 (title: "done")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the second review! Landing all CLs now. https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.h (right): https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.h:25: // list. On 2016/11/04 21:46:51, Lei Zhang wrote: > Mention this must be called on the UI thread. Done. https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2463473002/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:66: content::BrowserThread::PostTask(content::BrowserThread::UI, FROM_HERE, On 2016/11/04 21:46:51, Lei Zhang wrote: > Not sure if you need to PostTask(), or if you can just do cb.Run(printer_list) > right here, since you are already on the right thread. No, it's not strictly necessary here.
Thanks for the second review! Landing all CLs now.
Message was sent while issue was closed.
Description was changed from ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. CL 1 of 3 to enable printer selection on Chrome OS BUG=658461 TEST=Verified that printers from preferences show up in printer selection. ========== to ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. CL 1 of 3 to enable printer selection on Chrome OS BUG=658461 TEST=Verified that printers from preferences show up in printer selection. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. CL 1 of 3 to enable printer selection on Chrome OS BUG=658461 TEST=Verified that printers from preferences show up in printer selection. ========== to ========== Present the printer list from preferences for Chrome OS. Chrome OS keeps the canonical printer listing in preferences rather than in the printing backend. Add a new codepath on this platform. CL 1 of 3 to enable printer selection on Chrome OS BUG=658461 TEST=Verified that printers from preferences show up in printer selection. Committed: https://crrev.com/f7314c0fda0d093c22ceeba08ad00e88695e9d64 Cr-Commit-Position: refs/heads/master@{#430072} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/f7314c0fda0d093c22ceeba08ad00e88695e9d64 Cr-Commit-Position: refs/heads/master@{#430072} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
