New Tab: fix tab title flickering.

content/browser/web_contents/web_contents_impl.cc

Index: content/browser/web_contents/web_contents_impl.cc
diff --git a/content/browser/web_contents/web_contents_impl.cc b/content/browser/web_contents/web_contents_impl.cc
index 05c685391a67e294fe153da8b23a69902b0ded00..71d702c6e1c0b00509b8dd50b8e37997ddd13990 100644
--- a/content/browser/web_contents/web_contents_impl.cc
+++ b/content/browser/web_contents/web_contents_impl.cc
@@ -756,15 +756,13 @@ const string16& WebContentsImpl::GetTitle() const {
   // title.
   entry = controller_.GetLastCommittedEntry();
 
-  // We make an exception for initial navigations, because we can have a
-  // committed entry for an initial navigation when doing a history navigation
-  // in a new tab, such as Ctrl+Back.
-  if (entry && controller_.IsInitialNavigation())
+  // We make an exception for initial navigations, because there is no committed

[Charlie Reis, 2013/11/22 01:47:45]

This comment isn't correct: there may or may not be a committed entry for
IsInitialNavigation.  For example, there's none with the NTP, but there is a
last committed entry if you use Ctrl+Back, which first clones the tab but does a
back navigation for the initial navigation.

[samarth, 2013/11/22 17:25:54]

Can you suggest a better wording? This code is very subtle and I will probably
still get it wrong :)

[Charlie Reis, 2013/11/22 18:54:18]

Sure, but I don't think we've established that this is the right fix yet.  I'm
happy to suggest a comment once we decide what to do.

+  // entry at this point.
+  if (controller_.IsInitialNavigation())

[Charlie Reis, 2013/11/22 01:47:45]

I don't immediately see a security concern with removing the "entry &&" part,
but it definitely changes the intent of this block.  If we can avoid changing it
using GetOverriddenTitle, I'd prefer that.

[Charlie Reis, 2013/11/22 18:54:18]

Ok, so GetOverriddenTitle doesn't work.  Still, this could end up changing other
cases.  For example, it looks like a slow URL opened in a new tab currently uses
page_title_when_no_navigation_entry_.  Is that something like "Loading" or
"Untitled", or an empty string that causes those strings to be displayed in some
other logic?  Your change would make it return the entry's title instead.  (If
there's no title, I think that's the URL, right?)

I want to understand what the implications are before we change this.  If
there's existing behavior we want to keep, then maybe this line should be
checking whether the pending entry has a non-empty GetTitle before deciding to
show it.

[samarth, 2013/11/22 20:03:19]

I tested this by ctrl-clicking on a link for http://blackhole.webpagetest.org/.
Without my change, the tab title says "Loading..." until it errors out. With my
change, it says "Loading.." and then switches to the URL as you guessed.

[Charlie Reis, 2013/11/22 20:33:47]

Ok, thanks for trying it out.  Let's try checking whether the pending entry has
a non-empty GetTitle on this line as well.  I don't expect most initial pending
entries to already have a title, unless we set it for a specific reason (like
your case).  The only other case that should have a title is the Ctrl+Back case,
and we wanted to show it in that case anyway.

[samarth, 2013/11/22 21:54:41]

Done.

     entry = controller_.GetVisibleEntry();
 
-  if (entry) {
+  if (entry)
     return entry->GetTitleForDisplay(accept_languages);
-  }
 
   // |page_title_when_no_navigation_entry_| is finally used
   // if no title cannot be retrieved.