Refactor the implementation of the webNavigation extension API.

chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc

Index: chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
index 14612edb9c2888683a751333efb7b3235f3fa096..1fea323e251da6dd6b83bc48aecc3bf600a8b5ce 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_api_helpers.cc
@@ -16,10 +16,12 @@
 #include "chrome/browser/extensions/extension_tab_util.h"
 #include "chrome/browser/profiles/profile.h"
 #include "chrome/common/extensions/api/web_navigation.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/render_view_host.h"
 #include "content/public/browser/web_contents.h"
+#include "content/public/common/url_constants.h"
 #include "extensions/browser/event_router.h"
 #include "extensions/browser/extension_api_frame_id_map.h"
 #include "extensions/common/event_filtering_info.h"
@@ -66,41 +68,73 @@ int GetFrameId(content::RenderFrameHost* frame_host) {
   return ExtensionApiFrameIdMap::GetFrameId(frame_host);
 }
 
+int GetFrameId(content::NavigationHandle* navigation_handle) {

[Devlin, 2016/02/05 18:30:28]

It's a shame that we duplicate the logic between here and
ExtensionApiFrameIdMap(), and, ideally, I'd prefer only the latter to need to
know that extension api frame ids are the FTN ID *or* 0 for the main frame *or*
-1 for an invalid frame, and so on.  Would it be too weird to add a
Get[Parent]FrameId(content::NavigationHandle*) in ExtensionApiFrameIdMap?

[nasko, 2016/02/05 23:41:22]

Totally makes sense. Done.

+  if (navigation_handle->IsInMainFrame()) {

[Devlin, 2016/02/05 18:30:28]

nit: If we keep this, I'd prefer a ternary here.

[nasko, 2016/02/05 23:41:22]

Done.

+    return 0;
+  }
+  return navigation_handle->GetFrameTreeNodeId();
+}
+
+int GetParentFrameId(content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->IsInMainFrame()) {
+    return -1;
+  }
+  if (navigation_handle->IsParentMainFrame()) {
+    return 0;
+  }
+  return navigation_handle->GetParentFrameTreeNodeId();
+}
+
 // Constructs and dispatches an onBeforeNavigate event.
-void DispatchOnBeforeNavigate(content::WebContents* web_contents,
-                              content::RenderFrameHost* frame_host,
-                              const GURL& validated_url) {
+void DispatchOnBeforeNavigate(content::NavigationHandle* navigation_handle) {
   scoped_ptr<base::ListValue> args(new base::ListValue());
   base::DictionaryValue* dict = new base::DictionaryValue();
-  dict->SetInteger(keys::kTabIdKey, ExtensionTabUtil::GetTabId(web_contents));
-  dict->SetString(keys::kUrlKey, validated_url.spec());
-  dict->SetInteger(keys::kProcessIdKey, frame_host->GetProcess()->GetID());
-  dict->SetInteger(keys::kFrameIdKey, GetFrameId(frame_host));
+
+  GURL url(navigation_handle->GetURL());
+  if (navigation_handle->IsSrcdoc())
+    url = GURL(content::kAboutSrcDocURL);
+
+  dict->SetInteger(keys::kTabIdKey, ExtensionTabUtil::GetTabId(

[Devlin, 2016/02/05 18:30:28]

This is typically prettier when we use
api::web_navigation::OnBeforeNavigate::Details (also for others).

[nasko, 2016/02/05 23:41:22]

You asked for it! :)

+                                        navigation_handle->GetWebContents()));
+  dict->SetString(keys::kUrlKey, url.spec());
+  dict->SetInteger(keys::kProcessIdKey, -1);
+  dict->SetInteger(keys::kFrameIdKey, GetFrameId(navigation_handle));
   dict->SetInteger(keys::kParentFrameIdKey,
-                   GetFrameId(frame_host->GetParent()));
+                   GetParentFrameId(navigation_handle));
   dict->SetDouble(keys::kTimeStampKey, MilliSecondsFromTime(base::Time::Now()));
   args->Append(dict);
-
-  DispatchEvent(web_contents->GetBrowserContext(),
+  DispatchEvent(navigation_handle->GetWebContents()->GetBrowserContext(),
                 events::WEB_NAVIGATION_ON_BEFORE_NAVIGATE,
                 web_navigation::OnBeforeNavigate::kEventName, std::move(args),
-                validated_url);
+                url);
 }
 
 // Constructs and dispatches an onCommitted or onReferenceFragmentUpdated
 // event.
 void DispatchOnCommitted(events::HistogramValue histogram_value,
                          const std::string& event_name,
-                         content::WebContents* web_contents,
-                         content::RenderFrameHost* frame_host,
-                         const GURL& url,
-                         ui::PageTransition transition_type) {
+                         content::NavigationHandle* navigation_handle) {
+  content::WebContents* web_contents = navigation_handle->GetWebContents();
+  GURL url(navigation_handle->GetURL());
+  content::RenderFrameHost* frame_host =
+      navigation_handle->GetRenderFrameHost();
+  ui::PageTransition transition_type = navigation_handle->GetPageTransition();
+
+  if (navigation_handle->IsSrcdoc())
+    url = GURL(content::kAboutSrcDocURL);
+
   scoped_ptr<base::ListValue> args(new base::ListValue());
   base::DictionaryValue* dict = new base::DictionaryValue();
   dict->SetInteger(keys::kTabIdKey, ExtensionTabUtil::GetTabId(web_contents));
   dict->SetString(keys::kUrlKey, url.spec());
   dict->SetInteger(keys::kProcessIdKey, frame_host->GetProcess()->GetID());
   dict->SetInteger(keys::kFrameIdKey, GetFrameId(frame_host));
+
+  if (navigation_handle->WasServerRedirect()) {
+    transition_type = ui::PageTransitionFromInt(

[Devlin, 2016/02/05 18:30:28]

Out of curiosity, why isn't this done as part of the transition type that
NavigationHandle::GetPageTransition() returns?

[nasko, 2016/02/05 23:41:22]

Not sure. I'd rather not touch that though :), or at least not part of this CL.

+        transition_type | ui::PAGE_TRANSITION_SERVER_REDIRECT);
+  }
+
   std::string transition_type_string =
       ui::PageTransitionGetCoreTransitionString(transition_type);
   // For webNavigation API backward compatibility, keep "start_page" even after
@@ -220,6 +254,30 @@ void DispatchOnErrorOccurred(content::WebContents* web_contents,
                 url);
 }
 
+void DispatchOnErrorOccurred(content::NavigationHandle* navigation_handle) {
+  scoped_ptr<base::ListValue> args(new base::ListValue());
+  base::DictionaryValue* dict = new base::DictionaryValue();
+
+  dict->SetInteger(keys::kTabIdKey, ExtensionTabUtil::GetTabId(
+                                        navigation_handle->GetWebContents()));
+  dict->SetString(keys::kUrlKey, navigation_handle->GetURL().spec());
+  dict->SetInteger(keys::kProcessIdKey, -1);
+  dict->SetInteger(keys::kFrameIdKey, GetFrameId(navigation_handle));
+  if (navigation_handle->GetNetErrorCode() != net::OK) {
+    dict->SetString(keys::kErrorKey,
+                    net::ErrorToString(navigation_handle->GetNetErrorCode()));
+  } else {
+    dict->SetString(keys::kErrorKey, net::ErrorToString(net::ERR_ABORTED));
+  }
+  dict->SetDouble(keys::kTimeStampKey, MilliSecondsFromTime(base::Time::Now()));
+  args->Append(dict);
+
+  DispatchEvent(navigation_handle->GetWebContents()->GetBrowserContext(),
+                events::WEB_NAVIGATION_ON_ERROR_OCCURRED,
+                web_navigation::OnErrorOccurred::kEventName, std::move(args),
+                navigation_handle->GetURL());
+}
+
 // Constructs and dispatches an onTabReplaced event.
 void DispatchOnTabReplaced(
     content::WebContents* old_web_contents,