Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2459943002:
Use printer id to populate CUPS instead of printer name. (Closed)
|
Created:
4 years, 1 month ago by skau Modified:
4 years, 1 month ago CC:
chromium-reviews, michaelpg+watch-md-settings_chromium.org, dbeam+watch-settings_chromium.org, stevenjb+watch-md-settings_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse printer id to populate CUPS instead of printer name.
Printer name can be ambiguous. Printer id is not ambiguous. Register
printers using printer id instead of printer name using CUPS and fix
some resulting problems.
CL 2 of 3 to enable printer selection on Chrome OS
BUG=637272
Committed: https://crrev.com/7fb79ad447b89800ca047c2de1b59096231bae4d
Cr-Commit-Position: refs/heads/master@{#430117}
Patch Set 1 #Patch Set 2 : proof #
Total comments: 2
Patch Set 3 : rebase #Patch Set 4 : rebase #Patch Set 5 : minimize diff #Patch Set 6 : rebase #Patch Set 7 : rebase #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 26 (15 generated)
Description was changed from ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. NOTE: Add CQDEPEND BUG=637272 ========== to ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. CQ-DEPEND=CL:2457933004 BUG=637272 ==========
Description was changed from ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. CQ-DEPEND=CL:2457933004 BUG=637272 ========== to ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. This CL is intended to land with CL:2457933004 BUG=637272 ==========
Description was changed from ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. This CL is intended to land with CL:2457933004 BUG=637272 ========== to ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. CL 2 of 3 to enable printer selection on Chrome OS BUG=637272 ==========
skau@chromium.org changed reviewers: + xdai@chromium.org
xdai@: Please review.
Please see my inline comment. https://codereview.chromium.org/2459943002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2459943002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:389: return std::make_pair(printer.printer_display_name, Where does printer_display_name come from? Seems there is no definition of this class member? And also, what is the relationship of the printing::PrinterBasicInfo::printer_name and chromeos::Printer::id? Are they the same? And how about printing::PrinterBasicInfo::printer_display_name and chromeos::Printer::display_name?
Thanks for taking a look. Please note that some of the changes have been picked back into https://codereview.chromium.org/2463473002/ on which you are now cc'd. https://codereview.chromium.org/2459943002/diff/20001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2459943002/diff/20001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:389: return std::make_pair(printer.printer_display_name, On 2016/10/31 18:08:15, xdai1 wrote: > Where does printer_display_name come from? Seems there is no definition of this > class member? > And also, what is the relationship of the > printing::PrinterBasicInfo::printer_name and chromeos::Printer::id? Are they the > same? And how about printing::PrinterBasicInfo::printer_display_name and > chromeos::Printer::display_name? printer_display_name is defined in the dependent patch set. It's just a string. printing::PrinterBasicInfo::printer_display_name and chromeos::Printer::display_name are equivalent. PrinterBasicInfo::printer_name is the name in CUPS so for Chrome OS, that'll be chromeos::Printer::id. I've updated the comment to maybe be a little easier to read.
xdai@ Can you take a look before I add the OWNERS? I've also minimized the diff.
On 2016/11/03 22:19:09, skau wrote: > xdai@ Can you take a look before I add the OWNERS? I've also minimized the > diff. lgtm
skau@chromium.org changed reviewers: + dbeam@chromium.org
dbeam@: Please review cups_printers_handler for OWNERS approval
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...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by skau@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xdai@chromium.org, dbeam@chromium.org Link to the patchset: https://codereview.chromium.org/2459943002/#ps120001 (title: "rebase")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2463473002 Patch 180001). Please resolve the dependency and try again. If you are sure that there is no real dependency, please use one of the options listed in https://goo.gl/9Es4OR to land the CL.
The CQ bit was checked by skau@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. CL 2 of 3 to enable printer selection on Chrome OS BUG=637272 ========== to ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. CL 2 of 3 to enable printer selection on Chrome OS BUG=637272 ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. CL 2 of 3 to enable printer selection on Chrome OS BUG=637272 ========== to ========== Use printer id to populate CUPS instead of printer name. Printer name can be ambiguous. Printer id is not ambiguous. Register printers using printer id instead of printer name using CUPS and fix some resulting problems. CL 2 of 3 to enable printer selection on Chrome OS BUG=637272 Committed: https://crrev.com/7fb79ad447b89800ca047c2de1b59096231bae4d Cr-Commit-Position: refs/heads/master@{#430117} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/7fb79ad447b89800ca047c2de1b59096231bae4d Cr-Commit-Position: refs/heads/master@{#430117} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
