Navigation: Move states from NavigationHandleState to NavigationState 4.

This moves the remaining states to NavigationRequest::NavigationState
and removes NavigationHandleState completely.

Bug: 916537
Change-Id: I6ccd701c8f5c12a59af3fa852031b0c9e2c55d53
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1890033
Commit-Queue: Mohamed Abdelhalim <zetamoo@chromium.org>
Reviewed-by: Camille Lamy <clamy@chromium.org>
Cr-Commit-Position: refs/heads/master@{#713052}

content/browser/frame_host/navigation_request.cc

@@ -860,9 +860,6 @@ std::unique_ptr<NavigationRequest> NavigationRequest::CreateForCommit(
   navigation_request->StartNavigation(true);
   DCHECK(navigation_request->IsNavigationStarted());
 
-  // Update the state of the NavigationRequest to match the fact that the
-  // navigation just committed.
-  navigation_request->state_ = RESPONSE_STARTED;
   return navigation_request;
 }
 
@@ -1059,7 +1056,7 @@ NavigationRequest::NavigationRequest(
 NavigationRequest::~NavigationRequest() {
   TRACE_EVENT_ASYNC_END0("navigation", "NavigationRequest", this);
   ResetExpectedProcess();
-  if (state_ >= WILL_START_NAVIGATION && state_ < RESPONSE_STARTED) {
+  if (state_ >= WILL_START_NAVIGATION && state_ < READY_TO_COMMIT) {
     devtools_instrumentation::OnNavigationRequestFailed(
         *this, network::URLLoaderCompletionStatus(net::ERR_ABORTED));
   }
@@ -1198,7 +1195,6 @@ void NavigationRequest::BeginNavigation() {
     // it immediately.
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                  "ResponseStarted");
-    state_ = RESPONSE_STARTED;
 
     // Select an appropriate RenderFrameHost.
     render_frame_host_ =
@@ -1339,10 +1335,9 @@ void NavigationRequest::ResetForCrossDocumentRestart() {
     TraceNavigationEnd();
   }
 
-  // Reset the states of the NavigationRequest, and the navigation_handle_id.
+  // Reset the state of the NavigationRequest, and the navigation_handle_id.
   StopCommitTimeout();
   state_ = NOT_STARTED;
-  handle_state_ = NOT_CREATED;
   processing_navigation_throttle_ = false;
   navigation_handle_id_ = 0;
 
@@ -1603,7 +1598,7 @@ void NavigationRequest::OnResponseStarted(
   DCHECK(response_head);
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "OnResponseStarted");
-  state_ = RESPONSE_STARTED;
+  state_ = WILL_PROCESS_RESPONSE;
   response_head_ = response_head;
   response_body_ = std::move(response_body);
   ssl_info_ = response_head->head.ssl_info;
@@ -1887,7 +1882,7 @@ void NavigationRequest::OnRequestFailedInternal(
     bool collapse_frame) {
   DCHECK(state_ == WILL_START_NAVIGATION || state_ == WILL_START_REQUEST ||
          state_ == WILL_REDIRECT_REQUEST || state_ == WILL_PROCESS_RESPONSE ||
-         state_ == RESPONSE_STARTED || state_ == CANCELING ||
+         state_ == DID_COMMIT || state_ == CANCELING ||
          state_ == WILL_FAIL_REQUEST);
   DCHECK(!(status.error_code == net::ERR_ABORTED &&
            error_page_content.has_value()));
@@ -1906,7 +1901,7 @@ void NavigationRequest::OnRequestFailedInternal(
   // net_error is a certificate error.
   TRACE_EVENT_ASYNC_STEP_INTO1("navigation", "NavigationRequest", this,
                                "OnRequestFailed", "error", status.error_code);
-  state_ = FAILED;
+  state_ = WILL_FAIL_REQUEST;
   processing_navigation_throttle_ = false;
 
   // Ensure the pending entry also gets discarded if it has no other active
@@ -3140,9 +3135,9 @@ void NavigationRequest::WillProcessResponse(
     ThrottleChecksFinishedCallback callback) {
   TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationRequest", this,
                                "WillProcessResponse");
+  DCHECK_EQ(state_, WILL_PROCESS_RESPONSE);
 
   complete_callback_ = std::move(callback);
-  state_ = WILL_PROCESS_RESPONSE;
   processing_navigation_throttle_ = true;
 
   // Notify each throttle of the response.
@@ -3202,18 +3197,18 @@ void NavigationRequest::DidCommitNavigation(
       net_error_ != net::OK) {
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                  "DidCommitNavigation: error page");
-    handle_state_ = DID_COMMIT_ERROR_PAGE;
+    state_ = DID_COMMIT_ERROR_PAGE;
   } else {
     TRACE_EVENT_ASYNC_STEP_INTO0("navigation", "NavigationHandle", this,
                                  "DidCommitNavigation");
-    handle_state_ = DID_COMMIT;
+    state_ = DID_COMMIT;
   }
 
   StopCommitTimeout();
 
   // Record metrics for the time it took to commit the navigation if it was to
   // another document without error.
-  if (!IsSameDocument() && handle_state_ != DID_COMMIT_ERROR_PAGE) {
+  if (!IsSameDocument() && state_ != DID_COMMIT_ERROR_PAGE) {
     ui::PageTransition transition = common_params_->transition;
     base::Optional<bool> is_background =
         render_frame_host_->GetProcess()->IsProcessBackgrounded();
@@ -3230,8 +3225,7 @@ void NavigationRequest::DidCommitNavigation(
 
   // For successful navigations, ensure the frame owner element is no longer
   // collapsed as a result of a prior navigation.
-  if (handle_state_ != DID_COMMIT_ERROR_PAGE &&
-      !frame_tree_node()->IsMainFrame()) {
+  if (state_ != DID_COMMIT_ERROR_PAGE && !frame_tree_node()->IsMainFrame()) {
     // The last committed load in collapsed frames will be an error page with
     // |kUnreachableWebDataURL|. Same-document navigation should not be
     // possible.
@@ -3312,8 +3306,7 @@ void NavigationRequest::ReadyToCommitNavigation(bool is_error) {
   AddNetworkServiceDebugEvent(
       std::string("RTCN") +
       (render_frame_host_->GetProcess()->IsReady() ? "1" : "0"));
-  handle_state_ = READY_TO_COMMIT;
-  state_ = RESPONSE_STARTED;
+  state_ = READY_TO_COMMIT;
   ready_to_commit_time_ = base::TimeTicks::Now();
   RestartCommitTimeout();
 
@@ -3352,7 +3345,7 @@ NavigationRequest::TakeAppCacheHandle() {
 }
 
 bool NavigationRequest::IsWaitingToCommit() {
-  return handle_state_ == NavigationRequest::READY_TO_COMMIT;
+  return state_ == READY_TO_COMMIT;
 }
 
 void NavigationRequest::RunCompleteCallback(
@@ -3387,7 +3380,7 @@ void NavigationRequest::StopCommitTimeout() {
 
 void NavigationRequest::RestartCommitTimeout() {
   commit_timeout_timer_.Stop();
-  if (handle_state_ >= DID_COMMIT)
+  if (state_ >= DID_COMMIT)
     return;
 
   RenderProcessHost* renderer_host =
@@ -3407,7 +3400,7 @@ void NavigationRequest::RestartCommitTimeout() {
 }
 
 void NavigationRequest::OnCommitTimeout() {
-  DCHECK_EQ(READY_TO_COMMIT, handle_state_);
+  DCHECK_EQ(READY_TO_COMMIT, state_);
   AddNetworkServiceDebugEvent("T");
 #if defined(OS_ANDROID)
   // Rate limit the number of stack dumps so we don't overwhelm our crash
@@ -3554,11 +3547,11 @@ net::IPEndPoint NavigationRequest::GetSocketAddress() {
 }
 
 bool NavigationRequest::HasCommitted() {
-  return handle_state_ == DID_COMMIT || handle_state_ == DID_COMMIT_ERROR_PAGE;
+  return state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE;
 }
 
 bool NavigationRequest::IsErrorPage() {
-  return handle_state_ == DID_COMMIT_ERROR_PAGE;
+  return state_ == DID_COMMIT_ERROR_PAGE;
 }
 
 net::HttpResponseInfo::ConnectionInfo NavigationRequest::GetConnectionInfo() {
@@ -3730,22 +3723,22 @@ net::NetworkIsolationKey NavigationRequest::GetNetworkIsolationKey() {
 
 bool NavigationRequest::HasSubframeNavigationEntryCommitted() {
   DCHECK(!frame_tree_node_->IsMainFrame());
-  DCHECK(handle_state_ == DID_COMMIT || handle_state_ == DID_COMMIT_ERROR_PAGE);
+  DCHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE);
   return subframe_entry_committed_;
 }
 
 bool NavigationRequest::DidReplaceEntry() {
-  DCHECK(handle_state_ == DID_COMMIT || handle_state_ == DID_COMMIT_ERROR_PAGE);
+  DCHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE);
   return did_replace_entry_;
 }
 
 bool NavigationRequest::ShouldUpdateHistory() {
-  DCHECK(handle_state_ == DID_COMMIT || handle_state_ == DID_COMMIT_ERROR_PAGE);
+  DCHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE);
   return should_update_history_;
 }
 
 const GURL& NavigationRequest::GetPreviousURL() {
-  DCHECK(handle_state_ == DID_COMMIT || handle_state_ == DID_COMMIT_ERROR_PAGE);
+  DCHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE);
   return previous_url_;
 }
 

content/browser/frame_host/navigation_request.h

@@ -110,9 +110,12 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
     // asynchronous.
     WILL_PROCESS_RESPONSE,
 
-    // The response started on the IO thread and is ready to be committed. This
-    // is one of the two final states for the request.
-    RESPONSE_STARTED,
+    // The response started on the IO thread and is ready to be committed.
+    READY_TO_COMMIT,
+
+    // The response has been committed. This is one of the two final states of
+    // the request.
+    DID_COMMIT,
 
     // The request is being canceled.
     CANCELING,
@@ -123,19 +126,6 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
 
     // The request failed on the IO thread and an error page should be
     // displayed. This is one of the two final states for the request.
-    FAILED,
-  };
-
-  // Used to track the navigation state of NavigationHandleImpl.
-  // Note: the states named PROCESSING_* indicate that NavigationThrottles are
-  // currently processing the corresponding event. When they are done, the
-  // state will move to the next in the list.
-  // TODO(zetamoo): Merge NavigationHandleState with NavigationState, and remove
-  //                the duplicates.
-  enum NavigationHandleState {
-    NOT_CREATED = 0,
-    READY_TO_COMMIT,
-    DID_COMMIT,
     DID_COMMIT_ERROR_PAGE,
   };
 
@@ -389,8 +379,6 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
   // navigation_client used to commit.
   void IgnoreCommitInterfaceDisconnection();
 
-  NavigationHandleState handle_state() { return handle_state_; }
-
   // Resume and CancelDeferredNavigation must only be called by the
   // NavigationThrottle that is currently deferring the navigation.
   // |resuming_throttle| and |cancelling_throttle| are the throttles calling
@@ -414,7 +402,7 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
   }
 
   // Called when the navigation was committed.
-  // This will update the |handle_state_|.
+  // This will update the |state_|.
   // |navigation_entry_committed| indicates whether the navigation changed which
   // NavigationEntry is current.
   // |did_replace_entry| is true if the committed entry has replaced the
@@ -427,7 +415,7 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
       NavigationType navigation_type);
 
   NavigationType navigation_type() const {
-    DCHECK_GE(handle_state_, DID_COMMIT);
+    DCHECK(state_ == DID_COMMIT || state_ == DID_COMMIT_ERROR_PAGE);
     return navigation_type_;
   }
 
@@ -792,7 +780,7 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
   bool NeedsUrlLoader();
 
   // Called when the navigation is ready to be committed. This will update the
-  // |handle_state_| and inform the delegate.
+  // |state_| and inform the delegate.
   void ReadyToCommitNavigation(bool is_error);
 
   // Whether the navigation was sent to be committed in a renderer by the
@@ -973,9 +961,6 @@ class CONTENT_EXPORT NavigationRequest : public NavigationHandle,
   // See NavigationHandle::GetNavigationEntryOffset() for details.
   int navigation_entry_offset_ = 0;
 
-  // TODO(zetamoo): Merge |handle_state_| with |state_|.
-  NavigationHandleState handle_state_ = NOT_CREATED;
-
   // Owns the NavigationThrottles associated with this navigation, and is
   // responsible for notifying them about the various navigation events.
   std::unique_ptr<NavigationThrottleRunner> throttle_runner_;

content/browser/frame_host/navigation_request_unittest.cc

@@ -139,13 +139,7 @@ class NavigationRequestTest : public RenderViewHostImplTestHarness {
     return callback_result_;
   }
 
-  NavigationRequest::NavigationHandleState state() {
-    return request_->handle_state();
-  }
-
-  NavigationRequest::NavigationState request_state() {
-    return request_->state();
-  }
+  NavigationRequest::NavigationState state() { return request_->state(); }
 
   bool call_counts_match(TestNavigationThrottle* throttle,
                          int start,
@@ -288,19 +282,19 @@ TEST_F(NavigationRequestTest, SimpleDataChecksFailure) {
 TEST_F(NavigationRequestTest, CancelDeferredWillStart) {
   TestNavigationThrottle* test_throttle =
       CreateTestNavigationThrottle(NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest. The request should be deferred. The callback
   // should not have been called.
   SimulateWillStartRequest();
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
   EXPECT_FALSE(was_callback_called());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
 
   // Cancel the request. The callback should have been called.
   CancelDeferredNavigation(NavigationThrottle::CANCEL_AND_IGNORE);
-  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
+  EXPECT_EQ(NavigationRequest::CANCELING, state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
@@ -311,19 +305,19 @@ TEST_F(NavigationRequestTest, CancelDeferredWillStart) {
 TEST_F(NavigationRequestTest, CancelDeferredWillRedirect) {
   TestNavigationThrottle* test_throttle =
       CreateTestNavigationThrottle(NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillRedirectRequest. The request should be deferred. The callback
   // should not have been called.
   SimulateWillRedirectRequest();
-  EXPECT_EQ(NavigationRequest::WILL_REDIRECT_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_REDIRECT_REQUEST, state());
   EXPECT_FALSE(was_callback_called());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 1, 0, 0));
 
   // Cancel the request. The callback should have been called.
   CancelDeferredNavigation(NavigationThrottle::CANCEL_AND_IGNORE);
-  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
+  EXPECT_EQ(NavigationRequest::CANCELING, state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 1, 0, 0));
@@ -334,7 +328,7 @@ TEST_F(NavigationRequestTest, CancelDeferredWillRedirect) {
 TEST_F(NavigationRequestTest, CancelDeferredWillFail) {
   TestNavigationThrottle* test_throttle = CreateTestNavigationThrottle(
       TestNavigationThrottle::WILL_FAIL_REQUEST, NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest.
@@ -344,13 +338,13 @@ TEST_F(NavigationRequestTest, CancelDeferredWillFail) {
   // Simulate WillFailRequest. The request should be deferred. The callback
   // should not have been called.
   SimulateWillFailRequest(net::ERR_CERT_DATE_INVALID);
-  EXPECT_EQ(NavigationRequest::WILL_FAIL_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_FAIL_REQUEST, state());
   EXPECT_FALSE(was_callback_called());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 1, 0));
 
   // Cancel the request. The callback should have been called.
   CancelDeferredNavigation(NavigationThrottle::CANCEL_AND_IGNORE);
-  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
+  EXPECT_EQ(NavigationRequest::CANCELING, state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL_AND_IGNORE, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 1, 0));
@@ -360,19 +354,19 @@ TEST_F(NavigationRequestTest, CancelDeferredWillFail) {
 TEST_F(NavigationRequestTest, CancelDeferredWillRedirectNoIgnore) {
   TestNavigationThrottle* test_throttle =
       CreateTestNavigationThrottle(NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest. The request should be deferred. The callback
   // should not have been called.
   SimulateWillStartRequest();
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
 
   // Cancel the request. The callback should have been called with CANCEL, and
   // not CANCEL_AND_IGNORE.
   CancelDeferredNavigation(NavigationThrottle::CANCEL);
-  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
+  EXPECT_EQ(NavigationRequest::CANCELING, state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 0, 0));
@@ -383,7 +377,7 @@ TEST_F(NavigationRequestTest, CancelDeferredWillRedirectNoIgnore) {
 TEST_F(NavigationRequestTest, CancelDeferredWillFailNoIgnore) {
   TestNavigationThrottle* test_throttle = CreateTestNavigationThrottle(
       TestNavigationThrottle::WILL_FAIL_REQUEST, NavigationThrottle::DEFER);
-  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_START_REQUEST, state());
   EXPECT_TRUE(call_counts_match(test_throttle, 0, 0, 0, 0));
 
   // Simulate WillStartRequest.
@@ -393,14 +387,14 @@ TEST_F(NavigationRequestTest, CancelDeferredWillFailNoIgnore) {
   // Simulate WillFailRequest. The request should be deferred. The callback
   // should not have been called.
   SimulateWillFailRequest(net::ERR_CERT_DATE_INVALID);
-  EXPECT_EQ(NavigationRequest::WILL_FAIL_REQUEST, request_state());
+  EXPECT_EQ(NavigationRequest::WILL_FAIL_REQUEST, state());
   EXPECT_FALSE(was_callback_called());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 1, 0));
 
   // Cancel the request. The callback should have been called with CANCEL, and
   // not CANCEL_AND_IGNORE.
   CancelDeferredNavigation(NavigationThrottle::CANCEL);
-  EXPECT_EQ(NavigationRequest::CANCELING, request_state());
+  EXPECT_EQ(NavigationRequest::CANCELING, state());
   EXPECT_TRUE(was_callback_called());
   EXPECT_EQ(NavigationThrottle::CANCEL, callback_result());
   EXPECT_TRUE(call_counts_match(test_throttle, 1, 0, 1, 0));

content/browser/frame_host/render_frame_host_manager.cc

@@ -714,7 +714,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     //   navigation yet.
     if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
             browser_context, request->common_params().url) &&
-        request->state() != NavigationRequest::FAILED) {
+        request->state() < NavigationRequest::CANCELING) {
       if (render_frame_host_->has_committed_any_navigation()) {
         // If |render_frame_host_| has committed at least one navigation and it
         // is in a WebUI SiteInstance, then it must have the exact same WebUI
@@ -775,7 +775,7 @@ RenderFrameHostImpl* RenderFrameHostManager::GetFrameHostForNavigation(
     // allow the navigation to be served correctly.
     if (WebUIControllerFactoryRegistry::GetInstance()->UseWebUIForURL(
             browser_context, request->common_params().url) &&
-        request->state() != NavigationRequest::FAILED) {
+        request->state() < NavigationRequest::CANCELING) {
       bool created_web_ui = speculative_render_frame_host_->CreateWebUI(
           request->common_params().url, request->bindings());
       notify_webui_of_rf_creation =
@@ -2145,7 +2145,7 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest(
       request->common_params().url, request->GetSourceSiteInstance(),
       request->dest_site_instance(), candidate_site_instance,
       request->common_params().transition,
-      request->state() == NavigationRequest::FAILED,
+      request->state() >= NavigationRequest::CANCELING,
       request->GetRestoreType() != RestoreType::NONE, request->is_view_source(),
       request->WasServerRedirect());
 

content/test/test_render_frame_host.cc

@@ -403,7 +403,7 @@ void TestRenderFrameHost::PrepareForCommitInternal(
     return;  // |request| is destructed by now.
 
   CHECK(request->state() >= NavigationRequest::WILL_START_NAVIGATION &&
-        request->state() < NavigationRequest::RESPONSE_STARTED);
+        request->state() < NavigationRequest::READY_TO_COMMIT);
 
   if (!request->loader_for_testing()) {
     base::RunLoop loop;