Refactor the implementation of the webNavigation extension API.

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

 // Copyright (c) 2012 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 "chrome/browser/extensions/api/web_navigation/frame_navigation_state.h"
 
 #include "base/logging.h"
 #include "base/macros.h"
 #include "chrome/common/url_constants.h"
 #include "content/public/browser/render_frame_host.h"
@@ skipping 44 matching lines @@
 bool FrameNavigationState::CanSendEvents(
     content::RenderFrameHost* frame_host) const {
   FrameHostToStateMap::const_iterator it =
       frame_host_state_map_.find(frame_host);
   if (it == frame_host_state_map_.end() || it->second.error_occurred) {
     return false;
   }
   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];
   frame_state.error_occurred = is_error_page;
   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(
     content::RenderFrameHost* frame_host) {
   frame_hosts_.insert(frame_host);
 }
 
 void FrameNavigationState::FrameHostDeleted(
     content::RenderFrameHost* frame_host) {
   frame_host_state_map_.erase(frame_host);
@@ skipping 12 matching lines @@
 
 bool FrameNavigationState::IsValidFrame(
     content::RenderFrameHost* frame_host) const {
   return frame_host_state_map_.find(frame_host) != frame_host_state_map_.end();
 }
 
 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)
     return GURL(content::kAboutSrcDocURL);
   return it->second.url;
 }
 
 void FrameNavigationState::SetErrorOccurredInFrame(
     content::RenderFrameHost* frame_host) {
   FrameHostToStateMap::iterator it = frame_host_state_map_.find(frame_host);
@@ skipping 12 matching lines @@
   return it == frame_host_state_map_.end() || it->second.error_occurred;
 }
 
 void FrameNavigationState::SetNavigationCompleted(
     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_navigating = false;
+  it->second.is_loading = false;
 }
 
 bool FrameNavigationState::GetNavigationCompleted(
     content::RenderFrameHost* frame_host) const {
   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(
     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_parsing = false;
 }
 
 bool FrameNavigationState::GetParsingFinished(
     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_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