Separate Toolkit specific impl

Separate Toolkit specific impl
 ToolkitDelegateViews will be moved to components/renderer_contenxt_menu in a separate CL so that athena can use it.

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

[lazyboy, 2014-07-31 16:45:17]

One quick question before I start reviewing.

https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
File
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h
(right):

https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h:1:
// Copyright 2014 The Chromium Authors. All rights reserved.
Do you not need to update the cocoa version of this:
render_view_context_menu_mac

[oshima, 2014-07-31 17:03:04]

On 2014/07/31 16:45:17, lazyboy wrote:
> One quick question before I start reviewing.
> 
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
> File
> chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h
> (right):
> 
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h:1:
> // Copyright 2014 The Chromium Authors. All rights reserved.
> Do you not need to update the cocoa version of this:
> render_view_context_menu_mac

Thanks. It didn't hit my search in cs.chromium.org, but that was probably
because I was looking only for C++ files.... mybad.

I'll fix mac and will let you know.

[oshima, 2014-07-31 17:14:14]

On 2014/07/31 17:03:04, oshima wrote:
> On 2014/07/31 16:45:17, lazyboy wrote:
> > One quick question before I start reviewing.
> > 
> >
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
> > File
> >
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h
> > (right):
> > 
> >
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
> >
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h:1:
> > // Copyright 2014 The Chromium Authors. All rights reserved.
> > Do you not need to update the cocoa version of this:
> > render_view_context_menu_mac
> 
> Thanks. It didn't hit my search in http://cs.chromium.org, but that was
probably
> because I was looking only for C++ files.... mybad.
> 
> I'll fix mac and will let you know.

looks like both views/mac adds the same bidi items. is it ok to consolidate
them?

[lazyboy, 2014-07-31 17:43:10]

On 2014/07/31 17:14:14, oshima wrote:
> On 2014/07/31 17:03:04, oshima wrote:
> > On 2014/07/31 16:45:17, lazyboy wrote:
> > > One quick question before I start reviewing.
> > > 
> > >
> >
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
> > > File
> > >
> chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h
> > > (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
> > >
> >
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h:1:
> > > // Copyright 2014 The Chromium Authors. All rights reserved.
> > > Do you not need to update the cocoa version of this:
> > > render_view_context_menu_mac
> > 
> > Thanks. It didn't hit my search in http://cs.chromium.org, but that was
> probably
> > because I was looking only for C++ files.... mybad.
> > 
> > I'll fix mac and will let you know.
> 
> looks like both views/mac adds the same bidi items. is it ok to consolidate
> them?

It's better if we could imo.

[oshima, 2014-07-31 17:45:39]

On 2014/07/31 17:43:10, lazyboy wrote:
> On 2014/07/31 17:14:14, oshima wrote:
> > On 2014/07/31 17:03:04, oshima wrote:
> > > On 2014/07/31 16:45:17, lazyboy wrote:
> > > > One quick question before I start reviewing.
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
> > > > File
> > > >
> >
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h
> > > > (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/417063004/diff/80001/chrome/browser/ui/views/...
> > > >
> > >
> >
>
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.h:1:
> > > > // Copyright 2014 The Chromium Authors. All rights reserved.
> > > > Do you not need to update the cocoa version of this:
> > > > render_view_context_menu_mac
> > > 
> > > Thanks. It didn't hit my search in http://cs.chromium.org, but that was
> > probably
> > > because I was looking only for C++ files.... mybad.
> > > 
> > > I'll fix mac and will let you know.
> > 
> > looks like both views/mac adds the same bidi items. is it ok to consolidate
> > them?
> 
> It's better if we could imo.

Ok, I'll try it in a separate CL.

[oshima, 2014-07-31 18:22:12]

uploaded new pathch. PTAL

[lazyboy, 2014-07-31 18:59:04]

LGTM with nits.

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

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:776: enabled,
Indent.

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.h:57: //
Initialize the toolkit's menu.
nit: Initializes the ...

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.h:62: // Update
the actual menu items controlled by the toolkit.
nit: // Updates the actual ....

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:49:
// OSX implemenation of the ToolkitDelegate. This simply (re)delegate
nit: "This simply ... " starts from next line.
/s/(re)delegate)/(re)delegates

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:50:
// calls to RVContextMenuMac because this do not have to be componentized.
nit: s/this/they

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

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/ui/views...
chrome/browser/ui/views/renderer_context_menu/render_view_context_menu_views.cc:46:
// ToolkitDelegate:
nit:
// RenderViewContextMenu::ToolkitDelegate implementation.

[oshima, 2014-07-31 19:51:15]

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

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:776: enabled,
On 2014/07/31 18:59:04, lazyboy wrote:
> Indent.

Done.

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.h:57: //
Initialize the toolkit's menu.
On 2014/07/31 18:59:04, lazyboy wrote:
> nit: Initializes the ...

Done.

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.h:62: // Update
the actual menu items controlled by the toolkit.
On 2014/07/31 18:59:04, lazyboy wrote:
> nit: // Updates the actual ....

Done.

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/417063004/diff/170001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:50:
// calls to RVContextMenuMac because this do not have to be componentized.
On 2014/07/31 18:59:04, lazyboy wrote:
> nit: s/this/they

Done.

[oshima, 2014-07-31 19:53:36]

sky -> c/b/ui/views 
rsesk -> c/b/ui/cocoa

[Robert Sesek, 2014-07-31 20:59:23]

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

https://codereview.chromium.org/417063004/diff/190001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:45:
void CancelToolkitMenu();
These need comments.

https://codereview.chromium.org/417063004/diff/190001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/417063004/diff/190001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:53:
public:
nit: indent is wrong

[oshima, 2014-07-31 21:13:12]

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

https://codereview.chromium.org/417063004/diff/190001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:45:
void CancelToolkitMenu();
On 2014/07/31 20:59:22, rsesek wrote:
> These need comments.

Done.

https://codereview.chromium.org/417063004/diff/190001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/417063004/diff/190001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:53:
public:
On 2014/07/31 20:59:22, rsesek wrote:
> nit: indent is wrong

Done.

[sky, 2014-07-31 21:23:43]

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

https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:775:
toolkit_delegate_->UpdateMenuItem(command_id,
It seems like you're duplicating some of what MenuModelDelegate was intended
for. How about adding an ItemChanged to MenuModelDelegate and setters to
SimpleMenuModel for what this needs to change? I believe doing that would mean
no ToolkitDelegate.

https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.h:99: //
RenderViewContextMenuProxy implementation.em
nit: remove em

[oshima, 2014-07-31 22:02:16]

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

https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:775:
toolkit_delegate_->UpdateMenuItem(command_id,
On 2014/07/31 21:23:43, sky wrote:
> It seems like you're duplicating some of what MenuModelDelegate was intended
> for. How about adding an ItemChanged to MenuModelDelegate and setters to
> SimpleMenuModel for what this needs to change? I believe doing that would mean
> no ToolkitDelegate.

I think it's quite different (partly due to the way RVConextMenu works).

RenderViewContextMenu has its own way to manage and update menu item, in async
way.
Menu's view status are managed in different ways (observers are controlling
views state).

It may be possible to rearchitecut the renderer context menu to use
MenuModelDelegate,
but that'd be huge task and the outside of the scope of this CL. If I'm
misunderstanding
something and there is a simpler way, let me know.

https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.h (right):

https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.h:99: //
RenderViewContextMenuProxy implementation.em
On 2014/07/31 21:23:43, sky wrote:
> nit: remove em

Done.

[sky, 2014-07-31 22:19:40]

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

https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:775:
toolkit_delegate_->UpdateMenuItem(command_id,
On 2014/07/31 22:02:15, oshima wrote:
> On 2014/07/31 21:23:43, sky wrote:
> > It seems like you're duplicating some of what MenuModelDelegate was intended
> > for. How about adding an ItemChanged to MenuModelDelegate and setters to
> > SimpleMenuModel for what this needs to change? I believe doing that would
mean
> > no ToolkitDelegate.
> 
> I think it's quite different (partly due to the way RVConextMenu works).
> 
> RenderViewContextMenu has its own way to manage and update menu item, in async
> way.
> Menu's view status are managed in different ways (observers are controlling
> views state).
> 
> It may be possible to rearchitecut the renderer context menu to use
> MenuModelDelegate,
> but that'd be huge task and the outside of the scope of this CL. If I'm
> misunderstanding
> something and there is a simpler way, let me know.

I don't think it would be that much. If you make MenuModelAdapter a
MenuModelDegate and have RVCM::UpdateMenuItem call through to the delegate than
it'll work. For the cocoa side you could make RenderViewContextMenuCocoa
implement the MenuModelDelegate method. There may be a more central place there.
Not sure.

[oshima, 2014-08-01 11:34:12]

On 2014/07/31 22:19:40, sky wrote:
>
https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
> File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):
> 
>
https://codereview.chromium.org/417063004/diff/210001/chrome/browser/renderer...
> chrome/browser/renderer_context_menu/render_view_context_menu.cc:775:
> toolkit_delegate_->UpdateMenuItem(command_id,
> On 2014/07/31 22:02:15, oshima wrote:
> > On 2014/07/31 21:23:43, sky wrote:
> > > It seems like you're duplicating some of what MenuModelDelegate was
intended
> > > for. How about adding an ItemChanged to MenuModelDelegate and setters to
> > > SimpleMenuModel for what this needs to change? I believe doing that would
> mean
> > > no ToolkitDelegate.
> > 
> > I think it's quite different (partly due to the way RVConextMenu works).
> > 
> > RenderViewContextMenu has its own way to manage and update menu item, in
async
> > way.
> > Menu's view status are managed in different ways (observers are controlling
> > views state).
> > 
> > It may be possible to rearchitecut the renderer context menu to use
> > MenuModelDelegate,
> > but that'd be huge task and the outside of the scope of this CL. If I'm
> > misunderstanding
> > something and there is a simpler way, let me know.
> 
> I don't think it would be that much. If you make MenuModelAdapter a
> MenuModelDegate and have RVCM::UpdateMenuItem call through to the delegate
than
> it'll work. For the cocoa side you could make RenderViewContextMenuCocoa
> implement the MenuModelDelegate method. There may be a more central place
there.
> Not sure.

SpellingMenuObserver doesn't update the menu model right now so it's not that
simple.

I looked into a bit more I think I need following changes:
1) update or extend SimpleMenuModel so that I can update label by command id,
2) add OnMenuItemChanged to MenuModelDelegate (I don't want to recreate whole
menu)
3) change RVContextMenu/Observer to use MenuModelDelegate
4) RVContextMenuObserver needs IsVisibleAt as it's currently done via
UpdateMenuItem

Still I want to do that in separate CL as it's the outside of the scope of this
CL
and don't want to introduce regression in the same CL.

[sky, 2014-08-04 18:41:06]

Thanks for the investigation, LGTM

[Robert Sesek, 2014-08-04 19:03:38]

LGTM

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

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:1:
y// Copyright 2014 The Chromium Authors. All rights reserved.
Spurious y.

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:54:
ToolkitDelegateMac(RenderViewContextMenuMac* context_menu)
explicit

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:277:
[item setTitle:SysUTF16ToNSString(title)];
base::

[oshima, 2014-08-04 19:41:05]

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

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.h:1:
y// Copyright 2014 The Chromium Authors. All rights reserved.
On 2014/08/04 19:03:37, rsesek wrote:
> Spurious y.

Done.

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
File
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm
(right):

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:54:
ToolkitDelegateMac(RenderViewContextMenuMac* context_menu)
On 2014/08/04 19:03:37, rsesek wrote:
> explicit

Done.

https://codereview.chromium.org/417063004/diff/230001/chrome/browser/ui/cocoa...
chrome/browser/ui/cocoa/renderer_context_menu/render_view_context_menu_mac.mm:277:
[item setTitle:SysUTF16ToNSString(title)];
On 2014/08/04 19:03:37, rsesek wrote:
> base::

Done.

[oshima, 2014-08-04 20:33:53]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-04 20:36:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/417063004/270001

[commit-bot: I haz the power, 2014-08-04 22:30:16]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[oshima, 2014-08-05 03:14:27]

The CQ bit was unchecked by oshima@chromium.org

[oshima, 2014-08-05 03:14:33]

The CQ bit was checked by oshima@chromium.org

[commit-bot: I haz the power, 2014-08-05 03:15:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oshima@chromium.org/417063004/270001

[commit-bot: I haz the power, 2014-08-05 05:09:13]

Change committed as 287463