Separate out logic to handle different category/group of context menu items from RVContextMenu class

Separate out logic to handle different category/group of context menu items from RVContextMenu class.

This is separated into classes ContextMenuContentType*.

This way, we have separate ContextMenuContentType* classes for regular pages (ContextMenuContentType), platform_app (ContextMenuContentTypePlatformApp), extension_popup (ContextMenuContentTypeExtensionPopup), panel (ContextMenuContentTypePanel), app_mode (ContextMenuContentMenuContentTypeAppMode) and <webview> guest (ContetMenuContentTypeWebView).

The motivation is to separate out the logic to decide what category/group to show for <webview> guest from RVContextMenu independently. This will also stop cluttering RVContextMenu with is_guest_ checks.

Webview ContextMenu API would benefit from this. Unrelated, but docs can be found for Webview ContextMenu API can be found here:
https://docs.google.com/a/chromium.org/document/d/1AoMD6kF8ui1dikzTrwK-weVHegSqQaLV2zx4xJSh_fQ/edit
and
https://docs.google.com/a/chromium.org/document/d/11pE2PRGSyLbZGVVP1pspViK_xLWv_A-1dJDY2kjjbgc/edit

BUG=140315
Test=No behavioral change expected, internal refactor.

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

[lazyboy, 2014-02-22 00:17:24]

Hi Avi, Please take a look at RenderViewContextMenu::InitMenu() to see if this
change makes sense. Thanks.

[Avi (use Gerrit), 2014-02-22 01:28:16]

On 2014/02/22 00:17:24, lazyboy wrote:
> Hi Avi, Please take a look at RenderViewContextMenu::InitMenu() to see if this
> change makes sense. Thanks.

Lemme take a look. BTW, chrome/browser/tab_contents/ is a directory full of
random odds and ends. With a change like this, you totally should break the
context menu into its own top-level folder.

[Avi (use Gerrit), 2014-02-22 01:37:44]

I like that you're attacking this for cleanup. I'm not thrilled with this answer
but I can't think of anything better.

:\

LGTM if you move the whole context menu mess into its own subfolder.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/render_view_context_menu.h:207: bool
AppendCustomItems();
blank line above this

[Fady Samuel, 2014-02-24 19:07:28]

Some initial set of comments.

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/menu_category_base.cc (right):

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_base.cc:75: int
MenuCategoryBase::GetCategories() {
GetSupportedCategories or better yet, can we get rid of this all together?
SupportsCategory(..)

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_base.cc:76: int items = 0;
supported_categories

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_base.cc:101: bool
MenuCategoryBase::IsCategoryEnabled(int category) {
IsCategorySupported

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/menu_category_base.h (right):

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_base.h:27: class MenuCategoryBase {
This class represents a context not category. It tells RenderViewContextMenu
what categories are supported. How about renaming this to MenuContextBase?

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_base.h:34: ITEM_CATEGORY_CUSTOM,
Do the bit shifting here perhaps?

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_base.h:54: };
A helper method here: SupportsCategory(...) might be useful.

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/menu_category_factory.cc (right):

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_factory.cc:32: return new
MenuCategoryGuest(render_frame_host, params);
I would love for this to be moved to chrome/browser/guestview/webview.

If (WebViewGuest::FromWebContents(web_contents)) {
  return new WebViewMenuContext(render_frame_host, params);

}

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/menu_category_guest.cc (right):

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/menu_category_guest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
Can we somehow move this file to the webview directory? That way we can change
it when we need to?

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/169093009/diff/240001/chrome/browser/tab_cont...
chrome/browser/tab_contents/render_view_context_menu.cc:580: if (items & (1 <<
MenuCategoryBase::ITEM_CATEGORY_CUSTOM))
I would prefer this syntax:

if (context_->SupportsCategory(MenuContext::ITEM_CATEGORY_CUSTOM))

[lazyboy, 2014-02-24 23:45:00]

Avi: I've moved the files under c/b/tab_contents/menu_context/*. Files are
renamed based on review comments from fsamuel@, can you take a quick look again?

Fady: I've moved the guest specific files under c/b/guestview/webview/*.

Thanks.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
File chrome/browser/tab_contents/menu_category_base.cc (right):

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_base.cc:75: int
MenuCategoryBase::GetCategories() {
On 2014/02/24 19:07:29, Fady Samuel wrote:
> GetSupportedCategories or better yet, can we get rid of this all together?
> SupportsCategory(..)

Removed GetCategories()

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_base.cc:76: int items = 0;
On 2014/02/24 19:07:29, Fady Samuel wrote:
> supported_categories

Done.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_base.cc:101: bool
MenuCategoryBase::IsCategoryEnabled(int category) {
On 2014/02/24 19:07:29, Fady Samuel wrote:
> IsCategorySupported

Done.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
File chrome/browser/tab_contents/menu_category_base.h (right):

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_base.h:27: class MenuCategoryBase {
On 2014/02/24 19:07:29, Fady Samuel wrote:
> This class represents a context not category. It tells RenderViewContextMenu
> what categories are supported. How about renaming this to MenuContextBase?

I've renamed the class to MenuContext, others to MenuContextPanel/Guest...

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_base.h:34: ITEM_CATEGORY_CUSTOM,
On 2014/02/24 19:07:29, Fady Samuel wrote:
> Do the bit shifting here perhaps?

Obsolte now since we're not using masks anymore.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_base.h:54: };
On 2014/02/24 19:07:29, Fady Samuel wrote:
> A helper method here: SupportsCategory(...) might be useful.

Removed bitmask, so this is not required anymore.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
File chrome/browser/tab_contents/menu_category_factory.cc (right):

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_factory.cc:32: return new
MenuCategoryGuest(render_frame_host, params);
On 2014/02/24 19:07:29, Fady Samuel wrote:
> I would love for this to be moved to chrome/browser/guestview/webview.
> 
> If (WebViewGuest::FromWebContents(web_contents)) {
>   return new WebViewMenuContext(render_frame_host, params);
> 
> }

Done.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
File chrome/browser/tab_contents/menu_category_guest.cc (right):

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/menu_category_guest.cc:1: // Copyright 2014 The
Chromium Authors. All rights reserved.
On 2014/02/24 19:07:29, Fady Samuel wrote:
> Can we somehow move this file to the webview directory? That way we can change
> it when we need to?

Done.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/render_view_context_menu.cc:580: if (items & (1 <<
MenuCategoryBase::ITEM_CATEGORY_CUSTOM))
On 2014/02/24 19:07:29, Fady Samuel wrote:
> I would prefer this syntax:
> 
> if (context_->SupportsCategory(MenuContext::ITEM_CATEGORY_CUSTOM))

Done.

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
File chrome/browser/tab_contents/render_view_context_menu.h (right):

https://chromiumcodereview.appspot.com/169093009/diff/240001/chrome/browser/t...
chrome/browser/tab_contents/render_view_context_menu.h:207: bool
AppendCustomItems();
On 2014/02/22 01:37:45, Avi wrote:
> blank line above this

Done.

[Fady Samuel, 2014-02-25 00:33:53]

LGTM with nits.

https://codereview.chromium.org/169093009/diff/410001/chrome/browser/guestvie...
File chrome/browser/guestview/webview/menu_context_guest.cc (right):

https://codereview.chromium.org/169093009/diff/410001/chrome/browser/guestvie...
chrome/browser/guestview/webview/menu_context_guest.cc:7:
MenuContextGuest::MenuContextGuest(content::RenderFrameHost* render_frame_host,
Can we call this a WebViewMenuContext?

https://codereview.chromium.org/169093009/diff/410001/chrome/browser/tab_cont...
File chrome/browser/tab_contents/render_view_context_menu.cc (right):

https://codereview.chromium.org/169093009/diff/410001/chrome/browser/tab_cont...
chrome/browser/tab_contents/render_view_context_menu.cc:610: 
What is this?

[lazyboy, 2014-02-25 01:29:08]

render_view_context_menu.cc change seems like git cl upload issue, should be
fixed in latest upload.

https://codereview.chromium.org/169093009/diff/410001/chrome/browser/guestvie...
File chrome/browser/guestview/webview/menu_context_guest.cc (right):

https://codereview.chromium.org/169093009/diff/410001/chrome/browser/guestvie...
chrome/browser/guestview/webview/menu_context_guest.cc:7:
MenuContextGuest::MenuContextGuest(content::RenderFrameHost* render_frame_host,
On 2014/02/25 00:33:53, Fady Samuel wrote:
> Can we call this a WebViewMenuContext?

Done.

[Avi (use Gerrit), 2014-02-25 03:52:16]

On 2014/02/24 23:45:00, lazyboy wrote:
> Avi: I've moved the files under c/b/tab_contents/menu_context/*.

:/

I'm really confused by the naming. We have a "context menu", which is the menu
you get with a right-click, and a "menu context", which is used when deciding
what to show in a context menu.

First, can you make a chrome/browser/context_menu directory and move
render_view_context_menu.* into it as well as all the new code? I'd like to kill
c/b/tab_contents.

Second, that name makes me dizzy. I don't have a better suggestion, though.

[lazyboy, 2014-02-25 04:12:42]

On 2014/02/25 03:52:16, Avi wrote:
> On 2014/02/24 23:45:00, lazyboy wrote:
> > Avi: I've moved the files under c/b/tab_contents/menu_context/*.
> 
> :/
> 
> I'm really confused by the naming. We have a "context menu", which is the menu
> you get with a right-click, and a "menu context", which is used when deciding
> what to show in a context menu.
> 
> First, can you make a chrome/browser/context_menu directory and move
> render_view_context_menu.* into it as well as all the new code? I'd like to
kill
> c/b/tab_contents.
OK I'll do that shortly.
> 
> Second, that name makes me dizzy. I don't have a better suggestion, though.
How about ContextInfo, ContextInfoPlatformApp, ...

[Avi (use Gerrit), 2014-02-25 04:27:30]

On 2014/02/25 04:12:42, lazyboy wrote:
> On 2014/02/25 03:52:16, Avi wrote:
> > On 2014/02/24 23:45:00, lazyboy wrote:
> > > Avi: I've moved the files under c/b/tab_contents/menu_context/*.
> > 
> > :/
> > 
> > I'm really confused by the naming. We have a "context menu", which is the
menu
> > you get with a right-click, and a "menu context", which is used when
deciding
> > what to show in a context menu.
> > 
> > First, can you make a chrome/browser/context_menu directory and move
> > render_view_context_menu.* into it as well as all the new code? I'd like to
> kill
> > c/b/tab_contents.
> OK I'll do that shortly.
> > 
> > Second, that name makes me dizzy. I don't have a better suggestion, though.
> How about ContextInfo, ContextInfoPlatformApp, ...

That keeps the word "context" which is overloading it.

In "context menu" the "context" is the thing being right-clicked. That's why the
context menu has different items; it's being shown in a different context.

There's the second context, which is the type of WebContent: extension, panel,
etc. I'm wondering if there's a similar word we can use here.

ContextMenu[what goes here that means context?]

ContextMenuOrigin?
ContextMenuSource?
ContextMenuSpawner?

Fady, if you have thoughts...

[Fady Samuel, 2014-02-25 16:55:27]

On 2014/02/25 04:27:30, Avi wrote:
> On 2014/02/25 04:12:42, lazyboy wrote:
> > On 2014/02/25 03:52:16, Avi wrote:
> > > On 2014/02/24 23:45:00, lazyboy wrote:
> > > > Avi: I've moved the files under c/b/tab_contents/menu_context/*.
> > > 
> > > :/
> > > 
> > > I'm really confused by the naming. We have a "context menu", which is the
> menu
> > > you get with a right-click, and a "menu context", which is used when
> deciding
> > > what to show in a context menu.
> > > 
> > > First, can you make a chrome/browser/context_menu directory and move
> > > render_view_context_menu.* into it as well as all the new code? I'd like
to
> > kill
> > > c/b/tab_contents.
> > OK I'll do that shortly.
> > > 
> > > Second, that name makes me dizzy. I don't have a better suggestion,
though.
> > How about ContextInfo, ContextInfoPlatformApp, ...
> 
> That keeps the word "context" which is overloading it.
> 
> In "context menu" the "context" is the thing being right-clicked. That's why
the
> context menu has different items; it's being shown in a different context.
> 
> There's the second context, which is the type of WebContent: extension, panel,
> etc. I'm wondering if there's a similar word we can use here.
> 
> ContextMenu[what goes here that means context?]
> 
> ContextMenuOrigin?
> ContextMenuSource?
> ContextMenuSpawner?
> 
> Fady, if you have thoughts...

According to Merriam-Webster's Thesaurus
(http://www.merriam-webster.com/thesaurus/context), there doesn't seem to a good
synonym for "context" that works in this context. :P Eeek. How about
ContextMenuType?

[Avi (use Gerrit), 2014-02-25 17:20:41]

On 2014/02/25 16:55:27, Fady Samuel wrote:
> On 2014/02/25 04:27:30, Avi wrote:
> > On 2014/02/25 04:12:42, lazyboy wrote:
> > > On 2014/02/25 03:52:16, Avi wrote:
> > > > On 2014/02/24 23:45:00, lazyboy wrote:
> > > > > Avi: I've moved the files under c/b/tab_contents/menu_context/*.
> > > > 
> > > > :/
> > > > 
> > > > I'm really confused by the naming. We have a "context menu", which is
the
> > menu
> > > > you get with a right-click, and a "menu context", which is used when
> > deciding
> > > > what to show in a context menu.
> > > > 
> > > > First, can you make a chrome/browser/context_menu directory and move
> > > > render_view_context_menu.* into it as well as all the new code? I'd like
> to
> > > kill
> > > > c/b/tab_contents.
> > > OK I'll do that shortly.
> > > > 
> > > > Second, that name makes me dizzy. I don't have a better suggestion,
> though.
> > > How about ContextInfo, ContextInfoPlatformApp, ...
> > 
> > That keeps the word "context" which is overloading it.
> > 
> > In "context menu" the "context" is the thing being right-clicked. That's why
> the
> > context menu has different items; it's being shown in a different context.
> > 
> > There's the second context, which is the type of WebContent: extension,
panel,
> > etc. I'm wondering if there's a similar word we can use here.
> > 
> > ContextMenu[what goes here that means context?]
> > 
> > ContextMenuOrigin?
> > ContextMenuSource?
> > ContextMenuSpawner?
> > 
> > Fady, if you have thoughts...
> 
> According to Merriam-Webster's Thesaurus
> (http://www.merriam-webster.com/thesaurus/context), there doesn't seem to a
good
> synonym for "context" that works in this context. :P Eeek. How about
> ContextMenuType?

It's not the type of the context menu but the type of the thing that spawned it.
ContextMenuPageType?  A page type of "extension", a page type of "panel". It
feels good to me. What do you think?

[Fady Samuel, 2014-02-25 18:21:26]

On 2014/02/25 17:20:41, Avi wrote:
> On 2014/02/25 16:55:27, Fady Samuel wrote:
> > On 2014/02/25 04:27:30, Avi wrote:
> > > On 2014/02/25 04:12:42, lazyboy wrote:
> > > > On 2014/02/25 03:52:16, Avi wrote:
> > > > > On 2014/02/24 23:45:00, lazyboy wrote:
> > > > > > Avi: I've moved the files under c/b/tab_contents/menu_context/*.
> > > > > 
> > > > > :/
> > > > > 
> > > > > I'm really confused by the naming. We have a "context menu", which is
> the
> > > menu
> > > > > you get with a right-click, and a "menu context", which is used when
> > > deciding
> > > > > what to show in a context menu.
> > > > > 
> > > > > First, can you make a chrome/browser/context_menu directory and move
> > > > > render_view_context_menu.* into it as well as all the new code? I'd
like
> > to
> > > > kill
> > > > > c/b/tab_contents.
> > > > OK I'll do that shortly.
> > > > > 
> > > > > Second, that name makes me dizzy. I don't have a better suggestion,
> > though.
> > > > How about ContextInfo, ContextInfoPlatformApp, ...
> > > 
> > > That keeps the word "context" which is overloading it.
> > > 
> > > In "context menu" the "context" is the thing being right-clicked. That's
why
> > the
> > > context menu has different items; it's being shown in a different context.
> > > 
> > > There's the second context, which is the type of WebContent: extension,
> panel,
> > > etc. I'm wondering if there's a similar word we can use here.
> > > 
> > > ContextMenu[what goes here that means context?]
> > > 
> > > ContextMenuOrigin?
> > > ContextMenuSource?
> > > ContextMenuSpawner?
> > > 
> > > Fady, if you have thoughts...
> > 
> > According to Merriam-Webster's Thesaurus
> > (http://www.merriam-webster.com/thesaurus/context), there doesn't seem to a
> good
> > synonym for "context" that works in this context. :P Eeek. How about
> > ContextMenuType?
> 
> It's not the type of the context menu but the type of the thing that spawned
it.
> ContextMenuPageType?  A page type of "extension", a page type of "panel". It
> feels good to me. What do you think?

An "app" isn't exactly a "page". How about ContextMenuContentType?

[Avi (use Gerrit), 2014-02-25 18:49:02]

On 2014/02/25 18:21:26, Fady Samuel wrote:
> On 2014/02/25 17:20:41, Avi wrote:
> > On 2014/02/25 16:55:27, Fady Samuel wrote:
> > > On 2014/02/25 04:27:30, Avi wrote:
> > > > On 2014/02/25 04:12:42, lazyboy wrote:
> > > > > On 2014/02/25 03:52:16, Avi wrote:
> > > > > > On 2014/02/24 23:45:00, lazyboy wrote:
> > > > > > > Avi: I've moved the files under c/b/tab_contents/menu_context/*.
> > > > > > 
> > > > > > :/
> > > > > > 
> > > > > > I'm really confused by the naming. We have a "context menu", which
is
> > the
> > > > menu
> > > > > > you get with a right-click, and a "menu context", which is used when
> > > > deciding
> > > > > > what to show in a context menu.
> > > > > > 
> > > > > > First, can you make a chrome/browser/context_menu directory and move
> > > > > > render_view_context_menu.* into it as well as all the new code? I'd
> like
> > > to
> > > > > kill
> > > > > > c/b/tab_contents.
> > > > > OK I'll do that shortly.
> > > > > > 
> > > > > > Second, that name makes me dizzy. I don't have a better suggestion,
> > > though.
> > > > > How about ContextInfo, ContextInfoPlatformApp, ...
> > > > 
> > > > That keeps the word "context" which is overloading it.
> > > > 
> > > > In "context menu" the "context" is the thing being right-clicked. That's
> why
> > > the
> > > > context menu has different items; it's being shown in a different
context.
> > > > 
> > > > There's the second context, which is the type of WebContent: extension,
> > panel,
> > > > etc. I'm wondering if there's a similar word we can use here.
> > > > 
> > > > ContextMenu[what goes here that means context?]
> > > > 
> > > > ContextMenuOrigin?
> > > > ContextMenuSource?
> > > > ContextMenuSpawner?
> > > > 
> > > > Fady, if you have thoughts...
> > > 
> > > According to Merriam-Webster's Thesaurus
> > > (http://www.merriam-webster.com/thesaurus/context), there doesn't seem to
a
> > good
> > > synonym for "context" that works in this context. :P Eeek. How about
> > > ContextMenuType?
> > 
> > It's not the type of the context menu but the type of the thing that spawned
> it.
> > ContextMenuPageType?  A page type of "extension", a page type of "panel". It
> > feels good to me. What do you think?
> 
> An "app" isn't exactly a "page". How about ContextMenuContentType?

I could get behind that. lazyboy, WDYT?

[lazyboy, 2014-02-25 18:56:05]

On 2014/02/25 18:49:02, Avi wrote:
> On 2014/02/25 18:21:26, Fady Samuel wrote:
> > On 2014/02/25 17:20:41, Avi wrote:
> > > On 2014/02/25 16:55:27, Fady Samuel wrote:
> > > > On 2014/02/25 04:27:30, Avi wrote:
> > > > > On 2014/02/25 04:12:42, lazyboy wrote:
> > > > > > On 2014/02/25 03:52:16, Avi wrote:
> > > > > > > On 2014/02/24 23:45:00, lazyboy wrote:
> > > > > > > > Avi: I've moved the files under c/b/tab_contents/menu_context/*.
> > > > > > > 
> > > > > > > :/
> > > > > > > 
> > > > > > > I'm really confused by the naming. We have a "context menu", which
> is
> > > the
> > > > > menu
> > > > > > > you get with a right-click, and a "menu context", which is used
when
> > > > > deciding
> > > > > > > what to show in a context menu.
> > > > > > > 
> > > > > > > First, can you make a chrome/browser/context_menu directory and
move
> > > > > > > render_view_context_menu.* into it as well as all the new code?
I'd
> > like
> > > > to
> > > > > > kill
> > > > > > > c/b/tab_contents.
> > > > > > OK I'll do that shortly.
> > > > > > > 
> > > > > > > Second, that name makes me dizzy. I don't have a better
suggestion,
> > > > though.
> > > > > > How about ContextInfo, ContextInfoPlatformApp, ...
> > > > > 
> > > > > That keeps the word "context" which is overloading it.
> > > > > 
> > > > > In "context menu" the "context" is the thing being right-clicked.
That's
> > why
> > > > the
> > > > > context menu has different items; it's being shown in a different
> context.
> > > > > 
> > > > > There's the second context, which is the type of WebContent:
extension,
> > > panel,
> > > > > etc. I'm wondering if there's a similar word we can use here.
> > > > > 
> > > > > ContextMenu[what goes here that means context?]
> > > > > 
> > > > > ContextMenuOrigin?
> > > > > ContextMenuSource?
> > > > > ContextMenuSpawner?
> > > > > 
> > > > > Fady, if you have thoughts...
> > > > 
> > > > According to Merriam-Webster's Thesaurus
> > > > (http://www.merriam-webster.com/thesaurus/context), there doesn't seem
to
> a
> > > good
> > > > synonym for "context" that works in this context. :P Eeek. How about
> > > > ContextMenuType?
> > > 
> > > It's not the type of the context menu but the type of the thing that
spawned
> > it.
> > > ContextMenuPageType?  A page type of "extension", a page type of "panel".
It
> > > feels good to me. What do you think?
> > 
> > An "app" isn't exactly a "page". How about ContextMenuContentType?
> 
> I could get behind that. lazyboy, WDYT?

That sounds good. I'll do that change.

For the other comment to move render_view_context_menu.cc under
c/b/context_menu/*, I tried doing that:
https://chromiumcodereview.appspot.com/178193005/ and the changes required seem
to touch a lot of files (I haven't quite completed it yet). Would you be okay
with putting the newly added files in this CL under c/b/context_menu/* and move
render_view_context_menu.* related files in a subsequent patch?

Ty.

[Avi (use Gerrit), 2014-02-25 19:01:25]

On 2014/02/25 18:56:05, lazyboy wrote:
> On 2014/02/25 18:49:02, Avi wrote:
> > On 2014/02/25 18:21:26, Fady Samuel wrote:
> > > On 2014/02/25 17:20:41, Avi wrote:
> > > > On 2014/02/25 16:55:27, Fady Samuel wrote:
> > > > > On 2014/02/25 04:27:30, Avi wrote:
> > > > > > On 2014/02/25 04:12:42, lazyboy wrote:
> > > > > > > On 2014/02/25 03:52:16, Avi wrote:
> > > > > > > > On 2014/02/24 23:45:00, lazyboy wrote:
> > > > > > > > > Avi: I've moved the files under
c/b/tab_contents/menu_context/*.
> > > > > > > > 
> > > > > > > > :/
> > > > > > > > 
> > > > > > > > I'm really confused by the naming. We have a "context menu",
which
> > is
> > > > the
> > > > > > menu
> > > > > > > > you get with a right-click, and a "menu context", which is used
> when
> > > > > > deciding
> > > > > > > > what to show in a context menu.
> > > > > > > > 
> > > > > > > > First, can you make a chrome/browser/context_menu directory and
> move
> > > > > > > > render_view_context_menu.* into it as well as all the new code?
> I'd
> > > like
> > > > > to
> > > > > > > kill
> > > > > > > > c/b/tab_contents.
> > > > > > > OK I'll do that shortly.
> > > > > > > > 
> > > > > > > > Second, that name makes me dizzy. I don't have a better
> suggestion,
> > > > > though.
> > > > > > > How about ContextInfo, ContextInfoPlatformApp, ...
> > > > > > 
> > > > > > That keeps the word "context" which is overloading it.
> > > > > > 
> > > > > > In "context menu" the "context" is the thing being right-clicked.
> That's
> > > why
> > > > > the
> > > > > > context menu has different items; it's being shown in a different
> > context.
> > > > > > 
> > > > > > There's the second context, which is the type of WebContent:
> extension,
> > > > panel,
> > > > > > etc. I'm wondering if there's a similar word we can use here.
> > > > > > 
> > > > > > ContextMenu[what goes here that means context?]
> > > > > > 
> > > > > > ContextMenuOrigin?
> > > > > > ContextMenuSource?
> > > > > > ContextMenuSpawner?
> > > > > > 
> > > > > > Fady, if you have thoughts...
> > > > > 
> > > > > According to Merriam-Webster's Thesaurus
> > > > > (http://www.merriam-webster.com/thesaurus/context), there doesn't seem
> to
> > a
> > > > good
> > > > > synonym for "context" that works in this context. :P Eeek. How about
> > > > > ContextMenuType?
> > > > 
> > > > It's not the type of the context menu but the type of the thing that
> spawned
> > > it.
> > > > ContextMenuPageType?  A page type of "extension", a page type of
"panel".
> It
> > > > feels good to me. What do you think?
> > > 
> > > An "app" isn't exactly a "page". How about ContextMenuContentType?
> > 
> > I could get behind that. lazyboy, WDYT?
> 
> That sounds good. I'll do that change.
> 
> For the other comment to move render_view_context_menu.cc under
> c/b/context_menu/*, I tried doing that:
> https://chromiumcodereview.appspot.com/178193005/ and the changes required
seem
> to touch a lot of files (I haven't quite completed it yet). Would you be okay
> with putting the newly added files in this CL under c/b/context_menu/* and
move
> render_view_context_menu.* related files in a subsequent patch?
> 
> Ty.

I would be happy with that. Moving render_view_context_menu.* around is going to
be a ton of noise, so it's probably better that way anyway.

(SLGTM)

[lazyboy, 2014-02-25 20:49:08]

I've moved files under c/b/context_menu/* and renamed them to
ContextMenuContentTypeX, Avi/Fady PTAL.

[Fady Samuel, 2014-02-26 17:49:33]

lgtm

[Avi (use Gerrit), 2014-02-26 17:55:50]

LGTM

https://codereview.chromium.org/169093009/diff/490001/chrome/browser/context_...
File chrome/browser/context_menu/OWNERS (right):

https://codereview.chromium.org/169093009/diff/490001/chrome/browser/context_...
chrome/browser/context_menu/OWNERS:2: brettw@chromium.org
What? Not you? :)

[lazyboy, 2014-02-26 19:55:11]

Looks like we are going to rename the directory to renderer_context_menu/
(crrev.com/178193005), I'll wait to update this CL until the other CL is
settled.

https://chromiumcodereview.appspot.com/169093009/diff/490001/chrome/browser/c...
File chrome/browser/context_menu/OWNERS (right):

https://chromiumcodereview.appspot.com/169093009/diff/490001/chrome/browser/c...
chrome/browser/context_menu/OWNERS:2: brettw@chromium.org
On 2014/02/26 17:55:51, Avi wrote:
> What? Not you? :)

Added myself :)

[lazyboy, 2014-02-28 19:45:09]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-02-28 19:49:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/169093009/590001

[commit-bot: I haz the power, 2014-02-28 20:42:39]

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

[commit-bot: I haz the power, 2014-02-28 20:42:40]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[lazyboy, 2014-02-28 20:54:36]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-02-28 20:55:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/lazyboy@chromium.org/169093009/610001

[commit-bot: I haz the power, 2014-03-01 00:22:46]

Change committed as 254287