Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
78443005:
New Tab: fix tab title flickering. (Closed)
|
Created:
7 years, 1 month ago by samarth Modified:
7 years ago Reviewers:
Charlie Reis CC:
chromium-reviews, Jered Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionNew Tab: fix tab title flickering.
The fix requires two changes:
(1) Change the implementation of WebContents::GetTitle() to use a non-empty
title on a pending entry for "initial navigations".
(2) Set the title on pending NTP entries as soon as they're created.
BUG=322099
TESTED=manual, per bug report
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=237180
Patch Set 1 #
Total comments: 5
Patch Set 2 : Use pending entry. #
Total comments: 17
Patch Set 3 : Clean up change. #
Total comments: 4
Patch Set 4 : Address comment + add tests. #Patch Set 5 : Rebase. #Patch Set 6 : Try again. #
Total comments: 6
Patch Set 7 : Address comments. #
Messages
Total messages: 22 (0 generated)
PTAL. Thanks, Samarth
+creis https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... chrome/browser/ui/browser_navigator.cc:276: // For Instant Extended NTPs, add a transient entry and explicitly set the This seems like a total hack to me. Isn't there a better way?
https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... chrome/browser/ui/browser_navigator.cc:276: // For Instant Extended NTPs, add a transient entry and explicitly set the On 2013/11/21 23:16:31, sky wrote: > This seems like a total hack to me. Isn't there a better way? Yeah. I tried setting the title on the pending entry, but that doesn't work, because WebContentsImpl looks at the last committed entry. It does have an exception[1] for "IsInitialNavigation" which sounds like it should cover this case, but IsInitialNavigation is set to false at that point. Is that a bug? [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we...
https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... chrome/browser/ui/browser_navigator.cc:276: // For Instant Extended NTPs, add a transient entry and explicitly set the On 2013/11/21 23:20:58, samarth wrote: > On 2013/11/21 23:16:31, sky wrote: > > This seems like a total hack to me. Isn't there a better way? > > Yeah. I tried setting the title on the pending entry, but that doesn't work, > because WebContentsImpl looks at the last committed entry. > > It does have an exception[1] for "IsInitialNavigation" which sounds like it > should cover this case, but IsInitialNavigation is set to false at that point. > Is that a bug? > > [1] > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... It's worth understanding why IsInitialNavigation is returning false, at least. It does seem plausible for that to be true in this case (not sure), and you definitely shouldn't be using the transient entry logic here.
Thanks, Samarth https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... chrome/browser/ui/browser_navigator.cc:276: // For Instant Extended NTPs, add a transient entry and explicitly set the On 2013/11/21 23:23:57, creis wrote: > On 2013/11/21 23:20:58, samarth wrote: > > On 2013/11/21 23:16:31, sky wrote: > > > This seems like a total hack to me. Isn't there a better way? > > > > Yeah. I tried setting the title on the pending entry, but that doesn't work, > > because WebContentsImpl looks at the last committed entry. > > > > It does have an exception[1] for "IsInitialNavigation" which sounds like it > > should cover this case, but IsInitialNavigation is set to false at that point. > > > Is that a bug? > > > > [1] > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > It's worth understanding why IsInitialNavigation is returning false, at least. > It does seem plausible for that to be true in this case (not sure), and you > definitely shouldn't be using the transient entry logic here. Ah ok, so the problem is that the check is IsInitialNavigation && last_committed_entry != NULL. In the case of the very first load, there is no committed entry and IsInitialNavigation is true. So one possible fix that works would be to: 1) Change WebContentsImpl::GetTitle to use the title from the pending entry whenever IsInitialNavigation is true. 2) Here, set the title on the pending entry. Charlie: would that introduce any security concerns? (Actually looking at this more carefully, I don't understand how the existing condition could ever be true. is_initial_navigation_ starts out true, is set to false once any entry is committed and never changed after that. So how would it ever be true if there is a valid committed entry?)
https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/1/chrome/browser/ui/browser_nav... chrome/browser/ui/browser_navigator.cc:276: // For Instant Extended NTPs, add a transient entry and explicitly set the On 2013/11/21 23:44:06, samarth wrote: > On 2013/11/21 23:23:57, creis wrote: > > On 2013/11/21 23:20:58, samarth wrote: > > > On 2013/11/21 23:16:31, sky wrote: > > > > This seems like a total hack to me. Isn't there a better way? > > > > > > Yeah. I tried setting the title on the pending entry, but that doesn't > work, > > > because WebContentsImpl looks at the last committed entry. > > > > > > It does have an exception[1] for "IsInitialNavigation" which sounds like it > > > should cover this case, but IsInitialNavigation is set to false at that > point. > > > > > Is that a bug? > > > > > > [1] > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/we... > > > > > > It's worth understanding why IsInitialNavigation is returning false, at least. > > > It does seem plausible for that to be true in this case (not sure), and you > > definitely shouldn't be using the transient entry logic here. > > Ah ok, so the problem is that the check is IsInitialNavigation && > last_committed_entry != NULL. In the case of the very first load, there is no > committed entry and IsInitialNavigation is true. > > So one possible fix that works would be to: > 1) Change WebContentsImpl::GetTitle to use the title from the pending entry > whenever IsInitialNavigation is true. > 2) Here, set the title on the pending entry. > > Charlie: would that introduce any security concerns? > > (Actually looking at this more carefully, I don't understand how the existing > condition could ever be true. is_initial_navigation_ starts out true, is set to > false once any entry is committed and never changed after that. So how would it > ever be true if there is a valid committed entry?) I implemented this in PS2. Let me know what you think.
https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:747: const string16& title = our_web_ui->GetOverriddenTitle(); 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. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:759: // We make an exception for initial navigations, because there is no committed 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. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:761: if (controller_.IsInitialNavigation()) 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.
https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:747: const string16& title = our_web_ui->GetOverriddenTitle(); On 2013/11/22 01:47:45, creis wrote: > 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. With 1993, the NTP is no longer a WebUI, which is why this is an issue in the first place. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:759: // We make an exception for initial navigations, because there is no committed On 2013/11/22 01:47:45, creis wrote: > 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. Can you suggest a better wording? This code is very subtle and I will probably still get it wrong :)
https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:285: if (chrome::IsNTPURL( Hmm, this call happens for almost every URL. (Soon it will happen for some subframe navigations as well.) It seems unfortunate to put checks for specific URLs here. Is there any way to set the title somewhere in the NTP logic instead, right after starting the navigation? https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:759: // We make an exception for initial navigations, because there is no committed On 2013/11/22 17:25:54, samarth wrote: > On 2013/11/22 01:47:45, creis wrote: > > 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. > > Can you suggest a better wording? This code is very subtle and I will probably > still get it wrong :) 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. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:761: if (controller_.IsInitialNavigation()) On 2013/11/22 01:47:45, creis wrote: > 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. 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.
https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:747: const string16& title = our_web_ui->GetOverriddenTitle(); On 2013/11/22 17:25:54, samarth wrote: > On 2013/11/22 01:47:45, creis wrote: > > 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. > > With 1993, the NTP is no longer a WebUI, which is why this is an issue in the > first place. 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?
https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:285: if (chrome::IsNTPURL( On 2013/11/22 18:54:18, creis wrote: > Hmm, this call happens for almost every URL. (Soon it will happen for some > subframe navigations as well.) It seems unfortunate to put checks for specific > URLs here. > > Is there any way to set the title somewhere in the NTP logic instead, right > after starting the navigation? Agreed, this doesn't seem ideal. I don't know where else to put the check though. The way the NTP works right now is that 1) The browser starts a regular navigation to chrome://newtab 2) We rewrite the URL when appropriate here: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... Then the rest of the navigation continues as normal. I don't think the ContentBrowserClient code has visibility into the naventry being used for the navigation (but I could be wrong). --- Ok, how about if I move this into SearchTabHelper and add a listener there for NavigateToPendingEntry? That essentially the same, but at least the logic is contained in the correct place. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:747: const string16& title = our_web_ui->GetOverriddenTitle(); On 2013/11/22 19:03:01, creis wrote: > On 2013/11/22 17:25:54, samarth wrote: > > On 2013/11/22 01:47:45, creis wrote: > > > 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. > > > > With 1993, the NTP is no longer a WebUI, which is why this is an issue in the > > first place. > > 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? 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://) https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:761: if (controller_.IsInitialNavigation()) On 2013/11/22 18:54:18, creis wrote: > On 2013/11/22 01:47:45, creis wrote: > > 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. > > 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. 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.
https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:285: if (chrome::IsNTPURL( On 2013/11/22 20:03:19, samarth wrote: > On 2013/11/22 18:54:18, creis wrote: > > Hmm, this call happens for almost every URL. (Soon it will happen for some > > subframe navigations as well.) It seems unfortunate to put checks for > specific > > URLs here. > > > > Is there any way to set the title somewhere in the NTP logic instead, right > > after starting the navigation? > > Agreed, this doesn't seem ideal. I don't know where else to put the check > though. The way the NTP works right now is that > 1) The browser starts a regular navigation to chrome://newtab > 2) We rewrite the URL when appropriate here: > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > Then the rest of the navigation continues as normal. I don't think the > ContentBrowserClient code has visibility into the naventry being used for the > navigation (but I could be wrong). > > --- > > Ok, how about if I move this into SearchTabHelper and add a listener there for > NavigateToPendingEntry? That essentially the same, but at least the logic is > contained in the correct place. That sounds perfect-- thanks. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:747: const string16& title = our_web_ui->GetOverriddenTitle(); On 2013/11/22 20:03:19, samarth wrote: > On 2013/11/22 19:03:01, creis wrote: > > On 2013/11/22 17:25:54, samarth wrote: > > > On 2013/11/22 01:47:45, creis wrote: > > > > 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. > > > > > > With 1993, the NTP is no longer a WebUI, which is why this is an issue in > the > > > first place. > > > > 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? > > 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://) Makes sense. Thanks for elaborating. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:761: if (controller_.IsInitialNavigation()) On 2013/11/22 20:03:19, samarth wrote: > On 2013/11/22 18:54:18, creis wrote: > > On 2013/11/22 01:47:45, creis wrote: > > > 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. > > > > 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. > > 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. 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.
Thanks, PTAL. I'll see if it's possible to write a good unit test for this, but that might prove difficult. Thanks, Samarth https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... File chrome/browser/ui/browser_navigator.cc (right): https://codereview.chromium.org/78443005/diff/90001/chrome/browser/ui/browser... chrome/browser/ui/browser_navigator.cc:285: if (chrome::IsNTPURL( On 2013/11/22 20:33:47, creis wrote: > On 2013/11/22 20:03:19, samarth wrote: > > On 2013/11/22 18:54:18, creis wrote: > > > Hmm, this call happens for almost every URL. (Soon it will happen for some > > > subframe navigations as well.) It seems unfortunate to put checks for > > specific > > > URLs here. > > > > > > Is there any way to set the title somewhere in the NTP logic instead, right > > > after starting the navigation? > > > > Agreed, this doesn't seem ideal. I don't know where else to put the check > > though. The way the NTP works right now is that > > 1) The browser starts a regular navigation to chrome://newtab > > 2) We rewrite the URL when appropriate here: > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/chr... > > > > Then the rest of the navigation continues as normal. I don't think the > > ContentBrowserClient code has visibility into the naventry being used for the > > navigation (but I could be wrong). > > > > --- > > > > Ok, how about if I move this into SearchTabHelper and add a listener there for > > NavigateToPendingEntry? That essentially the same, but at least the logic is > > contained in the correct place. > > That sounds perfect-- thanks. Done. https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/90001/content/browser/web_conte... content/browser/web_contents/web_contents_impl.cc:761: if (controller_.IsInitialNavigation()) On 2013/11/22 20:33:47, creis wrote: > On 2013/11/22 20:03:19, samarth wrote: > > On 2013/11/22 18:54:18, creis wrote: > > > On 2013/11/22 01:47:45, creis wrote: > > > > 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. > > > > > > 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. > > > > 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. > > 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. Done.
On 2013/11/22 21:54:41, samarth wrote: > I'll see if it's possible to write a good unit test for this, but that might > prove difficult. > You could probably do something like WebContentsImplTest.NTPViewSource (in web_contents_impl_unittest.cc), but call SetTitle on the pending entry after the LoadURL call, then see what contents()->GetTitle() returns. https://codereview.chromium.org/78443005/diff/150002/chrome/browser/ui/search... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/78443005/diff/150002/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:229: if (chrome::IsNTPURL(url, profile())) { Is this just a sanity check, or is this called on every navigation? (Even if we have to do it for every navigation, I agree that this is a better spot than browser_navigator.cc.) https://codereview.chromium.org/78443005/diff/150002/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/150002/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:758: const string16& title = entry->GetTitleForDisplay(accept_languages); This isn't quite right. GetTitleForDisplay will return the URL if no title is set, so the empty check below won't ever fail. We should be doing something closer to: if (!entry->GetTitle().empty()) return entry->GetTitleForDisplay(accept_languages); Of course, that changes the Ctrl+Back case, so I'd rather not reorder this code. I think we want the following, down below the entry = LastCommitted line. // 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 || (controller_.GetVisibleEntry() && !controller_.GetVisibleEntry()->GetTitle().empty())) { entry = controller_.GetVisibleEntry(); } }
Added tests. Thanks, Samarth https://codereview.chromium.org/78443005/diff/150002/chrome/browser/ui/search... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/78443005/diff/150002/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:229: if (chrome::IsNTPURL(url, profile())) { On 2013/11/22 23:19:12, creis wrote: > Is this just a sanity check, or is this called on every navigation? > > (Even if we have to do it for every navigation, I agree that this is a better > spot than browser_navigator.cc.) Called on every navigation. https://codereview.chromium.org/78443005/diff/150002/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/78443005/diff/150002/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:758: const string16& title = entry->GetTitleForDisplay(accept_languages); On 2013/11/22 23:19:12, creis wrote: > This isn't quite right. GetTitleForDisplay will return the URL if no title is > set, so the empty check below won't ever fail. > > We should be doing something closer to: > if (!entry->GetTitle().empty()) > return entry->GetTitleForDisplay(accept_languages); > > Of course, that changes the Ctrl+Back case, so I'd rather not reorder this code. > I think we want the following, down below the entry = LastCommitted line. > > // 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 || > (controller_.GetVisibleEntry() && > !controller_.GetVisibleEntry()->GetTitle().empty())) { > entry = controller_.GetVisibleEntry(); > } > } Done, thanks.
Thanks! Looks like the upload didn't work, though, since I can't review the patch. Can you try uploading again?
Strange, try now?
Thanks, worked this time. Sorry about the name change to "NavigateToPendingEntry" in the meantime. LGTM with the minor changes below! Nice tests, and thanks for your patience in finding the right fix. https://codereview.chromium.org/78443005/diff/300001/chrome/browser/ui/search... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/78443005/diff/300001/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:228: // any flickering of the tab title. nit: Please move the comment inside the if. https://codereview.chromium.org/78443005/diff/300001/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:231: web_contents_->GetController().GetVisibleEntry(); Please use GetPendingEntry instead of GetVisibleEntry. It's possible for the visible entry to be the committed one instead of the pending one (e.g., if we think a spoof is in progress). There will definitely be a pending entry in this case (given how the method is invoked), but you can include the null check if you'd like. https://codereview.chromium.org/78443005/diff/300001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/78443005/diff/300001/content/browser/web_cont... content/browser/web_contents/web_contents_impl_unittest.cc:324: TEST_F(WebContentsImplTest, UseTitleFromPendingEntry) { nit: UseTitleFromPendingEntryIfSet
Thanks, addressed all comments. sky@: did you want to take another look? Thanks, Samarth https://codereview.chromium.org/78443005/diff/300001/chrome/browser/ui/search... File chrome/browser/ui/search/search_tab_helper.cc (right): https://codereview.chromium.org/78443005/diff/300001/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:228: // any flickering of the tab title. On 2013/11/25 18:17:47, creis wrote: > nit: Please move the comment inside the if. Done. https://codereview.chromium.org/78443005/diff/300001/chrome/browser/ui/search... chrome/browser/ui/search/search_tab_helper.cc:231: web_contents_->GetController().GetVisibleEntry(); On 2013/11/25 18:17:47, creis wrote: > Please use GetPendingEntry instead of GetVisibleEntry. It's possible for the > visible entry to be the committed one instead of the pending one (e.g., if we > think a spoof is in progress). > > There will definitely be a pending entry in this case (given how the method is > invoked), but you can include the null check if you'd like. Done. https://codereview.chromium.org/78443005/diff/300001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/78443005/diff/300001/content/browser/web_cont... content/browser/web_contents/web_contents_impl_unittest.cc:324: TEST_F(WebContentsImplTest, UseTitleFromPendingEntry) { On 2013/11/25 18:17:47, creis wrote: > nit: UseTitleFromPendingEntryIfSet Done.
creis is an owner in content and you're an OWNER in c/b/u/s, which means you don't need me anymore: -sky
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/samarth@chromium.org/78443005/310001
Message was sent while issue was closed.
Change committed as 237180 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
