Solidify Entry discarding logic (NavigationHandle keeps its ID)

content/browser/frame_host/navigation_handle_impl.cc

Index: content/browser/frame_host/navigation_handle_impl.cc
diff --git a/content/browser/frame_host/navigation_handle_impl.cc b/content/browser/frame_host/navigation_handle_impl.cc
index 9dfe16c5340b02cfab9f7eb592c63c7b5c7d06f6..be6966ca0980e5c7c84b6016427a8d51e3aa7b5e 100644
--- a/content/browser/frame_host/navigation_handle_impl.cc
+++ b/content/browser/frame_host/navigation_handle_impl.cc
@@ -13,6 +13,7 @@
 #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"
@@ -33,15 +34,17 @@ void UpdateThrottleCheckResult(
 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),
@@ -54,7 +57,8 @@ NavigationHandleImpl::NavigationHandleImpl(
       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);
 }