Don't rely on |entry| being null in initial title logic.

content/browser/web_contents/web_contents_impl.cc

 // Copyright (c) 2012 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/web_contents/web_contents_impl.h"
 
 #include <utility>
 
 #include "base/command_line.h"
 #include "base/lazy_instance.h"
@@ skipping 928 matching lines @@
         return title;
     }
   }
 
   // We use the title for the last committed entry rather than a pending
   // navigation entry. For example, when the user types in a URL, we want to
   // keep the old page's title until the new load has committed and we get a new
   // title.
   entry = controller_.GetLastCommittedEntry();
 
-  // We make an exception for initial navigations.
-  if (controller_.IsInitialNavigation()) {
-    // We only want to use the title from the visible entry in one of two cases:
-    // 1. There's already a committed entry for an initial navigation, in which
-    //    case we are doing a history navigation in a new tab (e.g., Ctrl+Back).
-    // 2. The pending entry has been explicitly assigned a title to display.
-    //
-    // If there's no last committed entry and no assigned title, we should fall
-    // back to |page_title_when_no_navigation_entry_| rather than showing the
-    // URL.
-    if (entry ||

[Avi (use Gerrit), 2015/09/28 19:11:39]

I'm not sure what this was supposed to mean. How could there be a last-committed
entry if the controller is in the initial navigation? A commit would have gone
through NavigationControllerImpl::RendererDidNavigate, which would have cleared
the initial navigation flag.

[Charlie Reis, 2015/09/28 19:21:41]

I thought the same at first glance, but there's actually a case with a "last
committed entry" during the initial navigation: Ctrl+Back.  This clones the tab
(and somewhat confusingly, the last committed entry index), but the initial
navigation is the Back navigation.

One of many fun cases I encountered in the epic https://crbug.com/9682.

-        (controller_.GetVisibleEntry() &&
-         !controller_.GetVisibleEntry()->GetTitle().empty())) {
-      entry = controller_.GetVisibleEntry();
-    }
+  // We make an exception for initial navigations. We only want to use the title
+  // from the visible entry if:
+  // 1. The pending entry has been explicitly assigned a title to display.
+  // 2. The user is doing a history navigation in a new tab (e.g., Ctrl+Back),
+  //    which case there is a pending entry index other than -1.
+  //
+  // Otherwise, we want to stick with the last committed entry's title during
+  // new navigations, which have pending entries at index -1 with no title.
+  if (controller_.IsInitialNavigation() &&
+      ((controller_.GetVisibleEntry() &&

[Charlie Reis, 2015/09/28 18:18:43]

Note: I'll be able to remove this line once we always have a last committed
entry, making this version a bit simpler and easier to read.

[Avi (use Gerrit), 2015/09/28 19:11:39]

Will you then (please) also kill
WebContentsImpl::page_title_when_no_navigation_entry_ ?

[Charlie Reis, 2015/09/28 19:21:41]

Yep!  There's a lot of cruft like that on the chopping block.  :)

+        !controller_.GetVisibleEntry()->GetTitle().empty()) ||
+       controller_.GetPendingEntryIndex() != -1)) {
+    entry = controller_.GetVisibleEntry();
   }
 
   if (entry) {
     return entry->GetTitleForDisplay(accept_languages);
   }
 
   // |page_title_when_no_navigation_entry_| is finally used
   // if no title cannot be retrieved.
   return page_title_when_no_navigation_entry_;
 }
@@ skipping 3667 matching lines @@
   return NULL;
 }
 
 void WebContentsImpl::SetForceDisableOverscrollContent(bool force_disable) {
   force_disable_overscroll_content_ = force_disable;
   if (view_)
     view_->SetOverscrollControllerEnabled(CanOverscrollContent());
 }
 
 }  // namespace content