Navigation: Move simple logic methods to NavigationRequest.

This CL is part of a series of CLs to remove any remaining logic inside
NavigationHandleImpl.

Bug: 916537
Change-Id: I03c88ff27ea6c05d8aec5a99a07a0ef919670efb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1751983
Commit-Queue: Mohamed Abdelhalim <zetamoo@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#688474}

content/browser/frame_host/navigation_handle_impl.cc

@@ -36,8 +36,7 @@ NavigationHandleImpl::NavigationHandleImpl(
     : navigation_request_(navigation_request) {
   const GURL& url = navigation_request_->common_params().url;
   TRACE_EVENT_ASYNC_BEGIN2("navigation", "NavigationHandle", this,
-                           "frame_tree_node",
-                           frame_tree_node()->frame_tree_node_id(), "url",
+                           "frame_tree_node", GetFrameTreeNodeId(), "url",
                            url.possibly_invalid_spec());
   DCHECK(!navigation_request_->common_params().navigation_start.is_null());
   DCHECK(!IsRendererDebugURL(url));
@@ -56,9 +55,6 @@ NavigationHandleImpl::NavigationHandleImpl(
 }
 
 NavigationHandleImpl::~NavigationHandleImpl() {
-
-  GetDelegate()->DidFinishNavigation(this);
-
   if (IsInMainFrame()) {
     TRACE_EVENT_ASYNC_END2("navigation", "Navigation StartToCommit", this,
                            "URL",
@@ -85,14 +81,11 @@ SiteInstanceImpl* NavigationHandleImpl::GetStartingSiteInstance() {
 }
 
 bool NavigationHandleImpl::IsInMainFrame() {
-  return frame_tree_node()->IsMainFrame();
+  return navigation_request_->IsInMainFrame();
 }
 
 bool NavigationHandleImpl::IsParentMainFrame() {
-  if (frame_tree_node()->parent())
-    return frame_tree_node()->parent()->IsMainFrame();
-
-  return false;
+  return navigation_request_->IsParentMainFrame();
 }
 
 bool NavigationHandleImpl::IsRendererInitiated() {
@@ -108,14 +101,11 @@ const std::vector<GURL>& NavigationHandleImpl::GetRedirectChain() {
 }
 
 int NavigationHandleImpl::GetFrameTreeNodeId() {
-  return frame_tree_node()->frame_tree_node_id();
+  return navigation_request_->GetFrameTreeNodeId();
 }
 
 RenderFrameHostImpl* NavigationHandleImpl::GetParentFrame() {
-  if (IsInMainFrame())
-    return nullptr;
-
-  return frame_tree_node()->parent()->current_frame_host();
+  return navigation_request_->GetParentFrame();
 }
 
 base::TimeTicks NavigationHandleImpl::NavigationStart() {
@@ -152,7 +142,7 @@ NavigationUIData* NavigationHandleImpl::GetNavigationUIData() {
 }
 
 bool NavigationHandleImpl::IsExternalProtocol() {
-  return !GetContentClient()->browser()->IsHandledURL(GetURL());
+  return navigation_request_->IsExternalProtocol();
 }
 
 net::Error NavigationHandleImpl::GetNetErrorCode() {
@@ -186,9 +176,7 @@ const net::HttpResponseHeaders* NavigationHandleImpl::GetResponseHeaders() {
 
 net::HttpResponseInfo::ConnectionInfo
 NavigationHandleImpl::GetConnectionInfo() {
-  return navigation_request_->response()
-             ? navigation_request_->response()->head.connection_info
-             : net::HttpResponseInfo::ConnectionInfo();
+  return navigation_request_->GetConnectionInfo();
 }
 
 const base::Optional<net::SSLInfo>& NavigationHandleImpl::GetSSLInfo() {
@@ -201,12 +189,11 @@ NavigationHandleImpl::GetAuthChallengeInfo() {
 }
 
 bool NavigationHandleImpl::HasCommitted() {
-  return state() == NavigationRequest::DID_COMMIT ||
-         state() == NavigationRequest::DID_COMMIT_ERROR_PAGE;
+  return navigation_request_->HasCommitted();
 }
 
 bool NavigationHandleImpl::IsErrorPage() {
-  return state() == NavigationRequest::DID_COMMIT_ERROR_PAGE;
+  return navigation_request_->IsErrorPage();
 }
 
 bool NavigationHandleImpl::HasSubframeNavigationEntryCommitted() {
@@ -226,13 +213,7 @@ const GURL& NavigationHandleImpl::GetPreviousURL() {
 }
 
 net::IPEndPoint NavigationHandleImpl::GetSocketAddress() {
-  // This is CANCELING because although the data comes in after
-  // WILL_PROCESS_RESPONSE, it's possible for the navigation to be cancelled
-  // after and the caller might want this value.
-  DCHECK_GE(state(), NavigationRequest::CANCELING);
-  return navigation_request_->response()
-             ? navigation_request_->response()->head.remote_endpoint
-             : net::IPEndPoint();
+  return navigation_request_->GetSocketAddress();
 }
 
 void NavigationHandleImpl::RegisterThrottleForTesting(
@@ -271,15 +252,11 @@ const GURL& NavigationHandleImpl::GetBaseURLForDataURL() {
 
 void NavigationHandleImpl::RegisterSubresourceOverride(
     mojom::TransferrableURLLoaderPtr transferrable_loader) {
-  if (!transferrable_loader)
-    return;
-
   navigation_request_->RegisterSubresourceOverride(
       std::move(transferrable_loader));
 }
 
 const GlobalRequestID& NavigationHandleImpl::GetGlobalRequestID() {
-  DCHECK_GE(state(), NavigationRequest::PROCESSING_WILL_PROCESS_RESPONSE);
   return navigation_request_->request_id();
 }
 
@@ -320,21 +297,16 @@ bool NavigationHandleImpl::FromDownloadCrossOriginRedirect() {
 }
 
 bool NavigationHandleImpl::IsSignedExchangeInnerResponse() {
-  return navigation_request_->response()
-             ? navigation_request_->response()
-                   ->head.is_signed_exchange_inner_response
-             : false;
+  return navigation_request_->IsSignedExchangeInnerResponse();
 }
 
 bool NavigationHandleImpl::HasPrefetchedAlternativeSubresourceSignedExchange() {
-  return !navigation_request_->commit_params()
-              .prefetched_signed_exchanges.empty();
+  return navigation_request_
+      ->HasPrefetchedAlternativeSubresourceSignedExchange();
 }
 
 bool NavigationHandleImpl::WasResponseCached() {
-  return navigation_request_->response()
-             ? navigation_request_->response()->head.was_fetched_via_cache
-             : false;
+  return navigation_request_->WasResponseCached();
 }
 
 const net::ProxyServer& NavigationHandleImpl::GetProxyServer() {

content/browser/frame_host/navigation_request.cc

@@ -903,11 +903,8 @@ NavigationRequest::~NavigationRequest() {
     navigation_handle_proxy_->DidFinish();
 #endif
 
-  // This is done manually here because the NavigationHandle destructor
-  // calls into WebContentsObserver::DidFinishNavigation, some of which need to
-  // then access navigation_request(). This is only possible if the handle is
-  // destroyed before the NavigationRequest.
-  navigation_handle_.reset();
+  if (navigation_handle())
+    GetDelegate()->DidFinishNavigation(navigation_handle());
 }
 
 void NavigationRequest::BeginNavigation() {
@@ -1146,9 +1143,12 @@ void NavigationRequest::ResetForCrossDocumentRestart() {
     navigation_handle_proxy_->DidFinish();
 #endif
 
-  // The below order of resets is necessary to avoid accessing null pointers.
-  // See https://crbug.com/958396.
-  navigation_handle_.reset();
+  // It is necessary to call DidFinishNavigation before resetting
+  // |navigation_handle_proxy_|. See https://crbug.com/958396.
+  if (navigation_handle()) {
+    GetDelegate()->DidFinishNavigation(navigation_handle());
+    navigation_handle_.reset();
+  }
 
 #if defined(OS_ANDROID)
   if (navigation_handle_proxy_)
@@ -1177,6 +1177,8 @@ void NavigationRequest::ResetForCrossDocumentRestart() {
 
 void NavigationRequest::RegisterSubresourceOverride(
     mojom::TransferrableURLLoaderPtr transferrable_loader) {
+  if (!transferrable_loader)
+    return;
   if (!subresource_overrides_)
     subresource_overrides_.emplace();
 
@@ -3234,4 +3236,59 @@ RenderFrameHostImpl* NavigationRequest::GetFrameHostForNavigation() {
   return frame_tree_node_->render_manager()->GetFrameHostForNavigation(*this);
 }
 
+bool NavigationRequest::IsExternalProtocol() {
+  return !GetContentClient()->browser()->IsHandledURL(common_params_->url);
+}
+
+bool NavigationRequest::IsSignedExchangeInnerResponse() {
+  return response() && response()->head.is_signed_exchange_inner_response;
+}
+
+net::IPEndPoint NavigationRequest::GetSocketAddress() {
+  // This is CANCELING because although the data comes in after
+  // WILL_PROCESS_RESPONSE, it's possible for the navigation to be cancelled
+  // after and the caller might want this value.
+  DCHECK_GE(handle_state_, CANCELING);
+  return response() ? response()->head.remote_endpoint : net::IPEndPoint();
+}
+
+bool NavigationRequest::HasCommitted() {
+  return handle_state_ == DID_COMMIT || handle_state_ == DID_COMMIT_ERROR_PAGE;
+}
+
+bool NavigationRequest::IsErrorPage() {
+  return handle_state_ == DID_COMMIT_ERROR_PAGE;
+}
+
+net::HttpResponseInfo::ConnectionInfo NavigationRequest::GetConnectionInfo() {
+  return response() ? response()->head.connection_info
+                    : net::HttpResponseInfo::ConnectionInfo();
+}
+
+bool NavigationRequest::IsInMainFrame() {
+  return frame_tree_node()->IsMainFrame();
+}
+
+RenderFrameHostImpl* NavigationRequest::GetParentFrame() {
+  return IsInMainFrame() ? nullptr
+                         : frame_tree_node()->parent()->current_frame_host();
+}
+
+bool NavigationRequest::IsParentMainFrame() {
+  FrameTreeNode* parent = frame_tree_node()->parent();
+  return parent && parent->IsMainFrame();
+}
+
+int NavigationRequest::GetFrameTreeNodeId() {
+  return frame_tree_node()->frame_tree_node_id();
+}
+
+bool NavigationRequest::WasResponseCached() {
+  return response() && response()->head.was_fetched_via_cache;
+}
+
+bool NavigationRequest::HasPrefetchedAlternativeSubresourceSignedExchange() {
+  return !commit_params_->prefetched_signed_exchanges.empty();
+}
+
 }  // namespace content

content/browser/frame_host/navigation_request.h

@@ -264,7 +264,10 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
   }
 
   const network::ResourceResponse* response() { return response_head_.get(); }
-  const GlobalRequestID& request_id() const { return request_id_; }
+  const GlobalRequestID& request_id() const {
+    DCHECK_GE(handle_state_, PROCESSING_WILL_PROCESS_RESPONSE);
+    return request_id_;
+  }
   bool is_download() const { return is_download_; }
   const base::Optional<net::SSLInfo>& ssl_info() { return ssl_info_; }
   const base::Optional<net::AuthChallengeInfo>& auth_challenge_info() {
@@ -498,6 +501,22 @@ class CONTENT_EXPORT NavigationRequest : public NavigationURLLoaderDelegate,
     return is_served_from_back_forward_cache_;
   }
 
+  // NavigationHandle implementation:
+  // TODO(https://crbug.com/995268): Make NavigationRequest inherit from
+  // NavigationHandle, once NavigationHandleImpl is deleted.
+  bool IsExternalProtocol();
+  bool IsSignedExchangeInnerResponse();
+  net::IPEndPoint GetSocketAddress();
+  bool HasCommitted();
+  bool IsErrorPage();
+  net::HttpResponseInfo::ConnectionInfo GetConnectionInfo();
+  bool IsInMainFrame();
+  RenderFrameHostImpl* GetParentFrame();
+  bool IsParentMainFrame();
+  int GetFrameTreeNodeId();
+  bool WasResponseCached();
+  bool HasPrefetchedAlternativeSubresourceSignedExchange();
+
  private:
   friend class NavigationRequestTest;