Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2215123002:
Disable Print in the context menu when a modal dialog is shown (Closed)
DescriptionDisable Print in the context menu when a modal dialog is shown
Disable the "Print..." option in the page context menu when chrome::CanPrint
is false. This disables printing when a modal dialog, such as the Media Router
dialog, is being shown, just like the Print option in the hotdog menu.
BUG=590650
Committed: https://crrev.com/10d41a7cb5416afdb434e98c3b411284c0ab0be8
Cr-Commit-Position: refs/heads/master@{#410138}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Address Lei's comments #Patch Set 3 : Address Lei's comment #
Messages
Total messages: 17 (7 generated)
Description was changed from ========== a BUG= ========== to ========== Disable Print in the context menu when a modal dialog is shown Disable the "Print..." option in the page context menu when chrome::CanPrint is false. This disables printing when a modal dialog (such as the Media Router dialog) is being shown. BUG=590650 ==========
Description was changed from ========== Disable Print in the context menu when a modal dialog is shown Disable the "Print..." option in the page context menu when chrome::CanPrint is false. This disables printing when a modal dialog (such as the Media Router dialog) is being shown. BUG=590650 ========== to ========== Disable Print in the context menu when a modal dialog is shown Disable the "Print..." option in the page context menu when chrome::CanPrint is false. This disables printing when a modal dialog, such as the Media Router dialog, is being shown, just like the Print option in the hotdog menu. BUG=590650 ==========
takumif@chromium.org changed reviewers: + thestig@chromium.org
Please take a look, thank you!
https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1430: if (id == IDC_PRINT && (content_restrictions & CONTENT_RESTRICTION_PRINT)) Maybe just leave this check in since it's relatively cheap to do? https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2082: Browser* browser = chrome::FindLastActive(); Seems everyone else here is calling chrome::FindBrowserWithWebContents(source_web_contents_); Does that seem more appropriate? https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2085: (params_.media_type == WebContextMenuData::MediaTypeNone || Check these separately first because it is the cheapest check?
Thank you for your fast review! Please take a look again, thanks! https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left): https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:1430: if (id == IDC_PRINT && (content_restrictions & CONTENT_RESTRICTION_PRINT)) On 2016/08/04 23:54:08, Lei Zhang wrote: > Maybe just leave this check in since it's relatively cheap to do? Done. https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2082: Browser* browser = chrome::FindLastActive(); On 2016/08/04 23:54:09, Lei Zhang wrote: > Seems everyone else here is calling > chrome::FindBrowserWithWebContents(source_web_contents_); > > Does that seem more appropriate? chrome::FindBrowserWithWebContents returns null in some cases (in particular when a modal dialog is open), so just for disabling Print for modal dialogs, that'd work as well. We wouldn't even have to call chrome::CanPrint in that case. However, that feels a bit less straightforward to me, and I thought it might be better to be consistent with the hotdog menu option, which relies on chrome::CanPrint. What do you think? https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2085: (params_.media_type == WebContextMenuData::MediaTypeNone || On 2016/08/04 23:54:09, Lei Zhang wrote: > Check these separately first because it is the cheapest check? Done.
https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2082: Browser* browser = chrome::FindLastActive(); On 2016/08/05 00:40:08, takumif wrote: > On 2016/08/04 23:54:09, Lei Zhang wrote: > > Seems everyone else here is calling > > chrome::FindBrowserWithWebContents(source_web_contents_); > > > > Does that seem more appropriate? > > chrome::FindBrowserWithWebContents returns null in some cases (in particular > when a modal dialog is open), so just for disabling Print for modal dialogs, > that'd work as well. We wouldn't even have to call chrome::CanPrint in that > case. However, that feels a bit less straightforward to me, and I thought it > might be better to be consistent with the hotdog menu option, which relies on > chrome::CanPrint. What do you think? How is checking chrome::FindBrowserWithWebContents() returning a nullptr any different from checking if chrome::FindLastActive() returning a nullptr?
https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2215123002/diff/1/chrome/browser/renderer_con... chrome/browser/renderer_context_menu/render_view_context_menu.cc:2082: Browser* browser = chrome::FindLastActive(); On 2016/08/05 01:48:47, Lei Zhang wrote: > On 2016/08/05 00:40:08, takumif wrote: > > On 2016/08/04 23:54:09, Lei Zhang wrote: > > > Seems everyone else here is calling > > > chrome::FindBrowserWithWebContents(source_web_contents_); > > > > > > Does that seem more appropriate? > > > > chrome::FindBrowserWithWebContents returns null in some cases (in particular > > when a modal dialog is open), so just for disabling Print for modal dialogs, > > that'd work as well. We wouldn't even have to call chrome::CanPrint in that > > case. However, that feels a bit less straightforward to me, and I thought it > > might be better to be consistent with the hotdog menu option, which relies on > > chrome::CanPrint. What do you think? > > How is checking chrome::FindBrowserWithWebContents() returning a nullptr any > different from checking if chrome::FindLastActive() returning a nullptr? It was unexpected to me that chrome::FindBrowserWithWebContents() would return a nullptr when a modal dialog is open, but it might be better to just take note of that in a comment. Changing.
lgtm
The CQ bit was checked by takumif@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 takumif@chromium.org
Message was sent while issue was closed.
Description was changed from ========== Disable Print in the context menu when a modal dialog is shown Disable the "Print..." option in the page context menu when chrome::CanPrint is false. This disables printing when a modal dialog, such as the Media Router dialog, is being shown, just like the Print option in the hotdog menu. BUG=590650 ========== to ========== Disable Print in the context menu when a modal dialog is shown Disable the "Print..." option in the page context menu when chrome::CanPrint is false. This disables printing when a modal dialog, such as the Media Router dialog, is being shown, just like the Print option in the hotdog menu. BUG=590650 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Disable Print in the context menu when a modal dialog is shown Disable the "Print..." option in the page context menu when chrome::CanPrint is false. This disables printing when a modal dialog, such as the Media Router dialog, is being shown, just like the Print option in the hotdog menu. BUG=590650 ========== to ========== Disable Print in the context menu when a modal dialog is shown Disable the "Print..." option in the page context menu when chrome::CanPrint is false. This disables printing when a modal dialog, such as the Media Router dialog, is being shown, just like the Print option in the hotdog menu. BUG=590650 Committed: https://crrev.com/10d41a7cb5416afdb434e98c3b411284c0ab0be8 Cr-Commit-Position: refs/heads/master@{#410138} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/10d41a7cb5416afdb434e98c3b411284c0ab0be8 Cr-Commit-Position: refs/heads/master@{#410138}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2214323003/ by takumif@chromium.org. The reason for reverting is: chrome::FindBrowserWithWebContents(source_web_contents_) returns a nullptr for the PDF viewer, causing the Print option to be disabled for PDFs. We need to replace chrome::FindBrowserWithWebContents(source_web_contents_).. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
