Always update the omnibox URL when cancelling via onbeforeunload

content/browser/frame_host/navigation_controller_impl.cc

Index: content/browser/frame_host/navigation_controller_impl.cc
diff --git a/content/browser/frame_host/navigation_controller_impl.cc b/content/browser/frame_host/navigation_controller_impl.cc
index 1e0e55b71fc212027d87e08368f5815c83eb9acf..e5946bb34b00dfa140a47f1a96b37240bcf3f519 100644
--- a/content/browser/frame_host/navigation_controller_impl.cc
+++ b/content/browser/frame_host/navigation_controller_impl.cc
@@ -895,7 +895,6 @@ bool NavigationControllerImpl::RendererDidNavigate(
       // the caller that nothing has happened.
       if (pending_entry_) {
         DiscardNonCommittedEntries();
-        delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
       }
       return false;
     default:
@@ -1736,13 +1735,26 @@ void NavigationControllerImpl::RemoveEntryAtIndexInternal(int index) {
 }
 
 void NavigationControllerImpl::DiscardNonCommittedEntries() {

[ncarter (slow), 2017/06/01 23:08:07]

I debated whether to go with the approach you see here, or to just add a new
call to NotifyNavigationStateChanged from
WebContentsImpl::OnDialogClosed.

I put it here because the proper place to send a notification that a value has
changed, is from the class that owns the value. And since we already do a
notification here for the transient case, so doing it for the pending case seems
to make sense too.

However, because DiscardNonCommittedEntries called from several places (it's
content/public, actually), it's possible that this approach will either cause
double-invalidation. One case I've thought about is this one:
https://cs.chromium.org/chromium/src/components/omnibox/browser/omnibox_edit_...

[Charlie Reis, 2017/06/02 23:57:57]

I like the approach you've taken.  Can you update the declaration comment in
navigation_controller.h?

(I think the OmniboxEditModel::OnEscapeKeyPressed case should be ok...)

[ncarter (slow), 2017/06/19 18:14:18]

Done.

-  bool transient = transient_entry_index_ != -1;
+  bool discarding_visible_entry = GetVisibleEntry() != GetLastCommittedEntry();
+  bool had_transient_entry = GetTransientEntry() != nullptr;
+
+  // Clearing the transient entry should always result in a change to the
+  // visible entry.
+  DCHECK(!had_transient_entry || discarding_visible_entry);
+
+  // Actually discard pending entries.
   DiscardNonCommittedEntriesInternal();
 
-  // If there was a transient entry, invalidate everything so the new active
-  // entry state is shown.
-  if (transient) {
-    delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_ALL);
+  DCHECK_EQ(GetVisibleEntry(), GetLastCommittedEntry());
+  DCHECK(!GetTransientEntry());
+  DCHECK(!GetPendingEntry());
+
+  if (discarding_visible_entry) {
+    // If we discarded a visible transient entry, invalidate all.
+    // Otherwise we've discarded a pending entry, and we only
+    // need to invalidate the URL.
+    delegate_->NotifyNavigationStateChanged(
+        had_transient_entry ? INVALIDATE_TYPE_ALL : INVALIDATE_TYPE_URL);
   }
 }