Solidify Entry discarding logic (NavigationHandle keeps its ID)

content/browser/frame_host/navigation_handle_impl.cc

 // Copyright 2015 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 "content/browser/frame_host/navigation_handle_impl.h"
 
 #include <utility>
 
 #include "content/browser/frame_host/frame_tree_node.h"
 #include "content/browser/frame_host/navigator.h"
 #include "content/browser/frame_host/navigator_delegate.h"
 #include "content/browser/service_worker/service_worker_context_wrapper.h"
 #include "content/browser/service_worker/service_worker_navigation_handle.h"
 #include "content/common/frame_messages.h"
 #include "content/public/browser/content_browser_client.h"
+#include "content/public/browser/navigation_entry.h"
 #include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/content_client.h"
 #include "net/url_request/redirect_info.h"
 
 namespace content {
 
 namespace {
 
 void UpdateThrottleCheckResult(
     NavigationThrottle::ThrottleCheckResult* to_update,
     NavigationThrottle::ThrottleCheckResult result) {
   *to_update = result;
 }
 
 }  // namespace
 
 // static
 scoped_ptr<NavigationHandleImpl> NavigationHandleImpl::Create(
     const GURL& url,
     FrameTreeNode* frame_tree_node,
-    const base::TimeTicks& navigation_start) {
-  return scoped_ptr<NavigationHandleImpl>(
-      new NavigationHandleImpl(url, frame_tree_node, navigation_start));
+    const base::TimeTicks& navigation_start,
+    NavigationEntry* navigation_entry) {
+  return scoped_ptr<NavigationHandleImpl>(new NavigationHandleImpl(
+      url, frame_tree_node, navigation_start, navigation_entry));
 }
 
 NavigationHandleImpl::NavigationHandleImpl(
     const GURL& url,
     FrameTreeNode* frame_tree_node,
-    const base::TimeTicks& navigation_start)
+    const base::TimeTicks& navigation_start,
+    NavigationEntry* navigation_entry)

[Charlie Reis, 2016/02/05 19:36:11]

If we're only using the NavEntry's ID, maybe we should just pass that in and not
have this class depend on NavigationEntry itself?

[Charlie Harrison, 2016/02/05 23:10:23]

My argument would be that there now *is* a real dependency between this class
and NavigationEntry, and this just makes that relationship explicit and
statically verifiable. Feel free to override though :)

[Charlie Reis, 2016/02/06 00:54:57]

I see your point, but I feel like this tempts us to use more and more data from
the NavigationEntry over time, and makes it unclear how much NavigationHandle
needs to know (e.g., should it care if a new value gets added to NavEntry?).  It
also looks like |url| becomes redundant with the NavEntry's URL, but in some
cases we don't have a NavEntry.

I'd prefer to just state what's needed in the contract, which would be the ID.

     : url_(url),
       is_post_(false),
       has_user_gesture_(false),
       transition_(ui::PAGE_TRANSITION_LINK),
       is_external_protocol_(false),
       net_error_code_(net::OK),
       render_frame_host_(nullptr),
       is_same_page_(false),
       state_(INITIAL),
       is_transferring_(false),
       frame_tree_node_(frame_tree_node),
       next_index_(0),
-      navigation_start_(navigation_start) {
+      navigation_start_(navigation_start),
+      entry_id_(navigation_entry ? navigation_entry->GetUniqueID() : -1) {

[Charlie Reis, 2016/02/05 19:36:11]

nit: We use 0 for none (as in RenderFrameHostImpl::nav_entry_id_).

[Charlie Harrison, 2016/02/05 23:10:23]

Done.

   DCHECK(!navigation_start.is_null());
   GetDelegate()->DidStartNavigation(this);
 }
 
 NavigationHandleImpl::~NavigationHandleImpl() {
   GetDelegate()->DidFinishNavigation(this);
 
   // Cancel the navigation on the IO thread if the NavigationHandle is being
   // destroyed in the middle of the NavigationThrottles checks.
   if (!IsBrowserSideNavigationEnabled() && !complete_callback_.is_null())
@@ skipping 313 matching lines @@
 void NavigationHandleImpl::RunCompleteCallback(
     NavigationThrottle::ThrottleCheckResult result) {
   DCHECK(result != NavigationThrottle::DEFER);
   if (!complete_callback_.is_null())
     complete_callback_.Run(result);
 
   complete_callback_.Reset();
 }
 
 }  // namespace content