WebView context menu cleanup.

chrome/browser/extensions/menu_manager.cc

Index: chrome/browser/extensions/menu_manager.cc
diff --git a/chrome/browser/extensions/menu_manager.cc b/chrome/browser/extensions/menu_manager.cc
index 3b50dad5c02436dc8e3b95c0fc2d8ba71a4913f5..654763ddc6e56f34ed3e1c44e0b14cec23994cda 100644
--- a/chrome/browser/extensions/menu_manager.cc
+++ b/chrome/browser/extensions/menu_manager.cc
@@ -23,6 +23,7 @@
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_source.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/child_process_host.h"
 #include "content/public/common/context_menu_params.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_registry.h"
@@ -33,7 +34,9 @@
 #include "ui/gfx/favicon_size.h"
 #include "ui/gfx/text_elider.h"
 
+using content::ChildProcessHost;
 using content::WebContents;
+using guest_view::kInstanceIDNone;
 
 namespace extensions {
 
@@ -341,8 +344,8 @@ const MenuItem::List* MenuManager::MenuItems(
 
 bool MenuManager::AddContextItem(const Extension* extension, MenuItem* item) {
   const MenuItem::ExtensionKey& key = item->id().extension_key;
-  // The item must have a non-empty extension id, and not have already been
-  // added.
+
+  // The item must have a non-empty key, and not have already been added.
   if (key.empty() || ContainsKey(items_by_id_, item->id()))
     return false;
 
@@ -513,9 +516,18 @@ bool MenuManager::RemoveContextMenuItem(const MenuItem::Id& id) {
 
 void MenuManager::RemoveAllContextItems(
     const MenuItem::ExtensionKey& extension_key) {
+  auto it = context_items_.find(extension_key);
+  if (it == context_items_.end())
+    return;
+
+  // We use the |extension_id| from the stored ExtensionKey, since the provided
+  // |extension_key| may leave it empty (if matching solely basted on the
+  // webview IDs).

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

// 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]

Done.

+  std::string extension_id = it->first.extension_id;
+  MenuItem::List& context_items_for_key = it->second;
   MenuItem::List::iterator i;
-  for (i = context_items_[extension_key].begin();
-       i != context_items_[extension_key].end();
+  for (i = context_items_for_key.begin();
+       i != context_items_for_key.end();
        ++i) {
     MenuItem* item = *i;
     items_by_id_.erase(item->id());
@@ -527,9 +539,9 @@ void MenuManager::RemoveAllContextItems(
       items_by_id_.erase(*j);
     }
   }
-  STLDeleteElements(&context_items_[extension_key]);
+  STLDeleteElements(&context_items_for_key);
   context_items_.erase(extension_key);
-  icon_manager_.RemoveIcon(extension_key.extension_id);
+  icon_manager_.RemoveIcon(extension_id);
 }
 
 MenuItem* MenuManager::GetItemById(const MenuItem::Id& id) const {
@@ -886,25 +898,52 @@ void MenuManager::RemoveAllIncognitoContextItems() {
     RemoveContextMenuItem(*remove_iter);
 }
 
-MenuItem::ExtensionKey::ExtensionKey() : webview_instance_id(0) {}
+MenuItem::ExtensionKey::ExtensionKey()
+    : webview_embedder_process_id(ChildProcessHost::kInvalidUniqueID),
+      webview_instance_id(kInstanceIDNone) {}
+
+MenuItem::ExtensionKey::ExtensionKey(const std::string& extension_id)
+    : extension_id(extension_id),
+      webview_embedder_process_id(ChildProcessHost::kInvalidUniqueID),
+      webview_instance_id(kInstanceIDNone) {
+  DCHECK(!extension_id.empty());
+}
 
 MenuItem::ExtensionKey::ExtensionKey(const std::string& extension_id,
+                                     int webview_embedder_process_id,
                                      int webview_instance_id)
-    : extension_id(extension_id), webview_instance_id(webview_instance_id) {}
-
-MenuItem::ExtensionKey::ExtensionKey(const std::string& extension_id)
-    : extension_id(extension_id), webview_instance_id(0) {}
+    : extension_id(extension_id),
+      webview_embedder_process_id(webview_embedder_process_id),
+      webview_instance_id(webview_instance_id) {
+  DCHECK(webview_embedder_process_id != ChildProcessHost::kInvalidUniqueID &&
+         webview_instance_id != kInstanceIDNone);
+}
 
 bool MenuItem::ExtensionKey::operator==(const ExtensionKey& other) const {
-  return extension_id == other.extension_id &&
-         webview_instance_id == other.webview_instance_id;
+  bool webview_ids_match = webview_instance_id == other.webview_instance_id &&
+      webview_embedder_process_id == other.webview_embedder_process_id;
+
+  // If either extension ID is empty, then these ExtensionKeys will be matched
+  // only based on the other IDs.
+  if (extension_id.empty() || other.extension_id.empty())
+    return webview_ids_match;
+
+  return extension_id == other.extension_id && webview_ids_match;
 }
 
 bool MenuItem::ExtensionKey::operator<(const ExtensionKey& other) const {
-  if (extension_id != other.extension_id)
-    return extension_id < other.extension_id;
+  if (webview_embedder_process_id != other.webview_embedder_process_id)
+    return webview_embedder_process_id < other.webview_embedder_process_id;
+
+  if (webview_instance_id != other.webview_instance_id)
+    return webview_instance_id < other.webview_instance_id;
+
+  // If either extension ID is empty, then these ExtensionKeys will be compared
+  // only based on the other IDs.
+  if (extension_id.empty() || other.extension_id.empty())
+    return false;
 
-  return webview_instance_id < other.webview_instance_id;
+  return extension_id < other.extension_id;
 }
 
 bool MenuItem::ExtensionKey::operator!=(const ExtensionKey& other) const {
@@ -912,7 +951,9 @@ bool MenuItem::ExtensionKey::operator!=(const ExtensionKey& other) const {
 }
 
 bool MenuItem::ExtensionKey::empty() const {
-  return extension_id.empty() && !webview_instance_id;
+  return extension_id.empty() &&
+      webview_embedder_process_id == ChildProcessHost::kInvalidUniqueID &&
+      webview_instance_id == kInstanceIDNone;
 }
 
 MenuItem::Id::Id() : incognito(false), uid(0) {}