Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(172)

Issue 2219173002: Re-land "Disable Print in the context menu when a modal dialog is shown" (Closed)

Created:
4 years, 4 months ago by takumif
Modified:
4 years, 4 months ago
Reviewers:
Lei Zhang, lazyboy
CC:
chromium-reviews, Fady Samuel
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Reland "Disable Print in the context menu when a modal dialog is shown" Original CL: https://codereview.chromium.org/2215123002/ 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/60869c312fa69649a1cdbcae7539071cefaa9b10 Cr-Commit-Position: refs/heads/master@{#411258}

Patch Set 1 : Original CL #

Total comments: 3

Patch Set 2 : Replace chrome::FindBrowserWithWebContents() wtith chrome::FindLastActive() #

Total comments: 1

Patch Set 3 : Create the GetBrowser() helper method #

Unified diffs Side-by-side diffs Delta from patch set Stats (+19 lines, -4 lines) Patch
M chrome/browser/renderer_context_menu/render_view_context_menu.h View 1 2 3 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/renderer_context_menu/render_view_context_menu.cc View 1 2 4 chunks +15 lines, -4 lines 0 comments Download

Messages

Total messages: 17 (5 generated)
takumif
Hi Lei, this is a re-submission of a CL I reverted earlier. Please take a ...
4 years, 4 months ago (2016-08-05 21:48:57 UTC) #3
Lei Zhang
I understand chrome::FindLastActive() works, but it "smells" bad because nobody else is. So I looked ...
4 years, 4 months ago (2016-08-05 22:48:09 UTC) #4
lazyboy
https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_context_menu/render_view_context_menu.cc File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_context_menu/render_view_context_menu.cc#newcode2090 chrome/browser/renderer_context_menu/render_view_context_menu.cc:2090: Browser* browser = chrome::FindBrowserWithWebContents(source_web_contents_); On 2016/08/05 22:48:08, Lei Zhang ...
4 years, 4 months ago (2016-08-06 00:27:22 UTC) #5
takumif
https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_context_menu/render_view_context_menu.cc File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right): https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_context_menu/render_view_context_menu.cc#newcode2090 chrome/browser/renderer_context_menu/render_view_context_menu.cc:2090: Browser* browser = chrome::FindBrowserWithWebContents(source_web_contents_); On 2016/08/06 00:27:22, lazyboy wrote: ...
4 years, 4 months ago (2016-08-08 19:08:11 UTC) #6
Lei Zhang
On 2016/08/08 19:08:11, takumif wrote: > The browser stays the same for the lifetime of ...
4 years, 4 months ago (2016-08-09 00:01:29 UTC) #7
takumif
On 2016/08/09 00:01:29, Lei Zhang wrote: > On 2016/08/08 19:08:11, takumif wrote: > > The ...
4 years, 4 months ago (2016-08-09 01:26:50 UTC) #8
Lei Zhang
This works for me. lazyboy: WDYT?
4 years, 4 months ago (2016-08-11 01:34:06 UTC) #9
lazyboy
On 2016/08/11 01:34:06, Lei Zhang (Soon to be OOO) wrote: > This works for me. ...
4 years, 4 months ago (2016-08-11 01:42:18 UTC) #10
Lei Zhang
++lgtm then
4 years, 4 months ago (2016-08-11 01:46:51 UTC) #11
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2219173002/40001
4 years, 4 months ago (2016-08-11 01:47:50 UTC) #13
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 4 months ago (2016-08-11 04:04:44 UTC) #15
commit-bot: I haz the power
4 years, 4 months ago (2016-08-11 04:07:50 UTC) #17
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/60869c312fa69649a1cdbcae7539071cefaa9b10
Cr-Commit-Position: refs/heads/master@{#411258}

Powered by Google App Engine
This is Rietveld 408576698