Always update the omnibox URL when cancelling via onbeforeunload

content/browser/frame_host/navigation_controller_impl.cc

 // Copyright 2013 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.
 
 /*
  * Copyright (C) 2006, 2007, 2008, 2009 Apple Inc. All rights reserved.
  * Copyright (C) 2008 Nokia Corporation and/or its subsidiary(-ies)
  * Copyright (C) 2008, 2009 Torch Mobile Inc. All rights reserved.
  *     (http://www.torchmobile.com/)
  *
@@ skipping 877 matching lines @@
         return false;
       }
       break;
     case NAVIGATION_TYPE_NAV_IGNORE:
       // If a pending navigation was in progress, this canceled it.  We should
       // discard it and make sure it is removed from the URL bar.  After that,
       // there is nothing we can do with this navigation, so we just return to
       // the caller that nothing has happened.
       if (pending_entry_) {
         DiscardNonCommittedEntries();
-        delegate_->NotifyNavigationStateChanged(INVALIDATE_TYPE_URL);
       }
       return false;
     default:
       NOTREACHED();
   }
 
   // At this point, we know that the navigation has just completed, so
   // record the time.
   //
   // TODO(akalin): Use "sane time" as described in
@@ skipping 819 matching lines @@
   DCHECK(index < GetEntryCount());
   DCHECK(index != last_committed_entry_index_);
 
   DiscardNonCommittedEntries();
 
   entries_.erase(entries_.begin() + index);
   if (last_committed_entry_index_ > index)
     last_committed_entry_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);
   }
 }
 
 NavigationEntryImpl* NavigationControllerImpl::GetPendingEntry() const {
   // If there is no pending_entry_, there should be no pending_entry_index_.
   DCHECK(pending_entry_ || pending_entry_index_ == -1);
 
   // If there is a pending_entry_index_, then pending_entry_ must be the entry
   // at that index.
   DCHECK(pending_entry_index_ == -1 ||
@@ skipping 437 matching lines @@
   DCHECK(pending_entry_index_ == -1 ||
          pending_entry_ == GetEntryAtIndex(pending_entry_index_));
 }
 
 void NavigationControllerImpl::SetGetTimestampCallbackForTest(
     const base::Callback<base::Time()>& get_timestamp_callback) {
   get_timestamp_callback_ = get_timestamp_callback;
 }
 
 }  // namespace content