Refactor the implementation of the webNavigation extension API.

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

Index: chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
diff --git a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
index 21aa3dd7c9d4d1f06633e93159a5969e4644b9e6..f6b90647b63e227e1120678721b31b38ab2cc0f9 100644
--- a/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
+++ b/chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
@@ -17,6 +17,7 @@
 #include "chrome/browser/ui/browser_list.h"
 #include "chrome/common/extensions/api/web_navigation.h"
 #include "content/public/browser/navigation_details.h"
+#include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/notification_service.h"
 #include "content/public/browser/notification_types.h"
 #include "content/public/browser/render_frame_host.h"
@@ -266,95 +267,35 @@ void WebNavigationTabObserver::RenderFrameHostChanged(
   navigation_state_.FrameHostCreated(new_host);
 }
 
-void WebNavigationTabObserver::DidStartProvisionalLoadForFrame(
-    content::RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    bool is_error_page,
-    bool is_iframe_srcdoc) {
-  DVLOG(2) << "DidStartProvisionalLoad("
-           << "render_frame_host=" << render_frame_host
-           << ", frame_num=" << render_frame_host->GetRoutingID()
-           << ", url=" << validated_url << ")";
-  navigation_state_.StartTrackingNavigation(render_frame_host, validated_url,
-                                            is_error_page, is_iframe_srcdoc);
-
-  if (!navigation_state_.CanSendEvents(render_frame_host))
+void WebNavigationTabObserver::DidStartNavigation(
+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->IsSynchronousNavigation())
     return;
 
-  helpers::DispatchOnBeforeNavigate(
-      web_contents(),
-      render_frame_host,
-      navigation_state_.GetUrl(render_frame_host));
-}
-
-void WebNavigationTabObserver::DidCommitProvisionalLoadForFrame(
-    content::RenderFrameHost* render_frame_host,
-    const GURL& url,
-    ui::PageTransition transition_type) {
-  DVLOG(2) << "DidCommitProvisionalLoad("
-           << "render_frame_host=" << render_frame_host
-           << ", frame_num=" << render_frame_host->GetRoutingID()
-           << ", url=" << url << ")";
-  bool is_reference_fragment_navigation =
-      IsReferenceFragmentNavigation(render_frame_host, url);
-  bool is_history_state_modification =
-      navigation_state_.GetNavigationCommitted(render_frame_host);
-
-  // Update the URL as it might have changed.
-  navigation_state_.UpdateFrame(render_frame_host, url);
-  navigation_state_.SetNavigationCommitted(render_frame_host);
-
-  if (!navigation_state_.CanSendEvents(render_frame_host))
+  if (!FrameNavigationState::IsValidUrl(navigation_handle->GetURL()))
     return;
 
-  events::HistogramValue histogram_value = events::UNKNOWN;
-  std::string event_name;
-  if (is_reference_fragment_navigation) {
-    histogram_value = events::WEB_NAVIGATION_ON_REFERENCE_FRAGMENT_UPDATED;
-    event_name = web_navigation::OnReferenceFragmentUpdated::kEventName;
-  } else if (is_history_state_modification) {
-    histogram_value = events::WEB_NAVIGATION_ON_HISTORY_STATE_UPDATED;
-    event_name = web_navigation::OnHistoryStateUpdated::kEventName;
-  } else {
-    if (navigation_state_.GetIsServerRedirected(render_frame_host)) {
-      transition_type = ui::PageTransitionFromInt(
-          transition_type | ui::PAGE_TRANSITION_SERVER_REDIRECT);
-    }
-    histogram_value = events::WEB_NAVIGATION_ON_COMMITTED;
-    event_name = web_navigation::OnCommitted::kEventName;
-  }
-  helpers::DispatchOnCommitted(
-      histogram_value, event_name, web_contents(), render_frame_host,
-      navigation_state_.GetUrl(render_frame_host), transition_type);
+  helpers::DispatchOnBeforeNavigate(navigation_handle);
 }
 
-void WebNavigationTabObserver::DidFailProvisionalLoad(
-    content::RenderFrameHost* render_frame_host,
-    const GURL& validated_url,
-    int error_code,
-    const base::string16& error_description,
-    bool was_ignored_by_handler) {
-  DVLOG(2) << "DidFailProvisionalLoad("
-           << "render_frame_host=" << render_frame_host
-           << ", frame_num=" << render_frame_host->GetRoutingID()
-           << ", url=" << validated_url << ")";
-  if (navigation_state_.CanSendEvents(render_frame_host)) {
-    helpers::DispatchOnErrorOccurred(
-        web_contents(),
-        render_frame_host,
-        navigation_state_.GetUrl(render_frame_host),
-        error_code);
+void WebNavigationTabObserver::DidFinishNavigation(

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

I'm sure it's just that I'm not as familiar with all these notifications, but
the flow of:
DidFinishNavigation -> StartTrackingDocumentLoad (which sets is_loading to true
on the navigation frame state - indicating "there is a navigation going on")
seems... odd.

I'm not really sure there's anything much to be done, but maybe we could add a
super short comment here, and/or rephrase the comment for is_loading?

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

I rephrased the comment on is_loading, which I missed initially.

+    content::NavigationHandle* navigation_handle) {
+  if (navigation_handle->HasCommitted()) {
+    if (navigation_handle->IsErrorPage()) {
+      HandleError(navigation_handle);

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

HandleError vs DispatchOnErrorOccurred is a little weird, especially when
HandleError() just starts tracking the document load and then calls
DispatchOnErrorOccurred().

Maybe this would be cleaner as

bool did_commit = handle->HasCommitted();
bool is_error = !did_commit || handle->IsErrorPage();
if (is_error) {
  if (did_commit)
    StartTrackingDocumentLoad();
  DispatchOnErrorOccurred();
} else {
  HandleCommit();
}

or:
if (!handle->HasCommitted() || handle->IsErrorPage())
  HandleError();  // Add check for HasCommitted to HandleError()
else
  HandleCommit();

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

Done.

+    } else {
+      HandleCommit(navigation_handle);
+    }
+  } else {
+    helpers::DispatchOnErrorOccurred(navigation_handle);
   }
-  navigation_state_.SetErrorOccurredInFrame(render_frame_host);
 }
 
 void WebNavigationTabObserver::DocumentLoadedInFrame(
     content::RenderFrameHost* render_frame_host) {
-  DVLOG(2) << "DocumentLoadedInFrame("
-           << "render_frame_host=" << render_frame_host
-           << ", frame_num=" << render_frame_host->GetRoutingID() << ")";
   if (!navigation_state_.CanSendEvents(render_frame_host))
     return;
+
   navigation_state_.SetParsingFinished(render_frame_host);
   helpers::DispatchOnDOMContentLoaded(
       web_contents(),
@@ -375,14 +316,11 @@ void WebNavigationTabObserver::DocumentLoadedInFrame(
 void WebNavigationTabObserver::DidFinishLoad(
     content::RenderFrameHost* render_frame_host,
     const GURL& validated_url) {
-  DVLOG(2) << "DidFinishLoad("
-           << "render_frame_host=" << render_frame_host
-           << ", frame_num=" << render_frame_host->GetRoutingID()
-           << ", url=" << validated_url << ")";
   // When showing replacement content, we might get load signals for frames
   // that weren't reguarly loaded.
   if (!navigation_state_.IsValidFrame(render_frame_host))
     return;
+
   navigation_state_.SetNavigationCompleted(render_frame_host);
   if (!navigation_state_.CanSendEvents(render_frame_host))
     return;
@@ -413,14 +351,11 @@ void WebNavigationTabObserver::DidFailLoad(
     int error_code,
     const base::string16& error_description,
     bool was_ignored_by_handler) {
-  DVLOG(2) << "DidFailLoad("
-           << "render_frame_host=" << render_frame_host
-           << ", frame_num=" << render_frame_host->GetRoutingID()
-           << ", url=" << validated_url << ")";
   // When showing replacement content, we might get load signals for frames
   // that weren't reguarly loaded.
   if (!navigation_state_.IsValidFrame(render_frame_host))
     return;
+
   if (navigation_state_.CanSendEvents(render_frame_host)) {
     helpers::DispatchOnErrorOccurred(
         web_contents(),
@@ -431,16 +366,6 @@ void WebNavigationTabObserver::DidFailLoad(
   navigation_state_.SetErrorOccurredInFrame(render_frame_host);
 }
 
-void WebNavigationTabObserver::DidGetRedirectForResourceRequest(
-    content::RenderFrameHost* render_frame_host,
-    const content::ResourceRedirectDetails& details) {
-  if (details.resource_type != content::RESOURCE_TYPE_MAIN_FRAME &&
-      details.resource_type != content::RESOURCE_TYPE_SUB_FRAME) {
-    return;
-  }
-  navigation_state_.SetIsServerRedirected(render_frame_host);
-}
-
 void WebNavigationTabObserver::DidOpenRequestedURL(
     content::WebContents* new_contents,
     content::RenderFrameHost* source_render_frame_host,
@@ -473,6 +398,43 @@ void WebNavigationTabObserver::WebContentsDestroyed() {
   registrar_.RemoveAll();
 }
 
+void WebNavigationTabObserver::HandleCommit(
+    content::NavigationHandle* navigation_handle) {
+  bool is_reference_fragment_navigation = IsReferenceFragmentNavigation(
+      navigation_handle->GetRenderFrameHost(), navigation_handle->GetURL());
+
+  navigation_state_.StartTrackingDocumentLoad(
+      navigation_handle->GetRenderFrameHost(), navigation_handle->GetURL(),
+      navigation_handle->IsSamePage(),
+      false,  // is_error_page
+      navigation_handle->IsSrcdoc());
+
+  events::HistogramValue histogram_value = events::UNKNOWN;
+  std::string event_name;
+  if (is_reference_fragment_navigation) {
+    histogram_value = events::WEB_NAVIGATION_ON_REFERENCE_FRAGMENT_UPDATED;
+    event_name = web_navigation::OnReferenceFragmentUpdated::kEventName;
+  } else if (navigation_handle->IsSamePage()) {
+    histogram_value = events::WEB_NAVIGATION_ON_HISTORY_STATE_UPDATED;
+    event_name = web_navigation::OnHistoryStateUpdated::kEventName;
+  } else {
+    histogram_value = events::WEB_NAVIGATION_ON_COMMITTED;
+    event_name = web_navigation::OnCommitted::kEventName;
+  }
+  helpers::DispatchOnCommitted(histogram_value, event_name, navigation_handle);
+}
+
+void WebNavigationTabObserver::HandleError(
+    content::NavigationHandle* navigation_handle) {
+  navigation_state_.StartTrackingDocumentLoad(
+      navigation_handle->GetRenderFrameHost(), navigation_handle->GetURL(),
+      navigation_handle->IsSamePage(),
+      true,  // is_error_page
+      navigation_handle->IsSrcdoc());
+
+  helpers::DispatchOnErrorOccurred(navigation_handle);
+}
+
 // See also NavigationController::IsURLInPageNavigation.
 bool WebNavigationTabObserver::IsReferenceFragmentNavigation(
     content::RenderFrameHost* render_frame_host,