New Tab: fix tab title flickering.

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/debug/trace_event.h"
@@ skipping 726 matching lines @@
   if (entry) {
     return entry->GetTitleForDisplay(accept_languages);
   }
   WebUI* our_web_ui = GetRenderManager()->pending_web_ui() ?
       GetRenderManager()->pending_web_ui() : GetRenderManager()->web_ui();
   if (our_web_ui) {
     // Don't override the title in view source mode.
     entry = controller_.GetVisibleEntry();
     if (!(entry && entry->IsViewSourceMode())) {
       // Give the Web UI the chance to override our title.
       const string16& title = our_web_ui->GetOverriddenTitle();

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

Shouldn't we be using the WebUI's overridden title for the case you describe? 
We should have a pending_web_ui() in this case, and that would let us avoid
changing the code below.

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

With 1993, the NTP is no longer a WebUI, which is why this is an issue in the
first place.

[Charlie Reis, 2013/11/22 19:03:01]

Actually, I'm a also bit confused about this.  How is the NTP no longer WebUI? 
The iframes still need access to things like most-visited, right?  How is that
done?

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

This is done by loading the NTP in the "Instant Process" which has access to
chrome-search:// resources. (Things like most visited, theme data, etc are
served to the NTP over chrome-search://)

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

Makes sense.  Thanks for elaborating.

       if (!title.empty())
         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, 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.
   return page_title_when_no_navigation_entry_;
 }
 
 int32 WebContentsImpl::GetMaxPageID() {
   return GetMaxPageIDForSiteInstance(GetSiteInstance());
 }
 
@@ skipping 2904 matching lines @@
 }
 
 void WebContentsImpl::OnFrameRemoved(
     RenderViewHostImpl* render_view_host,
     int64 frame_id) {
    FOR_EACH_OBSERVER(WebContentsObserver, observers_,
                      FrameDetached(render_view_host, frame_id));
 }
 
 }  // namespace content