Re-land "Disable Print in the context menu when a modal dialog is shown"

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}

[takumif, 2016-08-05 21:21:05]

Description was changed from

==========
BUG=
==========

to

==========
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
==========

[takumif, 2016-08-05 21:48:56]

takumif@chromium.org changed reviewers:
+ thestig@chromium.org

[takumif, 2016-08-05 21:48:57]

Hi Lei, this is a re-submission of a CL I reverted earlier. Please take a look,
thank you!

https://codereview.chromium.org/2219173002/diff/20001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/2219173002/diff/20001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:2089: Browser*
browser = chrome::FindLastActive();
As I wrote in the reason for the reversion, we can't use
chrome::FindBrowserWithWebContents() because it returns null for the PDF viewer.
On the other hand, chrome::FindLastActive() works in those situations, and it
should work fine, since a window needs to be active to get a context menu.

[Lei Zhang, 2016-08-05 22:48:09]

I understand chrome::FindLastActive() works, but it "smells" bad because nobody
else is.

So I looked into this, and now I'm wondering how deep the rabbit hole goes. I'm
finding a lot of brokenness in RenderViewContextMenu in general for PDFs. I've
CC'd the GuestView folks to see what we should do here.

More examples of brokenness:

- RenderViewContextMenu::AppendExitFullscreenItem() always returns false for
PDFs. So there's no right click -> Exit Full Screen when a PDF is full screen.
- The Cast... option is not available for PDFs in the right click context menu,
but it's available in the "3 dot" AKA "hamburger" AKA "wrench" menu.

https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_con...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:2090: Browser*
browser = chrome::FindBrowserWithWebContents(source_web_contents_);
Because the PDF plugin is a guest, passing in |embedder_web_contents_| here
works.

[lazyboy, 2016-08-06 00:27:22]

https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_con...
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 wrote:
> Because the PDF plugin is a guest, passing in |embedder_web_contents_| here
> works.

Anything depending on Browser* can/should use GetWebContentsToUse().
I'm guessing fullscreen related logic will work for that too.
Maybe we should add RVContextMenu::GetBrowser() that can use
GetWebContentsToUse() to return the Browser*?

[takumif, 2016-08-08 19:08:11]

https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_con...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/2219173002/diff/1/chrome/browser/renderer_con...
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:
> On 2016/08/05 22:48:08, Lei Zhang wrote:
> > Because the PDF plugin is a guest, passing in |embedder_web_contents_| here
> > works.
> 
> Anything depending on Browser* can/should use GetWebContentsToUse().
> I'm guessing fullscreen related logic will work for that too.
> Maybe we should add RVContextMenu::GetBrowser() that can use
> GetWebContentsToUse() to return the Browser*?

The browser stays the same for the lifetime of the RVContextMenu, right?
Can/should we just have a |browser_| attribute that we initialize with
chrome::FindBrowserWithWebContents(embedder_web_contents_) in the constructor?

[Lei Zhang, 2016-08-09 00:01:29]

On 2016/08/08 19:08:11, takumif wrote:
> The browser stays the same for the lifetime of the RVContextMenu, right?
> Can/should we just have a |browser_| attribute that we initialize with
> chrome::FindBrowserWithWebContents(embedder_web_contents_) in the constructor?

Each window is a different Browser. If a tab gets moved from one window to
another, the Browser* changes. Though it is hard / impossible to do this with a
mouse, I imagine the chrome.tabs.move API may be able to make this change while
the context menu is open.

[takumif, 2016-08-09 01:26:50]

On 2016/08/09 00:01:29, Lei Zhang wrote:
> On 2016/08/08 19:08:11, takumif wrote:
> > The browser stays the same for the lifetime of the RVContextMenu, right?
> > Can/should we just have a |browser_| attribute that we initialize with
> > chrome::FindBrowserWithWebContents(embedder_web_contents_) in the
constructor?
> 
> Each window is a different Browser. If a tab gets moved from one window to
> another, the Browser* changes. Though it is hard / impossible to do this with
a
> mouse, I imagine the chrome.tabs.move API may be able to make this change
while
> the context menu is open.

I see. I've added GetBrowser() that uses |embedder_web_contents_|, which stores
the value of GetWebContentsToUse().

I'll submit another CL to make IsRouteMediaEnabled() call GetBrowser(), which
will
fix the bug with the "Cast..." option (crbug/602177).

[Lei Zhang, 2016-08-11 01:34:06]

This works for me.

lazyboy: WDYT?

[lazyboy, 2016-08-11 01:42:18]

On 2016/08/11 01:34:06, Lei Zhang (Soon to be OOO) wrote:
> This works for me.
> 
> lazyboy: WDYT?
Patchset #3 lgtm.

[Lei Zhang, 2016-08-11 01:46:51]

++lgtm then

[takumif, 2016-08-11 01:47:10]

The CQ bit was checked by takumif@chromium.org

[commit-bot: I haz the power, 2016-08-11 01:47:50]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 04:04:43]

Description was changed from

==========
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
==========

to

==========
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
==========

[commit-bot: I haz the power, 2016-08-11 04:04:44]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-08-11 04:07:49]

Description was changed from

==========
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
==========

to

==========
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}
==========

[commit-bot: I haz the power, 2016-08-11 04:07:50]

Patchset 3 (id:??) landed as
https://crrev.com/60869c312fa69649a1cdbcae7539071cefaa9b10
Cr-Commit-Position: refs/heads/master@{#411258}