Print preview: Disable the right context menu items in print preview.

Print preview: Disable the right context menu items in print preview.

BUG=106876, 106915
TEST=see bug

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=113943

[Lei Zhang, 2011-12-09 07:07:49]

http://codereview.chromium.org/8879046/diff/1/chrome/browser/printing/print_p...
File chrome/browser/printing/print_preview_message_handler.cc (right):

http://codereview.chromium.org/8879046/diff/1/chrome/browser/printing/print_p...
chrome/browser/printing/print_preview_message_handler.cc:248: // This helps
prevents the print preview tab from being able to print.
Should not have removed this earlier.

[vandebo (ex-Chrome), 2011-12-09 17:40:19]

http://codereview.chromium.org/8879046/diff/1/chrome/browser/tab_contents/ren...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8879046/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_context_menu.cc:983: if
(observer->IsCommandIdSupported(id))
Could print preview just be a RenderViewContextMenuObserver ?

[Lei Zhang, 2011-12-10 01:37:23]

http://codereview.chromium.org/8879046/diff/1/chrome/browser/tab_contents/ren...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8879046/diff/1/chrome/browser/tab_contents/ren...
chrome/browser/tab_contents/render_view_context_menu.cc:983: if
(observer->IsCommandIdSupported(id))
On 2011/12/09 17:40:19, vandebo wrote:
> Could print preview just be a RenderViewContextMenuObserver ?

Sure, we can do that. See patch set 2.

[vandebo (ex-Chrome), 2011-12-10 02:18:22]

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_context_menu_observer.cc (right):

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_context_menu_observer.cc:33: return true;
I think IdSupported is the one that should return false for non print preview
tabs, otherwise other observers or the base class can't alter these behaviors.

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:602: if
(!print_preview_menu_observer_.get()) {
I don't see the definition for this, am I missing something?

[Lei Zhang, 2011-12-10 02:31:33]

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/printing/pri...
File chrome/browser/printing/print_preview_context_menu_observer.cc (right):

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/printing/pri...
chrome/browser/printing/print_preview_context_menu_observer.cc:33: return true;
On 2011/12/10 02:18:22, vandebo wrote:
> I think IdSupported is the one that should return false for non print preview
> tabs, otherwise other observers or the base class can't alter these behaviors.

Done.

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/tab_contents...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

http://codereview.chromium.org/8879046/diff/10002/chrome/browser/tab_contents...
chrome/browser/tab_contents/render_view_context_menu.cc:602: if
(!print_preview_menu_observer_.get()) {
On 2011/12/10 02:18:22, vandebo wrote:
> I don't see the definition for this, am I missing something?

Forgot to upload the .h file.

[vandebo (ex-Chrome), 2011-12-10 02:33:51]

LGTM

[commit-bot: I haz the power, 2011-12-10 02:40:47]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/8879046/10006

[commit-bot: I haz the power, 2011-12-10 03:45:29]

Try job failure for 8879046-10006 (retry) on win_rel for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-10 03:53:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thestig@chromium.org/8879046/8012

[commit-bot: I haz the power, 2011-12-10 05:50:57]

Change committed as 113943