Refactor the implementation of the webNavigation extension API.

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

Index: chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
diff --git a/chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc b/chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
index f24a8bcbf7935254ee366c035e98a7cf1e536bad..c7c67fd73195a2bc49c0682eae9397ada941ef06 100644
--- a/chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
+++ b/chrome/browser/extensions/api/web_navigation/frame_navigation_state.cc
@@ -62,9 +62,10 @@ bool FrameNavigationState::CanSendEvents(
   return IsValidUrl(it->second.url);
 }
 
-void FrameNavigationState::StartTrackingNavigation(
+void FrameNavigationState::StartTrackingDocumentLoad(
     content::RenderFrameHost* frame_host,
     const GURL& url,
+    bool is_same_page,
     bool is_error_page,
     bool is_iframe_srcdoc) {
   FrameState& frame_state = frame_host_state_map_[frame_host];
@@ -72,10 +73,10 @@ void FrameNavigationState::StartTrackingNavigation(
   frame_state.url = url;
   frame_state.is_iframe_srcdoc = is_iframe_srcdoc;
   DCHECK(!is_iframe_srcdoc || url == GURL(url::kAboutBlankURL));
-  frame_state.is_navigating = true;
-  frame_state.is_committed = false;
-  frame_state.is_server_redirected = false;
-  frame_state.is_parsing = true;
+  if (!is_same_page) {
+    frame_state.is_loading = true;
+    frame_state.is_parsing = true;
+  }
 }
 
 void FrameNavigationState::FrameHostCreated(
@@ -108,7 +109,6 @@ GURL FrameNavigationState::GetUrl(content::RenderFrameHost* frame_host) const {
   FrameHostToStateMap::const_iterator it =
       frame_host_state_map_.find(frame_host);
   if (it == frame_host_state_map_.end()) {
-    NOTREACHED();

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

Why is this okay now?

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

I am no longer tracking anything until the commit point. In order to know
whether the navigation was a reference fragment one, I need to check for
existing state before calling StartTrackingDocumentLoad. In that world, it is
expected for the map to not have any state for the frame, as this would be the
first navigation/document load in it, so it is fine to just return an empty
GURL.

     return GURL();
   }
   if (it->second.is_iframe_srcdoc)
@@ -141,7 +141,7 @@ void FrameNavigationState::SetNavigationCompleted(
     NOTREACHED();
     return;
   }
-  it->second.is_navigating = false;
+  it->second.is_loading = false;
 }
 
 bool FrameNavigationState::GetNavigationCompleted(
@@ -149,7 +149,7 @@ bool FrameNavigationState::GetNavigationCompleted(
   FrameHostToStateMap::const_iterator it =
       frame_host_state_map_.find(frame_host);
   DCHECK(it != frame_host_state_map_.end());

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

We typically don't encourage coding like
if (unexpected_condition) {
  DCHECK()
  // Safely handle unexpected condition
}
// Behave normally

(with the exception of, e.g., IPC messages and the like).  But this API seems to
do it a lot - is there any particular reason?

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

Unsure. It was written long ago, so probably remnant of the time. I would like
to not touch those parts, as it will make the refactor even bigger. If you feel
strongly about the syntax, I can do a follow up CL.

-  return it == frame_host_state_map_.end() || !it->second.is_navigating;
+  return it == frame_host_state_map_.end() || !it->second.is_loading;
 }
 
 void FrameNavigationState::SetParsingFinished(
@@ -170,40 +170,4 @@ bool FrameNavigationState::GetParsingFinished(
   return it == frame_host_state_map_.end() || !it->second.is_parsing;
 }
 
-void FrameNavigationState::SetNavigationCommitted(
-    content::RenderFrameHost* frame_host) {
-  FrameHostToStateMap::iterator it = frame_host_state_map_.find(frame_host);
-  if (it == frame_host_state_map_.end()) {
-    NOTREACHED();
-    return;
-  }
-  it->second.is_committed = true;
-}
-
-bool FrameNavigationState::GetNavigationCommitted(
-    content::RenderFrameHost* frame_host) const {
-  FrameHostToStateMap::const_iterator it =
-      frame_host_state_map_.find(frame_host);
-  DCHECK(it != frame_host_state_map_.end());
-  return it != frame_host_state_map_.end() && it->second.is_committed;
-}
-
-void FrameNavigationState::SetIsServerRedirected(
-    content::RenderFrameHost* frame_host) {
-  FrameHostToStateMap::iterator it = frame_host_state_map_.find(frame_host);
-  if (it == frame_host_state_map_.end()) {
-    NOTREACHED();
-    return;
-  }
-  it->second.is_server_redirected = true;
-}
-
-bool FrameNavigationState::GetIsServerRedirected(
-    content::RenderFrameHost* frame_host) const {
-  FrameHostToStateMap::const_iterator it =
-      frame_host_state_map_.find(frame_host);
-  DCHECK(it != frame_host_state_map_.end());
-  return it != frame_host_state_map_.end() && it->second.is_server_redirected;
-}
-
 }  // namespace extensions