Extensions: Cleanups related to ExtensionAPIFrameIdMap and ExtensionNavigationUIData.

This CL does the following cleanups in files related to ExtensionAPIFrameIdMap
and ExtensionNavigationUIData:
  - Consolidate code to populate tab data as a static method in
    ChromeExtensionAPIFrameIdMapHelper.
  - Use extension_misc::kUnknownTabId and extension_misc::kUnknownWindowId in
    place of hard-coded constants.
  - Rename ExtensionAPIFrameIdMap::[Cache/Remove]FrameData to
    ExtensionAPIFrameIdMap::OnRenderFrame[Created/Deleted]. This should help us
    reason about the lifetime of RenderFrameHosts as seen in
    ExtensionAPIFrameIdMap.
  - Rename ExtensionApiFrameIdMapHelper::GetTabAndWindowId to PopulateTabData.
  - Some other small cleanups (headers, make_unique etc.)

This CL should have no behavioral changes.

BUG=None

Change-Id: I1ff9aab92aab4d69f5fae511be6fb072e2b70107
Reviewed-on: https://chromium-review.googlesource.com/936322
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Istiaque Ahmed <lazyboy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540375}

chrome/browser/extensions/chrome_extension_api_frame_id_map_helper.cc

@@ -9,6 +9,7 @@
 #include "chrome/browser/sessions/session_tab_helper.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/web_contents.h"
+#include "extensions/common/constants.h"
 
 namespace extensions {
 
@@ -19,20 +20,33 @@ ChromeExtensionApiFrameIdMapHelper::ChromeExtensionApiFrameIdMapHelper(
                  content::NotificationService::AllBrowserContextsAndSources());
 }
 
-ChromeExtensionApiFrameIdMapHelper::~ChromeExtensionApiFrameIdMapHelper() {}
+ChromeExtensionApiFrameIdMapHelper::~ChromeExtensionApiFrameIdMapHelper() =
+    default;
 
-void ChromeExtensionApiFrameIdMapHelper::GetTabAndWindowId(
-    content::RenderFrameHost* rfh,
+// static
+bool ChromeExtensionApiFrameIdMapHelper::PopulateTabData(
+    content::WebContents* web_contents,
     int* tab_id,
     int* window_id) {
-  content::WebContents* web_contents =
-      content::WebContents::FromRenderFrameHost(rfh);
+  if (!web_contents)
+    return false;
+
   SessionTabHelper* session_tab_helper =
-      web_contents ? SessionTabHelper::FromWebContents(web_contents) : nullptr;
-  if (session_tab_helper) {
+      SessionTabHelper::FromWebContents(web_contents);
+  if (!session_tab_helper)
+    return false;
+
   *tab_id = session_tab_helper->session_id().id();
   *window_id = session_tab_helper->window_id().id();
-  }
+  return true;
+}
+
+void ChromeExtensionApiFrameIdMapHelper::PopulateTabData(
+    content::RenderFrameHost* rfh,
+    int* tab_id,
+    int* window_id) {
+  PopulateTabData(content::WebContents::FromRenderFrameHost(rfh), tab_id,
+                  window_id);
 }
 
 void ChromeExtensionApiFrameIdMapHelper::Observe(
@@ -42,12 +56,12 @@ void ChromeExtensionApiFrameIdMapHelper::Observe(
   DCHECK_EQ(chrome::NOTIFICATION_TAB_PARENTED, type);
   content::WebContents* web_contents =
       content::Source<content::WebContents>(source).ptr();
-  SessionTabHelper* session_tab_helper =
-      web_contents ? SessionTabHelper::FromWebContents(web_contents) : nullptr;
-  if (!session_tab_helper)
+
+  int tab_id = extension_misc::kUnknownTabId;
+  int window_id = extension_misc::kUnknownWindowId;
+  if (!PopulateTabData(web_contents, &tab_id, &window_id))
     return;
-  int tab_id = session_tab_helper->session_id().id();
-  int window_id = session_tab_helper->window_id().id();
+
   web_contents->ForEachFrame(
       base::BindRepeating(&ExtensionApiFrameIdMap::UpdateTabAndWindowId,
                           base::Unretained(owner_), tab_id, window_id));

chrome/browser/extensions/chrome_extension_api_frame_id_map_helper.h

@@ -10,6 +10,10 @@
 #include "content/public/browser/notification_registrar.h"
 #include "extensions/browser/extension_api_frame_id_map.h"
 
+namespace content {
+class WebContents;
+}  // namespace content
+
 namespace extensions {
 
 class ChromeExtensionApiFrameIdMapHelper
@@ -19,9 +23,15 @@ class ChromeExtensionApiFrameIdMapHelper
   explicit ChromeExtensionApiFrameIdMapHelper(ExtensionApiFrameIdMap* owner);
   ~ChromeExtensionApiFrameIdMapHelper() override;
 
+  // Populates the |tab_id| and |window_id| for the given |web_contents|.
+  // Returns true on success.
+  static bool PopulateTabData(content::WebContents* web_contents,
+                              int* tab_id,
+                              int* window_id);
+
  private:
   // ChromeExtensionApiFrameIdMapHelper:
-  void GetTabAndWindowId(content::RenderFrameHost* rfh,
+  void PopulateTabData(content::RenderFrameHost* rfh,
                        int* tab_id_out,
                        int* window_id_out) override;
 

chrome/browser/renderer_host/chrome_navigation_ui_data.cc

@@ -4,29 +4,16 @@
 
 #include "chrome/browser/renderer_host/chrome_navigation_ui_data.h"
 
-#include <tuple>
-
-#include "base/memory/ptr_util.h"
 #include "chrome/browser/prerender/prerender_contents.h"
 #include "chrome/browser/prerender/prerender_histograms.h"
-#include "chrome/browser/sessions/session_tab_helper.h"
 #include "content/public/browser/navigation_handle.h"
 #include "extensions/features/features.h"
 #include "ui/base/window_open_disposition.h"
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-namespace {
-std::pair<int, int> GetTabIdAndWindowId(content::WebContents* web_contents) {
-  SessionTabHelper* session_tab_helper =
-      SessionTabHelper::FromWebContents(web_contents);
-  if (!session_tab_helper)
-    return std::make_pair(-1, -1);
-
-  return std::make_pair(session_tab_helper->session_id().id(),
-                        session_tab_helper->window_id().id());
-}
-}  // namespace
-#endif  // BUILDFLAG(ENABLE_EXTENSIONS)
+#include "chrome/browser/extensions/chrome_extension_api_frame_id_map_helper.h"
+#include "extensions/common/constants.h"
+#endif
 
 ChromeNavigationUIData::ChromeNavigationUIData()
     : disposition_(WindowOpenDisposition::CURRENT_TAB) {}
@@ -36,9 +23,10 @@ ChromeNavigationUIData::ChromeNavigationUIData(
     : disposition_(WindowOpenDisposition::CURRENT_TAB) {
   auto* web_contents = navigation_handle->GetWebContents();
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  int tab_id;
-  int window_id;
-  std::tie(tab_id, window_id) = GetTabIdAndWindowId(web_contents);
+  int tab_id = extension_misc::kUnknownTabId;
+  int window_id = extension_misc::kUnknownWindowId;
+  extensions::ChromeExtensionApiFrameIdMapHelper::PopulateTabData(
+      web_contents, &tab_id, &window_id);
   extension_data_ = std::make_unique<extensions::ExtensionNavigationUIData>(
       navigation_handle, tab_id, window_id);
 #endif
@@ -64,9 +52,10 @@ ChromeNavigationUIData::CreateForMainFrameNavigation(
   navigation_ui_data->disposition_ = disposition;
 
 #if BUILDFLAG(ENABLE_EXTENSIONS)
-  int tab_id;
-  int window_id;
-  std::tie(tab_id, window_id) = GetTabIdAndWindowId(web_contents);
+  int tab_id = extension_misc::kUnknownTabId;
+  int window_id = extension_misc::kUnknownWindowId;
+  extensions::ChromeExtensionApiFrameIdMapHelper::PopulateTabData(
+      web_contents, &tab_id, &window_id);
 
   navigation_ui_data->extension_data_ =
       extensions::ExtensionNavigationUIData::CreateForMainFrameNavigation(
@@ -78,8 +67,7 @@ ChromeNavigationUIData::CreateForMainFrameNavigation(
 
 std::unique_ptr<content::NavigationUIData> ChromeNavigationUIData::Clone()
     const {
-  std::unique_ptr<ChromeNavigationUIData> copy =
-      std::make_unique<ChromeNavigationUIData>();
+  auto copy = std::make_unique<ChromeNavigationUIData>();
 
   copy->disposition_ = disposition_;
 

extensions/browser/extension_api_frame_id_map.cc

@@ -6,6 +6,7 @@
 
 #include <tuple>
 
+#include "base/logging.h"
 #include "base/metrics/histogram_macros.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_handle.h"
@@ -14,6 +15,7 @@
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/child_process_host.h"
 #include "extensions/browser/extensions_browser_client.h"
+#include "extensions/common/constants.h"
 
 namespace extensions {
 
@@ -39,8 +41,8 @@ const int ExtensionApiFrameIdMap::kTopFrameId = 0;
 ExtensionApiFrameIdMap::FrameData::FrameData()
     : frame_id(kInvalidFrameId),
       parent_frame_id(kInvalidFrameId),
-      tab_id(-1),
-      window_id(-1) {}
+      tab_id(extension_misc::kUnknownTabId),
+      window_id(extension_misc::kUnknownWindowId) {}
 
 ExtensionApiFrameIdMap::FrameData::FrameData(int frame_id,
                                              int parent_frame_id,
@@ -160,10 +162,10 @@ ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::KeyToValue(
     const RenderFrameIdKey& key) const {
   content::RenderFrameHost* rfh = content::RenderFrameHost::FromID(
       key.render_process_id, key.frame_routing_id);
-  int tab_id = -1;
-  int window_id = -1;
+  int tab_id = extension_misc::kUnknownTabId;
+  int window_id = extension_misc::kUnknownWindowId;
   if (helper_)
-    helper_->GetTabAndWindowId(rfh, &tab_id, &window_id);
+    helper_->PopulateTabData(rfh, &tab_id, &window_id);
   return FrameData(GetFrameId(rfh), GetParentFrameId(rfh), tab_id, window_id);
 }
 
@@ -314,7 +316,8 @@ ExtensionApiFrameIdMap::FrameData ExtensionApiFrameIdMap::GetFrameData(
   return LookupFrameDataOnUI(key, false /* is_from_io */);
 }
 
-void ExtensionApiFrameIdMap::CacheFrameData(content::RenderFrameHost* rfh) {
+void ExtensionApiFrameIdMap::OnRenderFrameCreated(
+    content::RenderFrameHost* rfh) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   const RenderFrameIdKey key(rfh->GetProcess()->GetID(), rfh->GetRoutingID());
@@ -326,7 +329,8 @@ void ExtensionApiFrameIdMap::CacheFrameData(const RenderFrameIdKey& key) {
   LookupFrameDataOnUI(key, false /* is_from_io */);
 }
 
-void ExtensionApiFrameIdMap::RemoveFrameData(content::RenderFrameHost* rfh) {
+void ExtensionApiFrameIdMap::OnRenderFrameDeleted(
+    content::RenderFrameHost* rfh) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
   DCHECK(rfh);
 
@@ -347,6 +351,8 @@ void ExtensionApiFrameIdMap::UpdateTabAndWindowId(
     iter->second.tab_id = tab_id;
     iter->second.window_id = window_id;
   } else {
+    // TODO(crbug.com/817075): Remove this branch, after ensuring
+    // OnRenderFrameCreated is called for each frame.
     frame_data_map_[key] =
         FrameData(GetFrameId(rfh), GetParentFrameId(rfh), tab_id, window_id);
   }

extensions/browser/extension_api_frame_id_map.h

@@ -24,7 +24,7 @@ namespace extensions {
 
 class ExtensionApiFrameIdMapHelper {
  public:
-  virtual void GetTabAndWindowId(content::RenderFrameHost* rfh,
+  virtual void PopulateTabData(content::RenderFrameHost* rfh,
                                int* tab_id_out,
                                int* window_id_out) = 0;
   virtual ~ExtensionApiFrameIdMapHelper() {}
@@ -124,16 +124,13 @@ class ExtensionApiFrameIdMap {
   // If |rfh| is nullptr, then the map is not modified.
   FrameData GetFrameData(content::RenderFrameHost* rfh) WARN_UNUSED_RESULT;
 
-  // Looks up the FrameData and stores it in the map. This method should be
-  // called as early as possible, e.g. in a
-  // WebContentsObserver::RenderFrameCreated notification.
-  void CacheFrameData(content::RenderFrameHost* rfh);
+  // Called when a render frame is created. Caches the FrameData for the given
+  // render frame.
+  void OnRenderFrameCreated(content::RenderFrameHost* rfh);
 
-  // Removes the FrameData mapping for a given frame. This method can be called
-  // at any time, but it is typically called when a frame is destroyed.
-  // If this method is not called, the cached mapping for the frame is retained
-  // forever.
-  void RemoveFrameData(content::RenderFrameHost* rfh);
+  // Called when a render frame is deleted. Removes the FrameData mapping for
+  // the given render frame.
+  void OnRenderFrameDeleted(content::RenderFrameHost* rfh);
 
   // Updates the tab and window id for the given RenderFrameHost, if any exists.
   void UpdateTabAndWindowId(int tab_id,

extensions/browser/extension_navigation_ui_data.cc

@@ -4,6 +4,7 @@
 
 #include "extensions/browser/extension_navigation_ui_data.h"
 
+#include "base/memory/ptr_util.h"
 #include "content/public/browser/navigation_handle.h"
 #include "extensions/browser/guest_view/web_view/web_view_guest.h"
 
@@ -49,8 +50,7 @@ ExtensionNavigationUIData::CreateForMainFrameNavigation(
 
 std::unique_ptr<ExtensionNavigationUIData> ExtensionNavigationUIData::DeepCopy()
     const {
-  std::unique_ptr<ExtensionNavigationUIData> copy(
-      new ExtensionNavigationUIData());
+  auto copy = std::make_unique<ExtensionNavigationUIData>();
   copy->frame_data_ = frame_data_;
   copy->is_web_view_ = is_web_view_;
   copy->web_view_instance_id_ = web_view_instance_id_;
@@ -63,12 +63,8 @@ ExtensionNavigationUIData::ExtensionNavigationUIData(
     int tab_id,
     int window_id,
     int frame_id,
-    int parent_frame_id) {
-  frame_data_.window_id = window_id;
-  frame_data_.tab_id = tab_id;
-  frame_data_.frame_id = frame_id;
-  frame_data_.parent_frame_id = parent_frame_id;
-
+    int parent_frame_id)
+    : frame_data_(frame_id, parent_frame_id, tab_id, window_id) {
   WebViewGuest* web_view = WebViewGuest::FromWebContents(web_contents);
   if (web_view) {
     is_web_view_ = true;

extensions/browser/extension_web_contents_observer.cc

@@ -91,7 +91,7 @@ void ExtensionWebContentsObserver::RenderFrameCreated(
   // Optimization: Look up the extension API frame ID to force the mapping to be
   // cached. This minimizes the number of IO->UI->IO thread hops when the ID is
   // looked up again on the IO thread for the webRequest API.
-  ExtensionApiFrameIdMap::Get()->CacheFrameData(render_frame_host);
+  ExtensionApiFrameIdMap::Get()->OnRenderFrameCreated(render_frame_host);
 
   const Extension* extension = GetExtensionFromFrame(render_frame_host, false);
   if (!extension)
@@ -133,7 +133,7 @@ void ExtensionWebContentsObserver::RenderFrameDeleted(
     content::RenderFrameHost* render_frame_host) {
   ProcessManager::Get(browser_context_)
       ->UnregisterRenderFrameHost(render_frame_host);
-  ExtensionApiFrameIdMap::Get()->RemoveFrameData(render_frame_host);
+  ExtensionApiFrameIdMap::Get()->OnRenderFrameDeleted(render_frame_host);
 }
 
 void ExtensionWebContentsObserver::DidFinishNavigation(

extensions/shell/browser/shell_navigation_ui_data.cc

@@ -4,8 +4,8 @@
 
 #include "extensions/shell/browser/shell_navigation_ui_data.h"
 
-#include "base/memory/ptr_util.h"
 #include "content/public/browser/navigation_handle.h"
+#include "extensions/common/constants.h"
 
 namespace extensions {
 
@@ -13,8 +13,9 @@ ShellNavigationUIData::ShellNavigationUIData() {}
 
 ShellNavigationUIData::ShellNavigationUIData(
     content::NavigationHandle* navigation_handle) {
-  extension_data_ =
-      std::make_unique<ExtensionNavigationUIData>(navigation_handle, -1, -1);
+  extension_data_ = std::make_unique<ExtensionNavigationUIData>(
+      navigation_handle, extension_misc::kUnknownTabId,
+      extension_misc::kUnknownWindowId);
 }
 
 ShellNavigationUIData::~ShellNavigationUIData() {}