Fix Flash fullscreen context menu target and position.

Fix Flash fullscreen context menu target and position.

This resolves two problems regarding the right-click context menu for Flash fullscreen widgets.  First, the WebContentsViewDelegate implementations (for Views and Cocoa) were assuming the event target for the context menu click is always the widget provided by WebContentsView.  Flash fullscreen is actually a separate widget.  Second, the renderer was trying to be "too smart" by computing the screen coordinates of the context menu itself.  This is fixed by removing the offset calculation, which allows the coordinates to be cleanly translated by the windowing toolkit, browser-side.

BUG=348965
TEST=Repro steps in bug 348965 result in expected behavior.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=255321

[miu, 2014-03-05 07:12:52]

imcheng: PTAL.

avi: OWNERS approval, please.

[Avi (use Gerrit), 2014-03-05 16:28:00]

I don't know Views much; is imcheng an expert?

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm
(right):

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:56:
ChromeWebContentsViewDelegateMac::GetRenderWidgetHostView() {
You might want to rename this to something that doesn't exactly mirror existing
function names to show that it's different. Maybe
GetActiveRenderWidgetHostView()?

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:65:
web_contents_->GetView()->GetContentNativeView();
web_contents_->GetView()->GetContentNativeView() is equivalent to
web_contents_->GetRenderWidgetHostView()->GetNativeView(), so eliminate this
helper function and instead do:

context_menu_.reset(new RenderViewContextMenuMac(
    render_frame_host, params, GetRenderWidgetHostView()->GetNativeView()));

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc
(right):

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:168:
web_contents_->GetView()->GetNativeView();
Does it matter that in one case you return a RWHV and in the other case a WCV?

[miu, 2014-03-05 20:26:54]

jam: Would appreciate if you could take a look at this change too.

> Avi said:
> I don't know Views much; is imcheng an expert?

I'm semi-expert myself, as I've made a whole lot of changes in the stack to
implement Flash fullscreen-within-tab, and also for the tab audio indicator. 
imcheng@ is a local teammate I wanted to loop-in.

I've added jam@, who recently worked on a major change to the context menus, to
have a reviewer with more background knowledge in this area.

Addressed comments:

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm
(right):

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:56:
ChromeWebContentsViewDelegateMac::GetRenderWidgetHostView() {
On 2014/03/05 16:28:00, Avi wrote:
> You might want to rename this to something that doesn't exactly mirror
existing
> function names to show that it's different. Maybe
> GetActiveRenderWidgetHostView()?

Done (and in ChromeWebContentsViewDelegateViews also).

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:65:
web_contents_->GetView()->GetContentNativeView();
On 2014/03/05 16:28:00, Avi wrote:
> web_contents_->GetView()->GetContentNativeView() is equivalent to
> web_contents_->GetRenderWidgetHostView()->GetNativeView(), so eliminate this
> helper function and instead do:
> 
> context_menu_.reset(new RenderViewContextMenuMac(
>     render_frame_host, params, GetRenderWidgetHostView()->GetNativeView()));

Done.  At first, I didn't want to do this since it assumes knowledge about the
implementation of GetContentNativeView().  However, it does clean up the code a
bit here, and I think the better long-term solution is to revisit the API and
impls of WebContentsView.  Meaning, perhaps WebContentsView should own the
"GetActiveNativeView()" logic?  I'll follow-up with jam@ on this.

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc
(right):

https://codereview.chromium.org/183973027/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/tab_contents/chrome_web_contents_view_delegate_views.cc:168:
web_contents_->GetView()->GetNativeView();
On 2014/03/05 16:28:00, Avi wrote:
> Does it matter that in one case you return a RWHV and in the other case a WCV?

I assume you're referring to the fact that, on Mac, we return
GetContentsView()->GetNativeView() instead of GetView()->GetNativeView().  So,
yes, there is a difference.  I noticed this but then also noticed Mac vs Aura
use a completely different ContextMenu implementation.  Mac runs the menu off of
the active RWHV, but Aura runs it on the top level widget reachable from the
active RWHV.  Basically, I did not want go "fixing" anything that wasn't broken.
 ;-)

[imcheng, 2014-03-05 20:28:04]

I am not an expert. Perhaps you should find someone who is to the reviewer list.
I just have a general comment.

https://codereview.chromium.org/183973027/diff/30001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm
(right):

https://codereview.chromium.org/183973027/diff/30001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:51:
render_frame_host, params, widget_view->GetNativeView()));
Is widget_view guaranteed to be non-null here?

[miu, 2014-03-05 20:34:11]

https://codereview.chromium.org/183973027/diff/30001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm
(right):

https://codereview.chromium.org/183973027/diff/30001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/tab_contents/chrome_web_contents_view_delegate_mac.mm:51:
render_frame_host, params, widget_view->GetNativeView()));
On 2014/03/05 20:28:05, imcheng1 wrote:
> Is widget_view guaranteed to be non-null here?

Yes.  This method is being invoked in response to the renderer sending a context
menu open IPC to the browser process.  The renderer host (and associated
objects) is validated to exist far upstream of this code.

[Avi (use Gerrit), 2014-03-05 20:44:12]

Mac stuff LGTM; I don't know about the Views stuff.

[jam, 2014-03-05 22:32:29]

i defer to sky

[sky, 2014-03-05 23:58:33]

LGTM

[miu, 2014-03-06 00:58:02]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-06 01:00:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/183973027/30001

[commit-bot: I haz the power, 2014-03-06 02:19:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-03-06 02:19:32]

Retried try job too often on win for step(s) compile
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[miu, 2014-03-06 02:22:36]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-06 02:25:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/183973027/30001

[commit-bot: I haz the power, 2014-03-06 03:09:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/183973027/30001

[miu, 2014-03-06 07:28:08]

The CQ bit was unchecked by miu@chromium.org

[miu, 2014-03-06 07:29:01]

The CQ bit was checked by miu@chromium.org

[commit-bot: I haz the power, 2014-03-06 07:29:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/miu@chromium.org/183973027/30001

[commit-bot: I haz the power, 2014-03-06 13:02:11]

Change committed as 255321