WebView context menu cleanup.

This patch allows context menus stored for a WebView to be cleaned up consistently when the WebView is destroyed.

BUG=355360
Committed: https://crrev.com/ed43726a3ae771b91a91feee22395f344e655d7a
Cr-Commit-Position: refs/heads/master@{#338053}

[paulmeyer, 2015-06-18 18:09:17]

paulmeyer@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[paulmeyer, 2015-06-18 18:09:18]

+rdevlin.cronin@

[Devlin, 2015-06-18 19:39:57]

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extensions_browser_client.cc:335: auto
menu_manager =
What's the motivation for moving this from the guest delegate?

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
chrome/browser/extensions/menu_manager.h:59: ExtensionKey(const std::string&
extension_id,
If we allow matching based on view id and process id, in which circumstance do
we need to also provide an extension id with these other two?  I think it mostly
just confuses things... I think I'd rather see us use *either* extension id (for
extensions) *or* view/process id (for web views).

[paulmeyer, 2015-06-18 19:49:59]

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extensions_browser_client.cc:335: auto
menu_manager =
On 2015/06/18 19:39:57, Devlin wrote:
> What's the motivation for moving this from the guest delegate?

When there is no WebViewGuest (yet), there is also no guest delegate. It is
possible for a WebView to be destroyed before WebViewGuest and the delegate are
created, in which case this cleanup would never happen, and the context menus
would all be leaked (without this CL).

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
chrome/browser/extensions/menu_manager.h:59: ExtensionKey(const std::string&
extension_id,
On 2015/06/18 19:39:57, Devlin wrote:
> If we allow matching based on view id and process id, in which circumstance do
> we need to also provide an extension id with these other two?  I think it
mostly
> just confuses things... I think I'd rather see us use *either* extension id
(for
> extensions) *or* view/process id (for web views).

Okay. If I were to use that approach, would you want to rename "ExtensionKey" to
something else, since it would sometimes not have any extension-specific data in
it?

[Devlin, 2015-06-18 20:09:21]

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extensions_browser_client.cc:335: auto
menu_manager =
On 2015/06/18 19:49:59, Paul Meyer wrote:
> On 2015/06/18 19:39:57, Devlin wrote:
> > What's the motivation for moving this from the guest delegate?
> 
> When there is no WebViewGuest (yet), there is also no guest delegate. It is
> possible for a WebView to be destroyed before WebViewGuest and the delegate
are
> created, in which case this cleanup would never happen, and the context menus
> would all be leaked (without this CL).

Ah, makes sense.  Thanks!

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
chrome/browser/extensions/menu_manager.h:59: ExtensionKey(const std::string&
extension_id,
On 2015/06/18 19:49:59, Paul Meyer wrote:
> On 2015/06/18 19:39:57, Devlin wrote:
> > If we allow matching based on view id and process id, in which circumstance
do
> > we need to also provide an extension id with these other two?  I think it
> mostly
> > just confuses things... I think I'd rather see us use *either* extension id
> (for
> > extensions) *or* view/process id (for web views).
> 
> Okay. If I were to use that approach, would you want to rename "ExtensionKey"
to
> something else, since it would sometimes not have any extension-specific data
in
> it?

I think "Key" would be sufficient.  If it's not an obscene amount of churn, go
for it, otherwise a "TODO: This isn't restricted to extensions" would suffice.

[paulmeyer, 2015-06-19 17:49:40]

Patchset #2 (id:20001) has been deleted

[paulmeyer, 2015-06-19 17:57:28]

ptal.

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/1/chrome/browser/extensions/m...
chrome/browser/extensions/menu_manager.h:59: ExtensionKey(const std::string&
extension_id,
On 2015/06/18 20:09:21, Devlin wrote:
> On 2015/06/18 19:49:59, Paul Meyer wrote:
> > On 2015/06/18 19:39:57, Devlin wrote:
> > > If we allow matching based on view id and process id, in which
circumstance
> do
> > > we need to also provide an extension id with these other two?  I think it
> > mostly
> > > just confuses things... I think I'd rather see us use *either* extension
id
> > (for
> > > extensions) *or* view/process id (for web views).
> > 
> > Okay. If I were to use that approach, would you want to rename
"ExtensionKey"
> to
> > something else, since it would sometimes not have any extension-specific
data
> in
> > it?
> 
> I think "Key" would be sufficient.  If it's not an obscene amount of churn, go
> for it, otherwise a "TODO: This isn't restricted to extensions" would suffice.

I made the change and left a TODO for now. I will also rename it to "Key", but I
believe that that is a large enough change to warrant being in a subsequent CL,
as it would likely just muddy this one.

[Fady Samuel, 2015-06-19 18:03:11]

fsamuel@chromium.org changed reviewers:
+ fsamuel@chromium.org

[Fady Samuel, 2015-06-19 18:03:12]

Drive by comment.

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h
(right):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:32: void
OnGuestDestroyed() override {}
Please get rid of this entirely from delegates if we're no longer using it.

[Devlin, 2015-06-19 18:05:48]

LGTM with nits.

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:331: auto
browser_context = content::RenderProcessHost::FromID(embedder_process_id)
nit: This is probably just my preference, but please only use "auto" when it
helps with readability - i.e., when the returned result is something crazy
(std::map<std::pair<int, std::string>, std::string>::const_iterator is a could
example) and the returned result is fairly obvious.  In this case,
content::BrowserContext* is very reasonable, so I'd prefer that.

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.h:48: // an extension, than
|extension_id| will be non-zero. Alternatively, if the
nit: s/than/then

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h
(right):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:32: void
OnGuestDestroyed() override {}
On 2015/06/19 18:03:12, Fady Samuel wrote:
> Please get rid of this entirely from delegates if we're no longer using it.

+1.

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

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:523: auto
web_view_guest =
same thought about auto.

[paulmeyer, 2015-06-19 18:18:10]

ptal.

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:331: auto
browser_context = content::RenderProcessHost::FromID(embedder_process_id)
On 2015/06/19 18:05:48, Devlin wrote:
> nit: This is probably just my preference, but please only use "auto" when it
> helps with readability - i.e., when the returned result is something crazy
> (std::map<std::pair<int, std::string>, std::string>::const_iterator is a could
> example) and the returned result is fairly obvious.  In this case,
> content::BrowserContext* is very reasonable, so I'd prefer that.

Done.

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.h:48: // an extension, than
|extension_id| will be non-zero. Alternatively, if the
On 2015/06/19 18:05:48, Devlin wrote:
> nit: s/than/then

Good catch! Done.

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/guest_vi...
File chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h
(right):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/guest_vi...
chrome/browser/guest_view/web_view/chrome_web_view_guest_delegate.h:32: void
OnGuestDestroyed() override {}
On 2015/06/19 18:03:12, Fady Samuel wrote:
> Please get rid of this entirely from delegates if we're no longer using it.

Done.

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

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:523: auto
web_view_guest =
On 2015/06/19 18:05:48, Devlin wrote:
> same thought about auto.

Done.

[Fady Samuel, 2015-06-19 18:36:10]

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:525:
extensions::WebViewGuest::GetViewInstanceId(source_web_contents_));
Do we still need GetViewInstanceId?

[paulmeyer, 2015-06-19 18:50:59]

fsamuel@: ptal.

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/renderer...
File chrome/browser/renderer_context_menu/render_view_context_menu.cc (left):

https://codereview.chromium.org/1181263007/diff/40001/chrome/browser/renderer...
chrome/browser/renderer_context_menu/render_view_context_menu.cc:525:
extensions::WebViewGuest::GetViewInstanceId(source_web_contents_));
On 2015/06/19 18:36:10, Fady Samuel wrote:
> Do we still need GetViewInstanceId?

Nope. Removed.

[Fady Samuel, 2015-06-19 18:53:08]

LGTM

[paulmeyer, 2015-06-19 18:53:42]

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

[paulmeyer, 2015-06-19 18:53:43]

+lazyboy@ for render_view_context_menu.cc

[lazyboy, 2015-06-19 20:34:59]

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.cc:916: return extension_id <
other.extension_id;
Is there any reason why you put this at the end? in other operators this is the
primary key that we use for comparison. Please move it back to first if there
isn't any specific reason. I can only assumed you want to reduce string
comparison?

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.h:48: // an extension, then
|extension_id| will be non-zero. Alternatively, if the
|extension_id| will be non-empty you mean?

[paulmeyer, 2015-06-23 13:38:13]

Patchset #5 (id:100001) has been deleted

[paulmeyer, 2015-06-23 13:42:50]

Patchset #5 (id:120001) has been deleted

[paulmeyer, 2015-06-23 13:43:28]

lazyboy@: ptal.

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.cc:916: return extension_id <
other.extension_id;
On 2015/06/19 20:34:59, lazyboy wrote:
> Is there any reason why you put this at the end? in other operators this is
the
> primary key that we use for comparison. Please move it back to first if there
> isn't any specific reason. I can only assumed you want to reduce string
> comparison?

Done.

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/menu_manager.h (right):

https://codereview.chromium.org/1181263007/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/menu_manager.h:48: // an extension, then
|extension_id| will be non-zero. Alternatively, if the
On 2015/06/19 20:34:59, lazyboy wrote:
> |extension_id| will be non-empty you mean?

Done.

[lazyboy, 2015-06-23 14:14:27]

lgtm

https://codereview.chromium.org/1181263007/diff/140001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/1181263007/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:887: : webview_embedder_process_id(0),
webview_instance_id(0) {}
kInvalidUniqueId (child_process_host.h) is the correct process id to use for
invalid process id.

https://codereview.chromium.org/1181263007/diff/140001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:924: return extension_id.empty() &&
!webview_embedder_process_id &&
Update here too.

[paulmeyer, 2015-06-23 14:14:52]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-06-23 14:14:52]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org, fsamuel@chromium.org
Link to the patchset: https://codereview.chromium.org/1181263007/#ps140001
(title: "Addressed comments by lazyboy@. Rebased.")

[commit-bot: I haz the power, 2015-06-23 14:15:08]

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

[commit-bot: I haz the power, 2015-06-23 14:48:56]

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

[commit-bot: I haz the power, 2015-06-23 14:48:57]

Exceeded global retry quota

[paulmeyer, 2015-07-02 21:53:16]

Patchset #6 (id:160001) has been deleted

[paulmeyer, 2015-07-03 13:18:14]

rdevlin.cronin@: I changed enough that I think you should take another look over
this. Let me know if you're still okay with it. I changed ExtensionKey back to
the way it was, and now I've implemented a way to reliably get the extension ID
from cleanup so I can clear the context menus normally.

[paulmeyer, 2015-07-03 13:24:08]

Patchset #6 (id:180001) has been deleted

[Devlin, 2015-07-06 17:46:18]

https://codereview.chromium.org/1181263007/diff/200001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
(right):

https://codereview.chromium.org/1181263007/diff/200001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.cc:751:
WebViewGuest::RegisterExtensionID(extension_id(),
This is a pretty bad hack.  This is basically hoping that the extension id is
set correctly on the renderer side, and then using that getting forwarded to the
browser.  The renderer side, in turn, actually just uses the url of the frame
that sent the message.  Relying on this massive chain to get to here strikes me
as fragile, and, frankly, rather slow.  If we want to rely on URL for the
extension id (which is actually okay, for the most part - we do it in a lot of
places), surely a web view knows its own url, without having to do round-trips
to/from the renderer?

https://codereview.chromium.org/1181263007/diff/200001/extensions/browser/api...
File extensions/browser/api/guest_view/web_view/web_view_internal_api.h (right):

https://codereview.chromium.org/1181263007/diff/200001/extensions/browser/api...
extensions/browser/api/guest_view/web_view/web_view_internal_api.h:371: bool
RunAsync() final;
See my comment in the .cc for why this is moot, but for future changes, please
*strongly* prefer using UIThreadExtensionFunction::Run() over RunAsync().  The
terminology is far more clear.  Also, the histogram enum (in
DECLARE_EXTENSION_FUNCTION) should match the function, so SETAUTOSIZE here is
wrong.

https://codereview.chromium.org/1181263007/diff/200001/extensions/browser/gue...
File extensions/browser/guest_view/web_view/web_view_guest.cc (right):

https://codereview.chromium.org/1181263007/diff/200001/extensions/browser/gue...
extensions/browser/guest_view/web_view/web_view_guest.cc:201: static
base::LazyInstance<WebViewKeyToExtensionIDMap>
Storing this as global state seems unnecessary (and, as with most global state,
undesirable).  Why not just store it on the WebViewGuest?

[paulmeyer, 2015-07-08 19:26:06]

Patchset #7 (id:220001) has been deleted

[paulmeyer, 2015-07-08 19:58:22]

The CQ bit was checked by paulmeyer@chromium.org to run a CQ dry run

[paulmeyer, 2015-07-08 19:58:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, rdevlin.cronin@chromium.org, lazyboy@chromium.org
Link to the patchset: https://codereview.chromium.org/1181263007/#ps240001
(title: "Made changes discussed with rdevlin.cronin@ offline.")

[commit-bot: I haz the power, 2015-07-08 19:58:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1181263007/240001

[paulmeyer, 2015-07-08 19:59:13]

rdevlin.cronin@: ptal

[Devlin, 2015-07-08 20:31:01]

Biggest concern is the commented dcheck.

https://codereview.chromium.org/1181263007/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/1181263007/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:352: //DCHECK_EQ(extension->id(),
key.extension_id);
?

https://codereview.chromium.org/1181263007/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:523: std::string extension_id =
it->first.extension_id;
comment why we need to use the item's extension_id

https://codereview.chromium.org/1181263007/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/embedder.js
(left):

https://codereview.chromium.org/1181263007/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/embedder.js:56:

please avoid changes like this, because it just makes noise in the git history
(and isn't a firm stylistic rule).

[Devlin, 2015-07-08 20:31:08]

The CQ bit was unchecked by rdevlin.cronin@chromium.org

[paulmeyer, 2015-07-08 21:05:52]

Patchset #8 (id:260001) has been deleted

[paulmeyer, 2015-07-08 21:06:55]

rdevlin.cronin@: ptal.

https://codereview.chromium.org/1181263007/diff/240001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/1181263007/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:352: //DCHECK_EQ(extension->id(),
key.extension_id);
On 2015/07/08 20:31:01, Devlin wrote:
> ?

Whoops. I only commented that out to test something. Uncommented.

https://codereview.chromium.org/1181263007/diff/240001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:523: std::string extension_id =
it->first.extension_id;
On 2015/07/08 20:31:01, Devlin wrote:
> comment why we need to use the item's extension_id

Done.

https://codereview.chromium.org/1181263007/diff/240001/chrome/test/data/exten...
File
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/embedder.js
(left):

https://codereview.chromium.org/1181263007/diff/240001/chrome/test/data/exten...
chrome/test/data/extensions/platform_apps/web_view/context_menus/basic/embedder.js:56:

On 2015/07/08 20:31:01, Devlin wrote:
> please avoid changes like this, because it just makes noise in the git history
> (and isn't a firm stylistic rule).

Done.

[Devlin, 2015-07-08 21:10:01]

lgtm

https://codereview.chromium.org/1181263007/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/1181263007/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:525: // webview IDs).
// TODO(paulmeyer): We can get rid of this hack if/when we track extension ids
at guest cleanup.

Just want to make it clear that, while not that bad, this still isn't ideal. :)

[paulmeyer, 2015-07-09 14:14:58]

https://codereview.chromium.org/1181263007/diff/280001/chrome/browser/extensi...
File chrome/browser/extensions/menu_manager.cc (right):

https://codereview.chromium.org/1181263007/diff/280001/chrome/browser/extensi...
chrome/browser/extensions/menu_manager.cc:525: // webview IDs).
On 2015/07/08 21:10:01, Devlin wrote:
> // TODO(paulmeyer): We can get rid of this hack if/when we track extension ids
> at guest cleanup.
> 
> Just want to make it clear that, while not that bad, this still isn't ideal.
:)

Done.

[paulmeyer, 2015-07-09 14:15:09]

The CQ bit was checked by paulmeyer@chromium.org

[paulmeyer, 2015-07-09 14:15:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
fsamuel@chromium.org, lazyboy@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1181263007/#ps300001
(title: "Added comment.")

[commit-bot: I haz the power, 2015-07-09 14:16:03]

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

[commit-bot: I haz the power, 2015-07-09 16:05:35]

Committed patchset #9 (id:300001)

[commit-bot: I haz the power, 2015-07-09 16:06:27]

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