Refactor the implementation of the webNavigation extension API.

extensions/browser/extension_api_frame_id_map.cc

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "extensions/browser/extension_api_frame_id_map.h"
 
 #include <tuple>
 
 #include "content/public/browser/browser_thread.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/common/child_process_host.h"
 
 namespace extensions {
 
 namespace {
 
 // The map is accessed on the IO and UI thread, so construct it once and never
@@ skipping 50 matching lines @@
 
 int ExtensionApiFrameIdMap::GetFrameId(content::RenderFrameHost* rfh) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   if (!rfh)
     return kInvalidFrameId;
   if (rfh->GetParent())
     return rfh->GetFrameTreeNodeId();
   return 0;  // Main frame.
 }
 

[Devlin, 2016/02/09 17:35:40]

nit: add // static (if you could also do this for existing static methods,
that'd be great).

[nasko, 2016/02/09 17:53:54]

Done.

+int ExtensionApiFrameIdMap::GetFrameId(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);

[Devlin, 2016/02/09 17:35:40]

Do these actually need to be done on the UI thread?  I mean, NavigationHandle is
a UI thread concept, but if this is the only place we're checking it, we have
bigger problems. :)  I'd prefer this class to only deal with checking for it's
own internal state (i.e., the map).

[nasko, 2016/02/09 17:53:54]

All the other Get* methods that access RFH also do the UI check, so I added it
to be consistent. If you want, I can remove it from all of them, but I'd rather
we be consistent - all have it or none.

[Devlin, 2016/02/09 18:01:15]

Let's remove it for all static methods, where by definition there's no internal
state to be accessed.

+
+  return navigation_handle->IsInMainFrame()
+             ? 0
+             : navigation_handle->GetFrameTreeNodeId();
+}
+
 int ExtensionApiFrameIdMap::GetParentFrameId(content::RenderFrameHost* rfh) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   return rfh ? GetFrameId(rfh->GetParent()) : kInvalidFrameId;
 }
 
+int ExtensionApiFrameIdMap::GetParentFrameId(
+    content::NavigationHandle* navigation_handle) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  if (navigation_handle->IsInMainFrame())
+    return -1;
+
+  if (navigation_handle->IsParentMainFrame())
+    return 0;
+
+  return navigation_handle->GetParentFrameTreeNodeId();
+}
+
 content::RenderFrameHost* ExtensionApiFrameIdMap::GetRenderFrameHostById(
     content::WebContents* web_contents,
     int frame_id) {
   // Although it is technically possible to map |frame_id| to a RenderFrameHost
   // without WebContents, we choose to not do that because in the extension API
   // frameIds are only guaranteed to be meaningful in combination with a tabId.
   if (!web_contents)
     return nullptr;
 
   if (frame_id == kInvalidFrameId)
@@ skipping 139 matching lines @@
 }
 
 void ExtensionApiFrameIdMap::RemoveFrameId(const RenderFrameIdKey& key) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
   base::AutoLock lock(frame_id_map_lock_);
   frame_id_map_.erase(key);
 }
 
 }  // namespace extensions