<webview>: Context menu API implementation CL.

<webview>: Context menu API implementation CL.

Very Similar to chrome.contextMenus API, only applies to the <webview> that it is called upon. 

var w = document.querySelector('webview'); 
w.contextMenus.OnClicked.addListener(function() { .. }); 
w.contextMenus.create({id: '1', title: 'one'}); 
w.contextMenus.update({id: '2', title: 'new', onclick: ...}); 
w.contextMenus.remove('1'); 
w.contextMenus.removeAll('1'); 

This CL adds the actual implementation to the previously exposed skeleton API.

Docs/snippets here:
https://docs.google.com/a/chromium.org/document/d/1AoMD6kF8ui1dikzTrwK-weVHegSqQaLV2zx4xJSh_fQ/edit

TBR=stevenjb@chromium.org
BUG=140315
Test=Can be tested inside a chrome apps <webview>.
<webview>.contextMenus.create(...) to create custom context menu items.
The API is similar to chrome.contextMenus API. Similarly .update,
.remove, .removeAll is also supported.
Click event handler can be specified as
<webview>.contextMenus.onClick.addListener(...).

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

[lazyboy, 2014-03-04 01:13:16]

Please take a look before I forward it to Yoyo.

[Fady Samuel, 2014-03-04 01:56:46]

https://codereview.chromium.org/186213003/diff/1/chrome/browser/extensions/me...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/186213003/diff/1/chrome/browser/extensions/me...
chrome/browser/extensions/menu_manager.cc:516: for (i =
context_items_[extension_id].begin();
I think it would be much cleaner if context items were keyed on webview instance
ID.

Perhaps context items can be a key-value pair where the key is <ExtensionId,
WebViewInstanceID<

https://codereview.chromium.org/186213003/diff/1/chrome/browser/extensions/me...
chrome/browser/extensions/menu_manager.cc:695:
properties->SetInteger("webviewInstanceId", guest_view->view_instance_id());
What is this?

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

https://codereview.chromium.org/186213003/diff/1/chrome/browser/renderer_cont...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:419:
WebViewGuest::FromWebContents(source_web_contents_))),
What if the webview is deleted while the context menu is showing? Is this safe?

[lazyboy, 2014-03-04 16:11:31]

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
chrome/browser/extensions/menu_manager.cc:516: for (i =
context_items_[extension_id].begin();
On 2014/03/04 01:56:46, Fady Samuel wrote:
> I think it would be much cleaner if context items were keyed on webview
instance
> ID.
> 
> Perhaps context items can be a key-value pair where the key is <ExtensionId,
> WebViewInstanceID<

Hmmm, for most cases, webview_instance_id is 0, adding an indirection step to
all cases for the webview case feels inefficient to me.

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
chrome/browser/extensions/menu_manager.cc:695:
properties->SetInteger("webviewInstanceId", guest_view->view_instance_id());
On 2014/03/04 01:56:46, Fady Samuel wrote:
> What is this?

This is a property on the onClick event.

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/render...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/render...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:419:
WebViewGuest::FromWebContents(source_web_contents_))),
On 2014/03/04 01:56:46, Fady Samuel wrote:
> What if the webview is deleted while the context menu is showing? Is this
safe?

I've changed this so we pass in the view_instance_id directly instead of passing
WebViewGuest*.

[Fady Samuel, 2014-03-04 16:36:00]

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
chrome/browser/extensions/menu_manager.cc:516: for (i =
context_items_[extension_id].begin();
On 2014/03/04 16:11:31, lazyboy wrote:
> On 2014/03/04 01:56:46, Fady Samuel wrote:
> > I think it would be much cleaner if context items were keyed on webview
> instance
> > ID.
> > 
> > Perhaps context items can be a key-value pair where the key is <ExtensionId,
> > WebViewInstanceID<
> 
> Hmmm, for most cases, webview_instance_id is 0, adding an indirection step to
> all cases for the webview case feels inefficient to me. 

There is no additional indirection being added in this proposal. It is just a
matter of moving hte webview_instance_id into the key rather than the value.
context_items_ is an std::map<std::string, MenuItem::List>. You can do the
equivalent of what I did in the declarative WebRequest API:

See RulesRegistryKey:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

and WebViewKey:

https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
chrome/browser/extensions/menu_manager.cc:695:
properties->SetInteger("webviewInstanceId", guest_view->view_instance_id());
On 2014/03/04 16:11:31, lazyboy wrote:
> On 2014/03/04 01:56:46, Fady Samuel wrote:
> > What is this?
> 
> This is a property on the onClick event.

Is this visible to the developer? What's it used for?

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:128: if
(params->menu_item_id.as_string)
{} blocks preferred for readability when there is an else clause. I'm not sure
if this is in the style guide but just a preference of mine. You're less likely
to make mistakes that way in the future as code is added.

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/gu...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/gu...
chrome/browser/guestview/webview/webview_guest.cc:175: GuestView* guest =
GuestView::FromWebContents(contents);
A small simplification:

WebViewGuest* guest = FromWebContents(contents);
if (!guest)
  return guestview::kInstanceIDNone;

return guest->view_instance_id();

[lazyboy, 2014-03-04 20:50:24]

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
chrome/browser/extensions/menu_manager.cc:516: for (i =
context_items_[extension_id].begin();
On 2014/03/04 16:36:00, Fady Samuel wrote:
> On 2014/03/04 16:11:31, lazyboy wrote:
> > On 2014/03/04 01:56:46, Fady Samuel wrote:
> > > I think it would be much cleaner if context items were keyed on webview
> > instance
> > > ID.
> > > 
> > > Perhaps context items can be a key-value pair where the key is
<ExtensionId,
> > > WebViewInstanceID<
> > 
> > Hmmm, for most cases, webview_instance_id is 0, adding an indirection step
to
> > all cases for the webview case feels inefficient to me. 
> 
> There is no additional indirection being added in this proposal. It is just a
> matter of moving hte webview_instance_id into the key rather than the value.
> context_items_ is an std::map<std::string, MenuItem::List>. You can do the
> equivalent of what I did in the declarative WebRequest API:
> 
> See RulesRegistryKey:
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...
> 
> and WebViewKey:
> 
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ext...

Included instance id to the key, called MenuItem::ExtensionKey

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
chrome/browser/extensions/menu_manager.cc:695:
properties->SetInteger("webviewInstanceId", guest_view->view_instance_id());
On 2014/03/04 16:36:00, Fady Samuel wrote:
> On 2014/03/04 16:11:31, lazyboy wrote:
> > On 2014/03/04 01:56:46, Fady Samuel wrote:
> > > What is this?
> > 
> > This is a property on the onClick event.
> 
> Is this visible to the developer? What's it used for?

Yes.
This is to indicate which webview the event corresponds to. One can use one
handler to handle all webview's events this way.
I don't have a concrete use case other than this. Generally this event info has
lots of params about the context, so I thought it would be nice to add them.

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/ex...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:128: if
(params->menu_item_id.as_string)
On 2014/03/04 16:36:00, Fady Samuel wrote:
> {} blocks preferred for readability when there is an else clause. I'm not sure
> if this is in the style guide but just a preference of mine. You're less
likely
> to make mistakes that way in the future as code is added.
Don't think it is in style guide.
Done.

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/gu...
File chrome/browser/guestview/webview/webview_guest.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/20001/chrome/browser/gu...
chrome/browser/guestview/webview/webview_guest.cc:175: GuestView* guest =
GuestView::FromWebContents(contents);
On 2014/03/04 16:36:00, Fady Samuel wrote:
> A small simplification:
> 
> WebViewGuest* guest = FromWebContents(contents);
> if (!guest)
>   return guestview::kInstanceIDNone;
> 
> return guest->view_instance_id();

Done, thanks.

[lazyboy, 2014-03-04 21:13:49]

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/1/chrome/browser/extens...
chrome/browser/extensions/menu_manager.cc:695:
properties->SetInteger("webviewInstanceId", guest_view->view_instance_id());
On 2014/03/04 20:50:24, lazyboy wrote:
> On 2014/03/04 16:36:00, Fady Samuel wrote:
> > On 2014/03/04 16:11:31, lazyboy wrote:
> > > On 2014/03/04 01:56:46, Fady Samuel wrote:
> > > > What is this?
> > > 
> > > This is a property on the onClick event.
> > 
> > Is this visible to the developer? What's it used for?
> 
> Yes.
> This is to indicate which webview the event corresponds to. One can use one
> handler to handle all webview's events this way.
> I don't have a concrete use case other than this. Generally this event info
has
> lots of params about the context, so I thought it would be nice to add them.

As discussed offline, I've removed exposing webviewInstanceId to the event.

[Fady Samuel, 2014-03-04 21:14:58]

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:657: GuestView* guest_view =
GuestView::FromWebContents(web_contents);
Can we make this WebViewGuest::FromWebContents instead? It's not yet clear
whether we want non-<webview> guests to have this functionality. We can change
it when the need arises.

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:888:
MenuItem::ExtensionKey::ExtensionKey(const MenuItem::Id& item_id)
We can probably get rid of this if MenuItem::Id has an ExtensionKey.

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:919: extension_id(extension_id),
So MenuItem now has an extension_id, and a webview_instance_id. Can this simply
be an ExtensionKey?

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:929: webview_instance_id ==
other.webview_instance_id);
one less condition to check if this thing was an ExtensionKey.

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:943: if (webview_instance_id <
other.webview_instance_id)
If this was an ExtensionKey you could do both comparisons in one line.

[lazyboy, 2014-03-04 23:07:01]

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:657: GuestView* guest_view =
GuestView::FromWebContents(web_contents);
On 2014/03/04 21:14:59, Fady Samuel wrote:
> Can we make this WebViewGuest::FromWebContents instead? It's not yet clear
> whether we want non-<webview> guests to have this functionality. We can change
> it when the need arises.

Done.

Also, the webviewInstanceId thing is something I'm using in
webview_custom_bindings.js, I forgot about that. However, I'm not exposing it to
the developer.

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:888:
MenuItem::ExtensionKey::ExtensionKey(const MenuItem::Id& item_id)
On 2014/03/04 21:14:59, Fady Samuel wrote:
> We can probably get rid of this if MenuItem::Id has an ExtensionKey.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:919: extension_id(extension_id),
On 2014/03/04 21:14:59, Fady Samuel wrote:
> So MenuItem now has an extension_id, and a webview_instance_id. Can this
simply
> be an ExtensionKey?

Done.

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:929: webview_instance_id ==
other.webview_instance_id);
On 2014/03/04 21:14:59, Fady Samuel wrote:
> one less condition to check if this thing was an ExtensionKey.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/30001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:943: if (webview_instance_id <
other.webview_instance_id)
On 2014/03/04 21:14:59, Fady Samuel wrote:
> If this was an ExtensionKey you could do both comparisons in one line.

Done.

[Fady Samuel, 2014-03-05 17:54:09]

Bunch of comments as I read through the code that all more or less center around
one theme: making MenuItem::Id's constructor take in an ExtensionKey.

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h
(right):

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:20:
template <typename PropertyWithEnumT>
Why add this space?

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:29: new
extensions::MenuItem::Id(is_off_the_record, key.extension_id));
As before, this really should take in an extension_key.

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:38:
parent_id->extension_key.webview_instance_id = key.webview_instance_id;
If MenuItem::Id took in an extension_key instead, these two lines become
unnecessary.

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (left):

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:91: MenuItem::Id
item_id(profile->IsOffTheRecord(), extension_id());
As above, MenuItem::Id should probably take in an ExtensionKey

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:70:
id.extension_key.webview_instance_id = params->instance_id;
MenuItem::Id should probably take in an ExtensionKey instead.

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:125: extension_id());
As above, MenuItem::Id should probably take in an ExtensionKey.

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/api/webview/webview_api.cc:139: if (!item ||
item->extension_id() != extension_id()) {
If item->extension() != extension_id() then we have a compromised renderer,
right? This should probably be a CHECK:
http://www.chromium.org/developers/coding-style

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/extension...
chrome/browser/extensions/menu_manager.cc:778: if
(extension_key.webview_instance_id)
A comment above this would be useful.

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/renderer_...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:506: // static.
This change seems unnecesary.

https://codereview.chromium.org/186213003/diff/90001/chrome/browser/renderer_...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:560: const
MenuItem::ExtensionKey& extension_key = MenuItem::ExtensionKey(id);
This doesn't look right. You're constructing a temporary object and holding a
reference to it. I think that may produce undefined behavior:

MenuItem::ExtensionKey extension_key(id);

is probably what you want.

[lazyboy, 2014-03-05 18:27:58]

Bunch of comments as I read through the code that all more or less center around
one theme: making MenuItem::Id's constructor take in an ExtensionKey.

->
I've changed the ctor from taking string extension_id to ExtensionKey.
I'm arguing whether I should keep both contructors, b/c with this change I had
to change
MenuItem::Id(.., extension_id()) to
MenuItem::Id(.., ExtensionKey(extension_id()) in bunch of places.
However, too many overloaded constructors...

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
File chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h
(right):

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:20:
template <typename PropertyWithEnumT>
On 2014/03/05 17:54:10, Fady Samuel wrote:
> Why add this space?

git cl format did this.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:29: new
extensions::MenuItem::Id(is_off_the_record, key.extension_id));
On 2014/03/05 17:54:10, Fady Samuel wrote:
> As before, this really should take in an extension_key.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/context_menus/context_menus_api_helpers.h:38:
parent_id->extension_key.webview_instance_id = key.webview_instance_id;
On 2014/03/05 17:54:10, Fady Samuel wrote:
> If MenuItem::Id took in an extension_key instead, these two lines become
> unnecessary.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
File chrome/browser/extensions/api/webview/webview_api.cc (left):

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:91: MenuItem::Id
item_id(profile->IsOffTheRecord(), extension_id());
On 2014/03/05 17:54:10, Fady Samuel wrote:
> As above, MenuItem::Id should probably take in an ExtensionKey

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:70:
id.extension_key.webview_instance_id = params->instance_id;
On 2014/03/05 17:54:10, Fady Samuel wrote:
> MenuItem::Id should probably take in an ExtensionKey instead.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:70:
id.extension_key.webview_instance_id = params->instance_id;
On 2014/03/05 17:54:10, Fady Samuel wrote:
> MenuItem::Id should probably take in an ExtensionKey instead.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:125: extension_id());
On 2014/03/05 17:54:10, Fady Samuel wrote:
> As above, MenuItem::Id should probably take in an ExtensionKey.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:125: extension_id());
On 2014/03/05 17:54:10, Fady Samuel wrote:
> As above, MenuItem::Id should probably take in an ExtensionKey.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/api/webview/webview_api.cc:139: if (!item ||
item->extension_id() != extension_id()) {
On 2014/03/05 17:54:10, Fady Samuel wrote:
> If item->extension() != extension_id() then we have a compromised renderer,
> right? This should probably be a CHECK:
> http://www.chromium.org/developers/coding-style

This is code from context_menus_api.cc and it was like this there.
The API allows you to call contextMenus.remove with any id, and this bit throws
an error if you're not allowed. Don't think we should kill the renderer here.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/ex...
chrome/browser/extensions/menu_manager.cc:778: if
(extension_key.webview_instance_id)
On 2014/03/05 17:54:10, Fady Samuel wrote:
> A comment above this would be useful. 

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/re...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/re...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:506: // static.
On 2014/03/05 17:54:10, Fady Samuel wrote:
> This change seems unnecesary.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/90001/chrome/browser/re...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:560: const
MenuItem::ExtensionKey& extension_key = MenuItem::ExtensionKey(id);
On 2014/03/05 17:54:10, Fady Samuel wrote:
> This doesn't look right. You're constructing a temporary object and holding a
> reference to it. I think that may produce undefined behavior:
> 
> MenuItem::ExtensionKey extension_key(id);
> 
> is probably what you want.

Ou, don't know how it got there, fixed, thanks.

[Fady Samuel, 2014-03-05 18:51:37]

lgtm

[lazyboy, 2014-03-06 05:29:16]

+yoz@ for review.

I've added browsertests.
Yoyo, can you take a look? This is the last bit of implementing
<webview>.contextMenus API.

[lazyboy, 2014-03-06 17:21:14]

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/test/data...
File
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/manifest.json
(right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/manifest.json:8:
"contextMenus"
FYI, this is something I still need to fix.
Internal only chrome.contextMenus bits need to be extracted to something like
contextMenusInternal and we need to specify "webview" permission implies
"contextMenusInternal" permission. Similar stuff is done for webRequestInternal:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

[Yoyo Zhou, 2014-03-07 02:40:30]

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/a...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/a...
chrome/browser/apps/web_view_browsertest.cc:142: class TestRenderViewContextMenu
: public RenderViewContextMenu {
This looks duplicated from ExtensionContextMenuBrowserTest. There is also a
class by this name in render_view_context_menu_test_util.h. Can we combine this
with at least the first one?

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/a...
chrome/browser/apps/web_view_browsertest.cc:2060: 
nit: stray newline

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/api/webview/webview_api.cc:137: // Ensure one
extension can't remove another's menu items.
Do you want to check that webview IDs match here also?

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.cc:901: if (webview_instance_id !=
other.webview_instance_id)
nit: I would sort using extension id as the primary key.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
File chrome/browser/extensions/menu_manager.h (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.h:45: // Key used to identify which
extension a menu item belongs to.
This sentence should mention webviews.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.h:276: // Returns the ids of extensions
which have menu items registered.
update comment

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.h:365: // We keep items organized by
mapping an extension id to a list of items.
update comment

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/g...
File chrome/browser/guestview/webview/webview_guest.h (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/g...
chrome/browser/guestview/webview/webview_guest.h:41: // Returns 0 if |contents|
does not correspond to a WebViewGuest.
Why not say kInstanceIDNone?

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/test/data...
File
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/manifest.json
(right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/manifest.json:8:
"contextMenus"
On 2014/03/06 17:21:14, lazyboy wrote:
> FYI, this is something I still need to fix.
> Internal only chrome.contextMenus bits need to be extracted to something like
> contextMenusInternal and we need to specify "webview" permission implies
> "contextMenusInternal" permission. Similar stuff is done for
webRequestInternal:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...

What will happen in the interim?

[lazyboy, 2014-03-07 06:38:02]

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/a...
File chrome/browser/apps/web_view_browsertest.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/a...
chrome/browser/apps/web_view_browsertest.cc:142: class TestRenderViewContextMenu
: public RenderViewContextMenu {
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> This looks duplicated from ExtensionContextMenuBrowserTest. There is also a
> class by this name in render_view_context_menu_test_util.h. Can we combine
this
> with at least the first one?

I've made WebViewTest (this) and ExtensionContextMenuBrowserTest use the one
from render_view_context_menu_test_util.h (after some modification).

Altho, codesearch shows there are 5+ other TestRenderViewContextMenu-s :(
I've filed http://crbug.com/350302 to track.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/a...
chrome/browser/apps/web_view_browsertest.cc:2060: 
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> nit: stray newline

Done.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
File chrome/browser/extensions/api/webview/webview_api.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/api/webview/webview_api.cc:137: // Ensure one
extension can't remove another's menu items.
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> Do you want to check that webview IDs match here also?

Good idea.
Done.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
File chrome/browser/extensions/menu_manager.cc (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.cc:901: if (webview_instance_id !=
other.webview_instance_id)
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> nit: I would sort using extension id as the primary key.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
File chrome/browser/extensions/menu_manager.h (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.h:45: // Key used to identify which
extension a menu item belongs to.
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> This sentence should mention webviews.

Done.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.h:276: // Returns the ids of extensions
which have menu items registered.
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> update comment

Done.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/e...
chrome/browser/extensions/menu_manager.h:365: // We keep items organized by
mapping an extension id to a list of items.
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> update comment

Done.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/g...
File chrome/browser/guestview/webview/webview_guest.h (right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/browser/g...
chrome/browser/guestview/webview/webview_guest.h:41: // Returns 0 if |contents|
does not correspond to a WebViewGuest.
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> Why not say kInstanceIDNone?

Done.

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/test/data...
File
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/manifest.json
(right):

https://chromiumcodereview.appspot.com/186213003/diff/220001/chrome/test/data...
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/manifest.json:8:
"contextMenus"
On 2014/03/07 02:40:30, Yoyo Zhou wrote:
> On 2014/03/06 17:21:14, lazyboy wrote:
> > FYI, this is something I still need to fix.
> > Internal only chrome.contextMenus bits need to be extracted to something
like
> > contextMenusInternal and we need to specify "webview" permission implies
> > "contextMenusInternal" permission. Similar stuff is done for
> webRequestInternal:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/common/...
> 
> What will happen in the interim?

Right now I need to specify "contextMenus" permission to make this work as I did
here.
I'm working on the mentioned *Internal approach, will send that out to code
review separately.

[Yoyo Zhou, 2014-03-07 23:03:21]

LGTM, looks much better. Thanks for cleaning up the test menu code.

https://chromiumcodereview.appspot.com/186213003/diff/270001/chrome/browser/r...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc
(right):

https://chromiumcodereview.appspot.com/186213003/diff/270001/chrome/browser/r...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc:18:
// static.
nit: no .

[lazyboy, 2014-03-08 00:16:45]

https://chromiumcodereview.appspot.com/186213003/diff/270001/chrome/browser/r...
File chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc
(right):

https://chromiumcodereview.appspot.com/186213003/diff/270001/chrome/browser/r...
chrome/browser/renderer_context_menu/render_view_context_menu_test_util.cc:18:
// static.
On 2014/03/07 23:03:21, Yoyo Zhou wrote:
> nit: no .

Done.

[lazyboy, 2014-03-08 02:02:59]

The CQ bit was checked by lazyboy@chromium.org

[lazyboy, 2014-03-08 06:48:53]

The CQ bit was unchecked by lazyboy@chromium.org

[lazyboy, 2014-03-08 06:49:05]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-03-08 11:16:03]

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

[commit-bot: I haz the power, 2014-03-08 11:49:00]

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

[commit-bot: I haz the power, 2014-03-08 11:49:01]

Try jobs failed on following builders: linux_chromium_rel

[lazyboy, 2014-03-08 13:49:58]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-03-08 13:50:45]

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

[commit-bot: I haz the power, 2014-03-08 14:18:53]

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

[commit-bot: I haz the power, 2014-03-08 14:18:53]

Try jobs failed on following builders: mac_chromium_compile_dbg

[lazyboy, 2014-03-08 20:24:41]

xiyuan@chromium.org: Please review changes in 

chrome/browser/ui/app_list/app_context_menu.cc

[lazyboy, 2014-03-08 23:49:51]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-03-08 23:49:57]

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

[lazyboy, 2014-03-08 23:49:57]

The CQ bit was unchecked by lazyboy@chromium.org

[lazyboy, 2014-03-08 23:50:35]

The CQ bit was unchecked by lazyboy@chromium.org

[xiyuan, 2014-03-09 01:09:39]

app_list lgtm

[lazyboy, 2014-03-09 06:39:15]

The CQ bit was checked by lazyboy@chromium.org

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

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

[commit-bot: I haz the power, 2014-03-09 07:12:25]

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

[commit-bot: I haz the power, 2014-03-09 07:12:26]

Try jobs failed on following builders: linux_chromium_chromeos_clang_dbg

[lazyboy, 2014-03-10 10:10:12]

The CQ bit was checked by lazyboy@chromium.org

[commit-bot: I haz the power, 2014-03-10 10:10:22]

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

[commit-bot: I haz the power, 2014-03-10 12:12:34]

Change committed as 255917