Remove Downcasting RVContextMenuBase in ContextMenuDeleagate implementations.

Remove Downcasting RVContextMenuBase in ContextMenuDeleagate implementations.

ContextMenuDelegate talks in RVCMBase. This is fine for
ContextMenuDelegate::BuildMenu, but while showing it through
ContextMenuDelegate::ShowMenu, each platform downcasts the
RVCMBase to its own variant (RVCMViews, RVCMMac, RVCMImpl),
this seems unnecessary since the showing functionality can be
moved to RVContextMenuBase.
This CL does that and adds a virtual Show() function on
RVCMBase.

BUG=397364
Test=No visible change should occur.
For reference, this CL is related to brining up right-click context menus
in chrome.

Committed: https://crrev.com/e934d06d39858a9ebfbe2fef35aa16c2084f50e5
Cr-Commit-Position: refs/heads/master@{#301564}

[lazyboy, 2014-10-24 19:59:07]

lazyboy@chromium.org changed reviewers:
+ oshima@chromium.org

[lazyboy, 2014-10-24 19:59:07]

+oshima@ for overall overview and athena/ OWNERS.

I'll add other reviewers once you're happy with this.

[oshima, 2014-10-24 20:18:38]

lgtm

https://codereview.chromium.org/678653003/diff/80001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
(right):

https://codereview.chromium.org/678653003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:31:
virtual void Show() override;
no virtual (new c++11 style)

[lazyboy, 2014-10-24 20:30:30]

lazyboy@chromium.org changed reviewers:
+ cpu@chromium.org, sky@chromium.org

[lazyboy, 2014-10-24 20:30:32]

+Scott for reviewing
chrome/browser/*

+Carlos for reviewing
components/renderer_context_menu/*

https://codereview.chromium.org/678653003/diff/80001/chrome/browser/ui/cocoa/...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h
(right):

https://codereview.chromium.org/678653003/diff/80001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:31:
virtual void Show() override;
On 2014/10/24 20:18:37, oshima wrote:
> no virtual (new c++11 style)

Done.

[cpu_(ooo_6.6-7.5), 2014-10-25 00:38:23]

lgtm for components/renderer_context_menu

[sky, 2014-10-25 15:30:04]

https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
File
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
(right):

https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:186:
gfx::Point screen_point(params().x, params().y);
I think you should change ContextMenuParams to provide coordinates in screen
too.

[lazyboy, 2014-10-25 18:46:59]

https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
File
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
(right):

https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:186:
gfx::Point screen_point(params().x, params().y);
On 2014/10/25 15:30:04, sky wrote:
> I think you should change ContextMenuParams to provide coordinates in screen
> too.

Do you mean adding a getter or a member to ContextMenuParams for screenX and
screenY?
Doing that is platform dependent (aura/mac),
aura needs screen_position_client and mac needs NSWindow::convertPoint.
ContextMenuParams is in content/../common, I don't know how to do that in
ContextMenuParamsBuilder.

Perhaps you were meaning sth else?

Other way would be to add a virtual GetScreenCoordinates() = 0
in RenderViewContextMenuBase and have that return the screen coordinates.

[sky, 2014-10-27 15:25:30]

On 2014/10/25 18:46:59, lazyboy wrote:
>
https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
> File
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
> (right):
> 
>
https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:186:
> gfx::Point screen_point(params().x, params().y);
> On 2014/10/25 15:30:04, sky wrote:
> > I think you should change ContextMenuParams to provide coordinates in screen
> > too.
> 
> Do you mean adding a getter or a member to ContextMenuParams for screenX and
> screenY?

Exactly.

> Doing that is platform dependent (aura/mac),
> aura needs screen_position_client and mac needs NSWindow::convertPoint.
> ContextMenuParams is in content/../common, I don't know how to do that in
> ContextMenuParamsBuilder.

Could the view do the mapping once it gets the ContextMenuParams?

> Perhaps you were meaning sth else?
> 
> Other way would be to add a virtual GetScreenCoordinates() = 0
> in RenderViewContextMenuBase and have that return the screen coordinates.

[lazyboy, 2014-10-27 17:50:57]

https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
File
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc
(right):

https://codereview.chromium.org/678653003/diff/120001/chrome/browser/ui/views...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:186:
gfx::Point screen_point(params().x, params().y);
On 2014/10/25 18:46:59, lazyboy wrote:
> On 2014/10/25 15:30:04, sky wrote:
> > I think you should change ContextMenuParams to provide coordinates in screen
> > too.
> 
> Do you mean adding a getter or a member to ContextMenuParams for screenX and
> screenY?
> Doing that is platform dependent (aura/mac),
> aura needs screen_position_client and mac needs NSWindow::convertPoint.
> ContextMenuParams is in content/../common, I don't know how to do that in
> ContextMenuParamsBuilder.
> 
> Perhaps you were meaning sth else?
> 
> Other way would be to add a virtual GetScreenCoordinates() = 0
> in RenderViewContextMenuBase and have that return the screen coordinates.

OK.
I've added
float screen_x, screen_y to ContextMenuParams (mac uses NSPoint so requires
float).
I've added CalculateScreenCoordiatesInParams() to RVCMBase, which gets called
from RenderViewContextMenu::Init().

[sky, 2014-10-27 19:24:53]

https://codereview.chromium.org/678653003/diff/180001/content/public/common/c...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/678653003/diff/180001/content/public/common/c...
content/public/common/context_menu_params.h:66: float screen_x;
These should be set in content before the params are passed to the delegate.

[lazyboy, 2014-10-27 22:01:13]

https://codereview.chromium.org/678653003/diff/180001/content/public/common/c...
File content/public/common/context_menu_params.h (right):

https://codereview.chromium.org/678653003/diff/180001/content/public/common/c...
content/public/common/context_menu_params.h:66: float screen_x;
On 2014/10/27 19:24:53, sky wrote:
> These should be set in content before the params are passed to the delegate.

I just hit a bump while doing this in RenderFrameHostImpl::OnContextMenu()

aura uses screen coordinates to bring up the context menu,
but mac actually doesn't, it rather requires coordinates within window
with y flipped so that 0 is at the bottom edge :(

In this case, calling this screen_x/y makes things confusing.
How about view_x/y?

My current plan is:
Add
virtual gfx::PointF
RenderWidgetHostViewBase::ConvertPointToContextMenuCoordinates() = 0;

and change:
RenderFrameHostImpl::OnContextMenu(ContextMenuParams params) {
  ..
  gfx::PointF view_point =
      render_view_host_->GetView()->ConvertPointToContextMenuCoordinates(
          gfx::Point(params.x, params.y));
  params.view_x = view_point.x();
  params.view_y = view_point.y();

  delegate_->ShowContextMenu(this, params);  
}

WDYT?

[sky, 2014-10-27 23:44:40]

Ugh. Ok, I suggest going back to what you had before I sent you on a wild goose
chase then.

[lazyboy, 2014-10-27 23:58:38]

On 2014/10/27 23:44:40, sky wrote:
> Ugh. Ok, I suggest going back to what you had before I sent you on a wild
goose
> chase then.

Ok, reverted back to patchset #7-ish in patcheset #12

[sky, 2014-10-28 01:58:15]

LGTM

[lazyboy, 2014-10-28 02:31:04]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-10-28 02:32:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678653003/220001

[commit-bot: I haz the power, 2014-10-28 02:39:35]

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

[commit-bot: I haz the power, 2014-10-28 02:39:37]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[lazyboy, 2014-10-28 04:06:16]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-10-28 04:07:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/678653003/260001

[commit-bot: I haz the power, 2014-10-28 04:51:53]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2014-10-28 04:52:40]

Patchset 14 (id:??) landed as
https://crrev.com/e934d06d39858a9ebfbe2fef35aa16c2084f50e5
Cr-Commit-Position: refs/heads/master@{#301564}