Solidify Entry discarding logic (NavigationHandle keeps its ID)

content/browser/frame_host/navigator_impl.cc

Index: content/browser/frame_host/navigator_impl.cc
diff --git a/content/browser/frame_host/navigator_impl.cc b/content/browser/frame_host/navigator_impl.cc
index 960ef9150e66aee0847045a0bcaaf7df3e28bf35..eeb3c9c2d03bca251d9c4e443596bfa6a8e3f785 100644
--- a/content/browser/frame_host/navigator_impl.cc
+++ b/content/browser/frame_host/navigator_impl.cc
@@ -152,7 +152,8 @@ void NavigatorImpl::DidStartProvisionalLoad(
   }
 
   render_frame_host->SetNavigationHandle(NavigationHandleImpl::Create(
-      validated_url, render_frame_host->frame_tree_node(), navigation_start));
+      validated_url, render_frame_host->frame_tree_node(), navigation_start,
+      controller_->GetPendingEntry()));
 }
 
 void NavigatorImpl::DidFailProvisionalLoadWithError(
@@ -214,10 +215,14 @@ void NavigatorImpl::DidFailProvisionalLoadWithError(
   // (e.g., hitting Escape with focus in the address bar).
   //
   // Note: don't touch the transient entry, since an interstitial may exist.
+  NavigationHandleImpl* handle = render_frame_host->navigation_handle();
+  NavigationEntry* pending = controller_->GetPendingEntry();

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

nit: pending_entry

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

Done.

+  bool pending_matches_fail =

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

nit: pending_entry_matches_fail_msg

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

Done.

+      handle && pending && handle->get_entry_id() == pending->GetUniqueID();
   bool should_preserve_entry = controller_->IsUnmodifiedBlankTab() ||
       delegate_->ShouldPreserveAbortedURLs();
-  if (controller_->GetPendingEntry() != controller_->GetVisibleEntry() ||
-      !should_preserve_entry) {
+  if ((pending != controller_->GetVisibleEntry() || !should_preserve_entry) &&
+      pending_matches_fail) {

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

This is really hard to follow.  (It was already really hard to follow, and now I
think it's past the threshold that I can reason about it.)  :)

I think we're basically saying: do what we did before, but make sure we only
discard the pending entry here if the DidFail message matches the pending entry.

Maybe this would be more legible like:

  // Big comment about why we only care if the pending entry matches the IPC
  NavHandle handle...
  if (pending_entry_matches_fail_msg) {
    // We usually...
    bool should_preserve...
    if (pending_entry != ...) {
      Discard + Notify
    }
  }

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

SGTM. My projections show that by next year, this method will read as a tidy
novella!

     controller_->DiscardPendingEntry(true);
 
     // Also force the UI to refresh.
@@ -773,8 +778,6 @@ void NavigatorImpl::OnBeginNavigation(
           controller_->GetLastCommittedEntryIndex(),
           controller_->GetEntryCount()));
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
-  navigation_request->CreateNavigationHandle();
-
   if (frame_tree_node->IsMainFrame()) {
     // Renderer-initiated main-frame navigations that need to swap processes
     // will go to the browser via a OpenURL call, and then be handled by the
@@ -789,6 +792,8 @@ void NavigatorImpl::OnBeginNavigation(
     navigation_data_.reset();
   }
 
+  navigation_request->CreateNavigationHandle(controller_->GetPendingEntry());

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

nit: Comment about why this must be after DidStartMainFrameNavigation, and no
blank line after.

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

Done.

+
   navigation_request->BeginNavigation();
 }
 
@@ -941,7 +946,7 @@ void NavigatorImpl::RequestNavigation(
           navigation_type, is_same_document_history_load, navigation_start,
           controller_));
   NavigationRequest* navigation_request = frame_tree_node->navigation_request();
-  navigation_request->CreateNavigationHandle();
+  navigation_request->CreateNavigationHandle(controller_->GetVisibleEntry());

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

Shouldn't this be GetPendingEntry()?  GetVisibleEntry() can return the last
committed entry in situations that we don't think it's safe to display the
pending one (e.g., renderer-initiated navigations).

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

Yes. Good catch. In fact, here we can use the NavigationEntryImpl that was
passed into the method.

 
   // Have the current renderer execute its beforeunload event if needed. If it
   // is not needed (when beforeunload dispatch is not needed or this navigation