Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2457933004:
Register and select printer on click. (Closed)
DescriptionRegister and select printer on click.
Use the printer pref object shown in the print preview dialog to select
the appropriate printer from the backend. If said printer is not
registered with the backend, register it first. Additional integration
with the PPD Provider and launching settings will be required in the
future.
NOTE: This disables printing for printers without PPDs for now.
CL 3 of 3 to enable printer selection on Chrome OS
BUG=637272
TEST=CrOS: Setup printer in preferences. Log out. Log in. Print something.
TEST=Linux: Verify behavior has not changed when printing.
Committed: https://crrev.com/32fefea307ec051458caa9a160c863b2a5255685
Cr-Commit-Position: refs/heads/master@{#430139}
Patch Set 1 #Patch Set 2 : user ppds only #Patch Set 3 : lint #
Total comments: 26
Patch Set 4 : rebase #Patch Set 5 : address comments from thestig@ #Patch Set 6 : revert HandlePrinterSelected #Patch Set 7 : rebase #
Total comments: 22
Patch Set 8 : rebase #Patch Set 9 : address comments #
Total comments: 4
Patch Set 10 : rebase #Patch Set 11 : rebase #Patch Set 12 : address comments #Patch Set 13 : rebase #Messages
Total messages: 31 (20 generated)
Description was changed from ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: Only user provided PPDs are supported at this time. BUG=637272 TEST=Setup printer in preferences. Log out. Log in. Print something. ========== to ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: Only user provided PPDs are supported at this time. CQ-DEPEND=CL:2459943002 CQ-DEPEND=CL:2463473002 BUG=637272 TEST=Setup printer in preferences. Log out. Log in. Print something. ==========
Description was changed from ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: Only user provided PPDs are supported at this time. CQ-DEPEND=CL:2459943002 CQ-DEPEND=CL:2463473002 BUG=637272 TEST=Setup printer in preferences. Log out. Log in. Print something. ========== to ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: Only user provided PPDs are supported at this time. BUG=637272 TEST=Setup printer in preferences. Log out. Log in. Print something. ==========
Description was changed from ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: Only user provided PPDs are supported at this time. BUG=637272 TEST=Setup printer in preferences. Log out. Log in. Print something. ========== to ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. CL 3 of 3 to enable printer selection on Chrome OS BUG=637272 TEST=Setup printer in preferences. Log out. Log in. Print something. ==========
Description was changed from ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. CL 3 of 3 to enable printer selection on Chrome OS BUG=637272 TEST=Setup printer in preferences. Log out. Log in. Print something. ========== to ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: This disables printing for printers without PPDs for now. CL 3 of 3 to enable printer selection on Chrome OS BUG=637272 TEST=CrOS: Setup printer in preferences. Log out. Log in. Print something. TEST=Linux: Verify behavior has not changed when printing. ==========
skau@chromium.org changed reviewers: + thestig@chromium.org
thestig@: Please take a look at the new structure. This CL could change slightly based on the reviews in dependent patches. Notice that Chrome OS has several more steps than the other code paths when selecting a local printer.
https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:102: if (!values) So either line 83 needs this check, or this isn't needed here. The two callers should be consistent. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:549: base::Bind(&PrintPreviewHandler::HandlePrinterSelected, This is weird in that the handler name no longer matches the JS call name. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1031: BrowserThread::PostBlockingPoolTask( Why can't we keep this as base::PostTaskAndReplyWithResult() ? Is it because the callback has to travel a bit further in the ChromeOS workflow? https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:152: // Gets the printer capabilities. First element of |args| is the printer name. Comment is out of date. Does HandleGetPrinterCapabilities() even exist anymore? https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.h (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy.h:29: // to setup the printer if necessary. Returns nullptr if there is an error. Documentation is not quite right. void functions don't return. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:56: base::Bind(cb, base::Passed(std::move(capabilities)))); base/bind_helper.h says you can do base::Passed(&capabilities). https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:66: // TODO(skau): Open printer settings if this is resolvable. I worry when the callback doesn't run. Is that ok or is someone going to continue waiting for it? https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:83: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); Use DCHECK_CURRENTLY_ON(). https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:108: bool IsPrinterSetup(const std::string& printer_id) { Maybe rename this to IsPrinterConfigured? I flip between parsing this as "Is Printer Setup" and "Is Printer Set up". https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_capabilities.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_capabilities.cc:85: // Constructs the JSON representing printer capabilities information suitable Duplicates the comment in the header. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_capabilities.h (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_capabilities.h:13: #include "printing/printing_export.h" Nothing is being exported here? https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_capabilities.h:31: const std::string& device_name, Please explain what this is exactly, as PrinterBasicInfo already has a printer_name field, which sometimes means something else.
Thanks for the review. Comments addressed. PTAL. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/printing/printer_pref_manager.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/printing/printer_pref_manager.cc:102: if (!values) On 2016/11/02 23:09:22, Lei Zhang wrote: > So either line 83 needs this check, or this isn't needed here. The two callers > should be consistent. It's not needed. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:549: base::Bind(&PrintPreviewHandler::HandlePrinterSelected, On 2016/11/02 23:09:22, Lei Zhang wrote: > This is weird in that the handler name no longer matches the JS call name. It is weird. I've changed the name back since the end result is still the retrieval of printer capabilities. Would you find it surprising that a method named 'HandleGetPrinterCapabilities' also sets up printers? https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1031: BrowserThread::PostBlockingPoolTask( On 2016/11/02 23:09:22, Lei Zhang wrote: > Why can't we keep this as base::PostTaskAndReplyWithResult() ? Is it because the > callback has to travel a bit further in the ChromeOS workflow? I would like to keep it as PostTaskAndReplyWithResult however the callback has to be passed to a call to dbus which doesn't have a blocking API (it assumes it has a message loop). I don't think I can do it without the callback but I'm open to suggestions. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:152: // Gets the printer capabilities. First element of |args| is the printer name. On 2016/11/02 23:09:22, Lei Zhang wrote: > Comment is out of date. Does HandleGetPrinterCapabilities() even exist anymore? Upon further consideration, I've reverted this since the result of the call is the same as before. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.h (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy.h:29: // to setup the printer if necessary. Returns nullptr if there is an error. On 2016/11/02 23:09:22, Lei Zhang wrote: > Documentation is not quite right. void functions don't return. Thanks. I forgot to change the comment when I changed to the callback. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:56: base::Bind(cb, base::Passed(std::move(capabilities)))); On 2016/11/02 23:09:22, Lei Zhang wrote: > base/bind_helper.h says you can do base::Passed(&capabilities). Done. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:66: // TODO(skau): Open printer settings if this is resolvable. On 2016/11/02 23:09:22, Lei Zhang wrote: > I worry when the callback doesn't run. Is that ok or is someone going to > continue waiting for it? Not running the callback leaves print preview in the loading state. However, returning an error value also leaves the print preview in an error state. I'll call the callback with nullptr for now. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:83: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2016/11/02 23:09:22, Lei Zhang wrote: > Use DCHECK_CURRENTLY_ON(). Thanks! I hadn't seen that. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:108: bool IsPrinterSetup(const std::string& printer_id) { On 2016/11/02 23:09:22, Lei Zhang wrote: > Maybe rename this to IsPrinterConfigured? I flip between parsing this as "Is > Printer Setup" and "Is Printer Set up". Done. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_capabilities.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_capabilities.cc:85: // Constructs the JSON representing printer capabilities information suitable On 2016/11/02 23:09:22, Lei Zhang wrote: > Duplicates the comment in the header. Done. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/printer_capabilities.h (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_capabilities.h:13: #include "printing/printing_export.h" On 2016/11/02 23:09:22, Lei Zhang wrote: > Nothing is being exported here? Done. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/printer_capabilities.h:31: const std::string& device_name, On 2016/11/02 23:09:22, Lei Zhang wrote: > Please explain what this is exactly, as PrinterBasicInfo already has a > printer_name field, which sometimes means something else. Done.
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:549: base::Bind(&PrintPreviewHandler::HandlePrinterSelected, On 2016/11/03 22:38:08, skau wrote: > On 2016/11/02 23:09:22, Lei Zhang wrote: > > This is weird in that the handler name no longer matches the JS call name. > > It is weird. I've changed the name back since the end result is still the > retrieval of printer capabilities. Would you find it surprising that a method > named 'HandleGetPrinterCapabilities' also sets up printers? Well, from the JS perspective, it wants to get the printer capabilities, so if you do that as promised, the JS code is happy. If there are logging statements as a side effect, the JS code doesn't care. If some UMA stats get generated as a side effect, the JS code doesn't care. If there's some printer configuration that happens... https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:149: // Gets the printer capabilities for the printer |printer_name|. You reverted back to HandleGetPrinterCapabilities() but the comment has been changed to refer to a parameter that does not exist. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:25: base::Bind(cb, base::Passed(std::move(result)))); Also less verbose if std::move(result) is removed in favor of &result. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:66: PostCallbackOnUIThread(cb, nullptr); An alternative to having PostCallbackOnUIThread() is to have a helper function that returns std::unique_ptr<base::DictionaryValue> do most of the work. Then ConfigurePrinterAndFetchCapabilities() just has to call that, and take the result and do a PostTask(). That way, will be less likely to forget to run the callback. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.h (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.h:28: // Verifies printer setup then retrieves printer capabilities for Maybe say "Verifies printer setup if needed ..." since on non-ChromeOS, there's no set up happening. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:61: GetSettingsDictionary(printer->id(), basic_info); If SettingsToUIThread() doesn't have to run on the blocking pool, you may be able to less thread hopping. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:120: // runs in blocking pool Replace comment with a DCHECK. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:141: // printer is not in CUPS. Attempt setup. This is a bit confusing. It sounds like this comment, as is, should go inside the if block, or it should say "Check if printer is in CUPS. Attempt setup if it is not." https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:151: chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->CupsAddPrinter( Just curious, why if the "debug daemon" the one we talk to for printer setup? https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:191: chromeos::PrinterPrefManagerFactory::GetForBrowserContext(profile); Is GetForBrowserContext() thread safe? Usually, one interacts with Profiles on the UI thread only, and we are not on the UI thread here. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_capabilities.cc (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_capabilities.cc:33: scoped_refptr<printing::PrintBackend> print_backend( no printing:: in namespace printing :-P https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_capabilities.cc:84: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); Does this still need to run on the blocking pool?
The CQ bit was checked by skau@chromium.org to run a CQ dry run
Comments addressed. PTAL. https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2457933004/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:549: base::Bind(&PrintPreviewHandler::HandlePrinterSelected, On 2016/11/03 23:41:46, Lei Zhang wrote: > On 2016/11/03 22:38:08, skau wrote: > > On 2016/11/02 23:09:22, Lei Zhang wrote: > > > This is weird in that the handler name no longer matches the JS call name. > > > > It is weird. I've changed the name back since the end result is still the > > retrieval of printer capabilities. Would you find it surprising that a method > > named 'HandleGetPrinterCapabilities' also sets up printers? > > Well, from the JS perspective, it wants to get the printer capabilities, so if > you do that as promised, the JS code is happy. If there are logging statements > as a side effect, the JS code doesn't care. If some UMA stats get generated as a > side effect, the JS code doesn't care. If there's some printer configuration > that happens... Thanks. When the flow to resolve a misconfigured printer is ready, I'll add a new event to the JS. The "is it configured" question should be largely transparent to Chrome and users. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.h:149: // Gets the printer capabilities for the printer |printer_name|. On 2016/11/03 23:41:46, Lei Zhang wrote: > You reverted back to HandleGetPrinterCapabilities() but the comment has been > changed to refer to a parameter that does not exist. Oops https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:25: base::Bind(cb, base::Passed(std::move(result)))); On 2016/11/03 23:41:46, Lei Zhang wrote: > Also less verbose if std::move(result) is removed in favor of &result. Switched to PostTaskAndReply. Code has been removed. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.cc:66: PostCallbackOnUIThread(cb, nullptr); On 2016/11/03 23:41:46, Lei Zhang wrote: > An alternative to having PostCallbackOnUIThread() is to have a helper function > that returns std::unique_ptr<base::DictionaryValue> do most of the work. Then > ConfigurePrinterAndFetchCapabilities() just has to call that, and take the > result and do a PostTask(). That way, will be less likely to forget to run the > callback. Thanks. I've done that and it is simpler now. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy.h (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy.h:28: // Verifies printer setup then retrieves printer capabilities for On 2016/11/03 23:41:46, Lei Zhang wrote: > Maybe say "Verifies printer setup if needed ..." since on non-ChromeOS, there's > no set up happening. Done. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:61: GetSettingsDictionary(printer->id(), basic_info); On 2016/11/03 23:41:46, Lei Zhang wrote: > If SettingsToUIThread() doesn't have to run on the blocking pool, you may be > able to less thread hopping. GetSettingsDictionary has to run on the blocking pool. I'll update the name. I have tweaked it to reply directly to the callback however. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:120: // runs in blocking pool On 2016/11/03 23:41:46, Lei Zhang wrote: > Replace comment with a DCHECK. Code has been removed. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:141: // printer is not in CUPS. Attempt setup. On 2016/11/03 23:41:46, Lei Zhang wrote: > This is a bit confusing. It sounds like this comment, as is, should go inside > the if block, or it should say "Check if printer is in CUPS. Attempt setup if > it is not." Comments have been revised. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:151: chromeos::DBusThreadManager::Get()->GetDebugDaemonClient()->CupsAddPrinter( On 2016/11/03 23:41:46, Lei Zhang wrote: > Just curious, why if the "debug daemon" the one we talk to for printer setup? The short answer: Some of the CUPS APIs suck The long answer: The most consistent way to add a printer to CUPS is to invoke lpadmin. The way that we do that in a secure way is to call a daemon over DBUS and have the daemon invoke the utility in a sandbox. DebugDaemon is the daemon we use for this despite its poor name. It should probably be called something else. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:191: chromeos::PrinterPrefManagerFactory::GetForBrowserContext(profile); On 2016/11/03 23:41:46, Lei Zhang wrote: > Is GetForBrowserContext() thread safe? Usually, one interacts with Profiles on > the UI thread only, and we are not on the UI thread here. It is not. This method now starts on the UI thread. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_capabilities.cc (right): https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_capabilities.cc:33: scoped_refptr<printing::PrintBackend> print_backend( On 2016/11/03 23:41:46, Lei Zhang wrote: > no printing:: in namespace printing :-P Done. https://codereview.chromium.org/2457933004/diff/120001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_capabilities.cc:84: DCHECK(content::BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread()); On 2016/11/03 23:41:46, Lei Zhang wrote: > Does this still need to run on the blocking pool? Yes it does. This calls GetPrinterCapabilitiesOnBlockingPoolThread directly. I would need to add a callback otherwise.
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
lgtm https://codereview.chromium.org/2457933004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2457933004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:30: enum SetupResult { UNKNOWN, SUCCESS, PPD_NOT_FOUND, FAILURE }; UNKNOWN isn't used? https://codereview.chromium.org/2457933004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:58: void PostCallbackError(const PrinterSetupCallback& cb) { Like in the other CL, can you drop this and just run the callback since you are on the right thread already?
Thanks for all the reviews! https://codereview.chromium.org/2457933004/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc (right): https://codereview.chromium.org/2457933004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:30: enum SetupResult { UNKNOWN, SUCCESS, PPD_NOT_FOUND, FAILURE }; On 2016/11/04 21:57:07, Lei Zhang wrote: > UNKNOWN isn't used? Removed. The result is never saved so I don't need it now. https://codereview.chromium.org/2457933004/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/printer_backend_proxy_chromeos.cc:58: void PostCallbackError(const PrinterSetupCallback& cb) { On 2016/11/04 21:57:07, Lei Zhang wrote: > Like in the other CL, can you drop this and just run the callback since you are > on the right thread already? I'm going to continue to post this to make sure that it executes after the calling stack frame goes out of scope so it's easier to reason about when the callback executes.
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/2457933004/#ps220001 (title: "address comments")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 2459943002 Patch 120001). 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
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/2457933004/#ps240001 (title: "rebase")
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 ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: This disables printing for printers without PPDs for now. CL 3 of 3 to enable printer selection on Chrome OS BUG=637272 TEST=CrOS: Setup printer in preferences. Log out. Log in. Print something. TEST=Linux: Verify behavior has not changed when printing. ========== to ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: This disables printing for printers without PPDs for now. CL 3 of 3 to enable printer selection on Chrome OS BUG=637272 TEST=CrOS: Setup printer in preferences. Log out. Log in. Print something. TEST=Linux: Verify behavior has not changed when printing. ==========
Message was sent while issue was closed.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: This disables printing for printers without PPDs for now. CL 3 of 3 to enable printer selection on Chrome OS BUG=637272 TEST=CrOS: Setup printer in preferences. Log out. Log in. Print something. TEST=Linux: Verify behavior has not changed when printing. ========== to ========== Register and select printer on click. Use the printer pref object shown in the print preview dialog to select the appropriate printer from the backend. If said printer is not registered with the backend, register it first. Additional integration with the PPD Provider and launching settings will be required in the future. NOTE: This disables printing for printers without PPDs for now. CL 3 of 3 to enable printer selection on Chrome OS BUG=637272 TEST=CrOS: Setup printer in preferences. Log out. Log in. Print something. TEST=Linux: Verify behavior has not changed when printing. Committed: https://crrev.com/32fefea307ec051458caa9a160c863b2a5255685 Cr-Commit-Position: refs/heads/master@{#430139} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/32fefea307ec051458caa9a160c863b2a5255685 Cr-Commit-Position: refs/heads/master@{#430139} |
