Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2376193006:
Disable Ctrl+Shift+P on Chrome OS (Closed)
DescriptionDisable Ctrl+Shift+P on Chrome OS
Chrome OS uses ENABLE_BASIC_PRINTING but has no system dialog. Disable
the shortcut as invoking printing in this state causes a crash.
BUG=652220
TEST=Verify printing still works. Ctrl+Shift+P is a nop.
Committed: https://crrev.com/03b6108c27abe7ebecec0179c34c05938ae3e905
Cr-Commit-Position: refs/heads/master@{#423047}
Patch Set 1 #
Total comments: 4
Patch Set 2 : better macros #Patch Set 3 : Fix test. #
Total comments: 14
Patch Set 4 : extra comments #Patch Set 5 : \ #Patch Set 6 : proofed #
Total comments: 2
Patch Set 7 : rebase #Patch Set 8 : remove comment #Patch Set 9 : fix for buildflag #
Total comments: 6
Patch Set 10 : declare what you use #
Messages
Total messages: 48 (23 generated)
skau@chromium.org changed reviewers: + thestig@chromium.org
This will disable the system dialog shortcut on Chrome OS. Let me know if there's a nicer way than the ifdefs I'm using.
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...
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Disable Ctrl+Shift+P on Chrome OS Chrome OS uses ENABLE_BASIC_PRINTING but has no system dialog. Disable the shortcut as invoking printing in this state causes a crash. BUG=651476 TEST=Verify printing still works. Ctrl+Shift+P is a nop. ========== to ========== Disable Ctrl+Shift+P on Chrome OS Chrome OS uses ENABLE_BASIC_PRINTING but has no system dialog. Disable the shortcut as invoking printing in this state causes a crash. BUG=652220 TEST=Verify printing still works. Ctrl+Shift+P is a nop. ==========
The CQ bit was checked by thestig@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/2376193006/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2376193006/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_command_controller.cc:1105: command_updater_.UpdateCommandEnabled(IDC_BASIC_PRINT, false); Why not just make CanBasicPrint() return false? https://codereview.chromium.org/2376193006/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2376193006/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1148: #if defined(ENABLE_BASIC_PRINTING) Why not just change this so HandleShowSystemDialog() only exists in the relevant build config?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by skau@chromium.org
Thanks for the review. I've made some simplifications. https://codereview.chromium.org/2376193006/diff/1/chrome/browser/ui/browser_c... File chrome/browser/ui/browser_command_controller.cc (right): https://codereview.chromium.org/2376193006/diff/1/chrome/browser/ui/browser_c... chrome/browser/ui/browser_command_controller.cc:1105: command_updater_.UpdateCommandEnabled(IDC_BASIC_PRINT, false); On 2016/10/03 18:33:03, Lei Zhang wrote: > Why not just make CanBasicPrint() return false? I wasn't sure if it was used elsewhere. I've double checked and it looks appropriate. https://codereview.chromium.org/2376193006/diff/1/chrome/browser/ui/webui/pri... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2376193006/diff/1/chrome/browser/ui/webui/pri... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1148: #if defined(ENABLE_BASIC_PRINTING) On 2016/10/03 18:33:03, Lei Zhang wrote: > Why not just change this so HandleShowSystemDialog() only exists in the relevant > build config? Done.
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
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2016/10/03 21:52:17, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. Not sure why you keep checking the commit box or doing some command line equivalent.
The CQ bit was checked by thestig@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...
looks ok, though I'm not thrilled about it because now "enable basic printing" has platform-specific behavior. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:267: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS While we are here, can you change this to: #endif // defined(ENABLE_BASIC_PRINTING) && !defined(OS_CHROMEOS) so it matches the #if ? https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:911: #endif // OS_CHROMEOS Ditto. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:616: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS omit? https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1167: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS omit? https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1418: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS omit? https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:156: #endif // defined(ENABLE_BASIC_PRINTING) && !defined(OS_CHROMEOS) I would just omit the comment since the #if and #endif are so close together, and there are no nested #ifs. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:63: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS omit
On 2016/10/03 22:03:46, Lei Zhang wrote: > On 2016/10/03 21:52:17, commit-bot: I haz the power wrote: > > No L-G-T-M from a valid reviewer yet. > > CQ run can only be started by full committers or once the patch has > > received an L-G-T-M from a full committer. > > Not sure why you keep checking the commit box or doing some command line > equivalent. I keep clicking it thinking it's the Trybot button in Gerrit.
On 2016/10/03 23:03:58, Lei Zhang wrote: > looks ok, though I'm not thrilled about it because now "enable basic printing" > has platform-specific behavior. Since all the changes here are inside chrome/, one alternative is to add a new feature to chrome/common/features.gni, e.g. "enable_basic_print_dialog = enable_basic_printing && !is_chromeos;" Then add a matching define is chrome/browser/BUILD.gn, gated on the variable, and then use that everywhere in place of "defined(ENABLE_BASIC_PRINTING) && !defined(OS_CHROMEOS)".
I'm not crazy about the condition I'm introducing for basic printing but we would need to introduce a new flag for the system dialog. I'm open to tackling a refactor but I'd like to fix the bug first. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/brows... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:267: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS On 2016/10/03 23:03:58, Lei Zhang wrote: > While we are here, can you change this to: > > #endif // defined(ENABLE_BASIC_PRINTING) && !defined(OS_CHROMEOS) > > so it matches the #if ? Done. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/brows... chrome/browser/ui/browser_commands.cc:911: #endif // OS_CHROMEOS On 2016/10/03 23:03:58, Lei Zhang wrote: > Ditto. Done. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:616: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS On 2016/10/03 23:03:58, Lei Zhang wrote: > omit? Done. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1167: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS On 2016/10/03 23:03:58, Lei Zhang wrote: > omit? This is a little far from the if. I'm going to leave it. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1418: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS On 2016/10/03 23:03:58, Lei Zhang wrote: > omit? Done. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_handler.h (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_handler.h:156: #endif // defined(ENABLE_BASIC_PRINTING) && !defined(OS_CHROMEOS) On 2016/10/03 23:03:58, Lei Zhang wrote: > I would just omit the comment since the #if and #endif are so close together, > and there are no nested #ifs. Done. https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc (right): https://codereview.chromium.org/2376193006/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:63: #endif // ENABLE_BASIC_PRINTING && !OS_CHROMEOS On 2016/10/03 23:03:58, Lei Zhang wrote: > omit Done.
On 2016/10/03 23:31:06, Lei Zhang wrote: > On 2016/10/03 23:03:58, Lei Zhang wrote: > > looks ok, though I'm not thrilled about it because now "enable basic printing" > > has platform-specific behavior. > > Since all the changes here are inside chrome/, one alternative is to add a new > feature to chrome/common/features.gni, e.g. "enable_basic_print_dialog = > enable_basic_printing && !is_chromeos;" > > Then add a matching define is chrome/browser/BUILD.gn, gated on the variable, > and then use that everywhere in place of "defined(ENABLE_BASIC_PRINTING) && > !defined(OS_CHROMEOS)". That's a good idea. Hold off on reviewing for a bit and I'll do that. Thanks!
I've added a new feature, enable_basic_print_dialog. PTAL.
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...
ENABLE_BASIC_PRINT_DIALOG isn't actually defined. https://codereview.chromium.org/2376193006/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2376193006/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1167: #endif // defined(ENABLE_BASIC_PRINT_DIALOG) omit comment?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/10/04 00:22:38, Lei Zhang wrote: > ENABLE_BASIC_PRINT_DIALOG isn't actually defined. > > https://codereview.chromium.org/2376193006/diff/100001/chrome/browser/ui/webu... > File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): > > https://codereview.chromium.org/2376193006/diff/100001/chrome/browser/ui/webu... > chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1167: #endif // > defined(ENABLE_BASIC_PRINT_DIALOG) > omit comment? It's defined in chrome/common/features.gni. The CQ passed. Is there something I'm missing?
https://codereview.chromium.org/2376193006/diff/100001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (right): https://codereview.chromium.org/2376193006/diff/100001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_handler.cc:1167: #endif // defined(ENABLE_BASIC_PRINT_DIALOG) On 2016/10/04 00:22:38, Lei Zhang wrote: > omit comment? Done.
On 2016/10/04 18:09:31, skau wrote: > On 2016/10/04 00:22:38, Lei Zhang wrote: > > defined(ENABLE_BASIC_PRINT_DIALOG) > > omit comment? > > It's defined in chrome/common/features.gni. The CQ passed. Is there something > I'm missing? The CQ passing just makes me thing we don't have enough test coverage. Have you tried a Linux build to make sure the native dialog functionality still works? features.gni adds an "enable_basic_print_dialog" GN variable. That does not add ENABLE_BASIC_PRINT_DIALOG AFAIK. If it does, show me where. From what I see, features.gni does automatically add a BUILDFLAG, which one is suppose to use as "#if BUILDFLAG(ENABLE_BASIC_PRINT_DIALOG)".
I fixed it to use BUILDFLAG and fixed the test to catch the previous problem. PTAL.
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...
https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2431: just a blank line change? https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:255: #if BUILDFLAG(ENABLE_BASIC_PRINT_DIALOG) include chrome/common/features.h https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc (right): https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:59: #if defined(ENABLE_BASIC_PRINTING) && !defined(OS_CHROMEOS) Can you add a comment to explain why this is not ENABLE_BASIC_PRINT_DIALOG on purpose?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
PTAL https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/BUILD.gn File chrome/browser/BUILD.gn (right): https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/BUILD.g... chrome/browser/BUILD.gn:2431: On 2016/10/04 22:10:54, Lei Zhang wrote: > just a blank line change? Removed. https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/brow... File chrome/browser/ui/browser_commands.cc (right): https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/brow... chrome/browser/ui/browser_commands.cc:255: #if BUILDFLAG(ENABLE_BASIC_PRINT_DIALOG) On 2016/10/04 22:10:54, Lei Zhang wrote: > include chrome/common/features.h Done. https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/webu... File chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc (right): https://codereview.chromium.org/2376193006/diff/160001/chrome/browser/ui/webu... chrome/browser/ui/webui/print_preview/print_preview_ui_browsertest.cc:59: #if defined(ENABLE_BASIC_PRINTING) && !defined(OS_CHROMEOS) On 2016/10/04 22:10:54, Lei Zhang wrote: > Can you add a comment to explain why this is not ENABLE_BASIC_PRINT_DIALOG on > purpose? Done.
lgtm
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 ========== Disable Ctrl+Shift+P on Chrome OS Chrome OS uses ENABLE_BASIC_PRINTING but has no system dialog. Disable the shortcut as invoking printing in this state causes a crash. BUG=652220 TEST=Verify printing still works. Ctrl+Shift+P is a nop. ========== to ========== Disable Ctrl+Shift+P on Chrome OS Chrome OS uses ENABLE_BASIC_PRINTING but has no system dialog. Disable the shortcut as invoking printing in this state causes a crash. BUG=652220 TEST=Verify printing still works. Ctrl+Shift+P is a nop. ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Disable Ctrl+Shift+P on Chrome OS Chrome OS uses ENABLE_BASIC_PRINTING but has no system dialog. Disable the shortcut as invoking printing in this state causes a crash. BUG=652220 TEST=Verify printing still works. Ctrl+Shift+P is a nop. ========== to ========== Disable Ctrl+Shift+P on Chrome OS Chrome OS uses ENABLE_BASIC_PRINTING but has no system dialog. Disable the shortcut as invoking printing in this state causes a crash. BUG=652220 TEST=Verify printing still works. Ctrl+Shift+P is a nop. Committed: https://crrev.com/03b6108c27abe7ebecec0179c34c05938ae3e905 Cr-Commit-Position: refs/heads/master@{#423047} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/03b6108c27abe7ebecec0179c34c05938ae3e905 Cr-Commit-Position: refs/heads/master@{#423047} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
