Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2885363003:
Add policy to use system default printer (Closed)
DescriptionAdd policy to use system default printer
Add a policy/command line flag to use the system default printer as the
default destination in Print Preview instead of the most recently used
destination.
BUG=375238
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2885363003
Cr-Commit-Position: refs/heads/master@{#480908}
Committed: https://chromium.googlesource.com/chromium/src/+/421c21956a3d5da8406d3aacec9e520b3653f904
Patch Set 1 #Patch Set 2 : Add to histograms and JSON #Patch Set 3 : Fix policy and rebase #Patch Set 4 : Rebase again #Patch Set 5 : Fix test #
Total comments: 12
Patch Set 6 : Address comments, add test #
Total comments: 2
Patch Set 7 : Fix dynamic refresh #
Total comments: 8
Patch Set 8 : Address comments #
Total comments: 6
Patch Set 9 : Remove IOS checks #Messages
Total messages: 66 (44 generated)
Description was changed from ========== Add policy to use system default printer Add a policy/command line flag to use the system default printer as the default destination in Print Preview instead of the most recently used destination. BUG=375238 ========== to ========== Add policy to use system default printer Add a policy/command line flag to use the system default printer as the default destination in Print Preview instead of the most recently used destination. BUG=375238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
The CQ bit was checked by rbpotter@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rbpotter@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by rbpotter@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_asan_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...)
The CQ bit was checked by rbpotter@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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...) win_clang on master.tryserver.chromium.win (JOB_FAILED, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by rbpotter@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: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Description was changed from ========== Add policy to use system default printer Add a policy/command line flag to use the system default printer as the default destination in Print Preview instead of the most recently used destination. BUG=375238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add policy to use system default printer Add a policy/command line flag to use the system default printer as the default destination in Print Preview instead of the most recently used destination. BUG=375238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
rbpotter@chromium.org changed reviewers: + thestig@chromium.org
FWIW, I believe OS X and Windows 10 (not sure about 8) have a system preference for this. As a follow up bug if one doesn't exist already, we should see if we can retrieve the system setting and use it when there is no policy in effect.
+potentially interested folks FYI too.
https://codereview.chromium.org/2885363003/diff/80001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:842: // Overrides the use of the most recent destination as the automatically How about: Uses the system default printer as the initially selected destination in print preview, instead of the most recently used destination. https://codereview.chromium.org/2885363003/diff/80001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:1235: const char kUseSystemDefaultPrinterAsDefault[] = Can we put print preview in the name, since it does not affect printing with the system print dialog? Maybe kPrintPreviewUseSystemDefaultPrinter? https://codereview.chromium.org/2885363003/diff/80001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:1780: "os": ["win", "mac", "linux"], skau: With CUPS on ChromeOS, does ChromeOS now have the concept of a default system printer?
Can we force-set "useSystemDefaultPrinter" to true in JS, and test the destination_store.js changes? https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:457: data_source->AddBoolean("useSystemDefaultPrinter", Can we do this in CreatePrintPreviewUISource()? Look in the bottom of that method where we do similar things already.
skau@chromium.org changed reviewers: + skau@chromium.org
https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/prefs/ch... File chrome/browser/prefs/chrome_command_line_pref_store.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/prefs/ch... chrome/browser/prefs/chrome_command_line_pref_store.cc:83: {switches::kUseSystemDefaultPrinter, Can we exclude this from ChromeOS? It'll have the odd behavior of always picking Save As PDF. https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:631: if (this.appState_.recentDestinations) { Can !this.useSystemDefaultAsDefault_ be moved here? it looks like foundDestination only changes on 654 and 656. There are also no side effects of this code block if a destination is not found. https://codereview.chromium.org/2885363003/diff/80001/chrome/test/data/policy... File chrome/test/data/policy/policy_test_cases.json (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/test/data/policy... chrome/test/data/policy/policy_test_cases.json:1780: "os": ["win", "mac", "linux"], On 2017/06/15 18:26:16, Lei Zhang wrote: > skau: With CUPS on ChromeOS, does ChromeOS now have the concept of a default > system printer? No, we don't maintain a system default printer in CrOS atm
The CQ bit was checked by rbpotter@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.
rbpotter@chromium.org changed reviewers: + pastarmovj@chromium.org
https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/prefs/ch... File chrome/browser/prefs/chrome_command_line_pref_store.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/prefs/ch... chrome/browser/prefs/chrome_command_line_pref_store.cc:83: {switches::kUseSystemDefaultPrinter, On 2017/06/15 19:41:31, skau wrote: > Can we exclude this from ChromeOS? It'll have the odd behavior of always > picking Save As PDF. Done. https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/resource... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/resource... chrome/browser/resources/print_preview/data/destination_store.js:631: if (this.appState_.recentDestinations) { On 2017/06/15 19:41:31, skau wrote: > Can !this.useSystemDefaultAsDefault_ be moved here? it looks like > foundDestination only changes on 654 and 656. There are also no side effects of > this code block if a destination is not found. Think we still want to mark the recent destinations recent, so that they populate the "Recent Destinations" list. This happens on line 653. https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui.cc:457: data_source->AddBoolean("useSystemDefaultPrinter", On 2017/06/15 18:29:34, Lei Zhang wrote: > Can we do this in CreatePrintPreviewUISource()? Look in the bottom of that > method where we do similar things already. Done. https://codereview.chromium.org/2885363003/diff/80001/chrome/common/chrome_sw... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/common/chrome_sw... chrome/common/chrome_switches.cc:842: // Overrides the use of the most recent destination as the automatically On 2017/06/15 18:26:16, Lei Zhang wrote: > How about: > > Uses the system default printer as the initially selected destination in print > preview, instead of the most recently used destination. Done. https://codereview.chromium.org/2885363003/diff/80001/chrome/common/pref_name... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2885363003/diff/80001/chrome/common/pref_name... chrome/common/pref_names.cc:1235: const char kUseSystemDefaultPrinterAsDefault[] = On 2017/06/15 18:26:16, Lei Zhang wrote: > Can we put print preview in the name, since it does not affect printing with the > system print dialog? Maybe kPrintPreviewUseSystemDefaultPrinter? Done.
lgtm Thanks!
Policy LGTM. https://codereview.chromium.org/2885363003/diff/100001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2885363003/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:9638: 'dynamic_refresh': False, Just to make sure - is this policy only consulted once on start? IF so than this is correct. If however the print dialog will change its behavior for consecutive prints when the policy is flipped than this must be true it doesn't have to flip the behavior of an already opened print dialog though.
rbpotter@chromium.org changed reviewers: + dpapad@chromium.org
+dpapad for print preview JS changes and test. https://codereview.chromium.org/2885363003/diff/100001/components/policy/reso... File components/policy/resources/policy_templates.json (right): https://codereview.chromium.org/2885363003/diff/100001/components/policy/reso... components/policy/resources/policy_templates.json:9638: 'dynamic_refresh': False, On 2017/06/16 08:23:41, pastarmovj wrote: > Just to make sure - is this policy only consulted once on start? IF so than this > is correct. If however the print dialog will change its behavior for consecutive > prints when the policy is flipped than this must be true it doesn't have to flip > the behavior of an already opened print dialog though. Fixed. It will change for later prints, as it is consulted each time the print preview UI is created. However, it is only consulted at dialog creation, so as you suggested flipping the policy would not affect a dialog that was already open.
https://codereview.chromium.org/2885363003/diff/120001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:619: { key::kPrintPreviewUseSystemDefaultPrinter, So if we are going to put this in a (!defined(OS_CHROMEOS) && !defined(OS_ANDROID)) section, should we do that consistently elsewhere? https://codereview.chromium.org/2885363003/diff/120001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:1169: #if !defined(OS_CHROMEOS) See comment below.
https://codereview.chromium.org/2885363003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination_store.js:175: * @type {boolean} Nit: @private {boolean} https://codereview.chromium.org/2885363003/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_ui_browsertest.js (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_ui_browsertest.js:119: loadTimeData.overrideValues({useSystemDefaultPrinter: true}); Can we not do this inside SystemDefaultPrinterPolicy test instead? Then we can just add 'SystemDefaultPrinterPolicy' to the array above. Also does this test make sense on ChromeOS? It seems that it should only run on non-ChromeOS code.
The CQ bit was checked by rbpotter@chromium.org to run a CQ dry run
Patchset #8 (id:140001) has been deleted
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/2885363003/diff/120001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:619: { key::kPrintPreviewUseSystemDefaultPrinter, On 2017/06/16 21:47:43, Lei Zhang wrote: > So if we are going to put this in a (!defined(OS_CHROMEOS) && > !defined(OS_ANDROID)) section, should we do that consistently elsewhere? Done. https://codereview.chromium.org/2885363003/diff/120001/chrome/browser/resourc... File chrome/browser/resources/print_preview/data/destination_store.js (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/browser/resourc... chrome/browser/resources/print_preview/data/destination_store.js:175: * @type {boolean} On 2017/06/19 17:43:37, dpapad wrote: > Nit: @private {boolean} Done. https://codereview.chromium.org/2885363003/diff/120001/chrome/common/chrome_s... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/common/chrome_s... chrome/common/chrome_switches.cc:1169: #if !defined(OS_CHROMEOS) On 2017/06/16 21:47:43, Lei Zhang wrote: > See comment below. Done. https://codereview.chromium.org/2885363003/diff/120001/chrome/test/data/webui... File chrome/test/data/webui/print_preview/print_preview_ui_browsertest.js (right): https://codereview.chromium.org/2885363003/diff/120001/chrome/test/data/webui... chrome/test/data/webui/print_preview/print_preview_ui_browsertest.js:119: loadTimeData.overrideValues({useSystemDefaultPrinter: true}); On 2017/06/19 17:43:37, dpapad wrote: > Can we not do this inside SystemDefaultPrinterPolicy test instead? Then we can > just add 'SystemDefaultPrinterPolicy' to the array above. > > Also does this test make sense on ChromeOS? It seems that it should only run on > non-ChromeOS code. It doesn't work inside the test as print preview has already been created at that point (in setup()). The policy is only checked at creation so it has to be set before then. Changed it to only run on non-ChromeOS.
+eugenebut to help answer the OS_IOS in chrome/ question. https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:621: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && !defined(OS_IOS) I'm not 100% sure, but I thought there's no iOS code in chrome/ ? +eugenebut to give a definitive answer. https://codereview.chromium.org/2885363003/diff/160001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2885363003/diff/160001/chrome/common/pref_nam... chrome/common/pref_names.h:432: #endif // !OS_CHROMEOS && !OS_ANDROID && !OS_IOS I wouldn't bother with the comment when the #if block is only 1 line long.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:621: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && !defined(OS_IOS) On 2017/06/19 23:41:40, Lei Zhang wrote: > I'm not 100% sure, but I thought there's no iOS code in chrome/ ? > > +eugenebut to give a definitive answer. I confirm that there is no iOS code in chrome/
https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:621: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && !defined(OS_IOS) On 2017/06/20 00:53:57, Eugene But wrote: > On 2017/06/19 23:41:40, Lei Zhang wrote: > > I'm not 100% sure, but I thought there's no iOS code in chrome/ ? > > > > +eugenebut to give a definitive answer. > I confirm that there is no iOS code in chrome/ Thanks! Should we in a separate CL, clear out the OS_IOS usage in chrome/, and maybe add a presubmit check?
> Thanks! Should we in a separate CL, clear out the OS_IOS usage in chrome/, and > maybe add a presubmit check? That sounds like a great idea.
LGTM webui
lgtm with no new OS_IOS bits added.
https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right): https://codereview.chromium.org/2885363003/diff/160001/chrome/browser/policy/... chrome/browser/policy/configuration_policy_handler_list_factory.cc:621: #if !defined(OS_CHROMEOS) && !defined(OS_ANDROID) && !defined(OS_IOS) On 2017/06/20 00:56:25, Lei Zhang wrote: > On 2017/06/20 00:53:57, Eugene But wrote: > > On 2017/06/19 23:41:40, Lei Zhang wrote: > > > I'm not 100% sure, but I thought there's no iOS code in chrome/ ? > > > > > > +eugenebut to give a definitive answer. > > I confirm that there is no iOS code in chrome/ > > Thanks! Should we in a separate CL, clear out the OS_IOS usage in chrome/, and > maybe add a presubmit check? Removed all newly added IOS checks. https://codereview.chromium.org/2885363003/diff/160001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2885363003/diff/160001/chrome/common/pref_nam... chrome/common/pref_names.h:432: #endif // !OS_CHROMEOS && !OS_ANDROID && !OS_IOS On 2017/06/19 23:41:40, Lei Zhang wrote: > I wouldn't bother with the comment when the #if block is only 1 line long. Done.
The CQ bit was checked by rbpotter@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.
The CQ bit was checked by rbpotter@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pastarmovj@chromium.org, skau@chromium.org, thestig@chromium.org, dpapad@chromium.org Link to the patchset: https://codereview.chromium.org/2885363003/#ps180001 (title: "Remove IOS checks")
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": 180001, "attempt_start_ts": 1497987272289470,
"parent_rev": "90583d6422c0c059b8c25845a49c51046b605fe2", "commit_rev":
"421c21956a3d5da8406d3aacec9e520b3653f904"}
Message was sent while issue was closed.
Description was changed from ========== Add policy to use system default printer Add a policy/command line flag to use the system default printer as the default destination in Print Preview instead of the most recently used destination. BUG=375238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== Add policy to use system default printer Add a policy/command line flag to use the system default printer as the default destination in Print Preview instead of the most recently used destination. BUG=375238 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2885363003 Cr-Commit-Position: refs/heads/master@{#480908} Committed: https://chromium.googlesource.com/chromium/src/+/421c21956a3d5da8406d3aacec9e... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:180001) as https://chromium.googlesource.com/chromium/src/+/421c21956a3d5da8406d3aacec9e... |
