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

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

This is a bit of cleanup and extra test coverage before introducing a
NavigationEntry for the initial blank page.

BUG=524208
TEST=No behavior change.

Committed: https://crrev.com/072b61ff751b6a5d73c5c7680d1b342e488d5c2b
Cr-Commit-Position: refs/heads/master@{#351134}

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

creis@chromium.org changed reviewers:
+ avi@chromium.org

[Charlie Reis, 2015-09-28 18:18:44]

Avi, can you take a look?  This was independent of the rest of the changes in
https://codereview.chromium.org/1309323004/, so I thought I'd land it first. 
The main change is using pending entry index instead of checking the existence
of the last committed entry.

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:958:
((controller_.GetVisibleEntry() &&
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]

lgtm

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:959: if (entry ||
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.

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:958:
((controller_.GetVisibleEntry() &&
On 2015/09/28 18:18:43, Charlie Reis wrote:
> 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.

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

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

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:959: if (entry ||
On 2015/09/28 19:11:39, Avi wrote:
> 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.

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.

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1372133002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:958:
((controller_.GetVisibleEntry() &&
On 2015/09/28 19:11:39, Avi wrote:
> On 2015/09/28 18:18:43, Charlie Reis wrote:
> > 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.
> 
> Will you then (please) also kill
> WebContentsImpl::page_title_when_no_navigation_entry_ ?

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

[Charlie Reis, 2015-09-28 19:21:48]

The CQ bit was checked by creis@chromium.org

[commit-bot: I haz the power, 2015-09-28 19:23:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1372133002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1372133002/1

[commit-bot: I haz the power, 2015-09-28 20:52:33]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-09-28 20:53:43]

Patchset 1 (id:??) landed as
https://crrev.com/072b61ff751b6a5d73c5c7680d1b342e488d5c2b
Cr-Commit-Position: refs/heads/master@{#351134}

[Avi (use Gerrit), 2015-09-28 20:56:31]

On 2015/09/28 19:21:41, Charlie Reis wrote:
> One of many fun cases I encountered in the epic https://crbug.com/9682.

Jeez. Can you at least put explanations like that in the comments? It's not
immediately obvious from the code how that could happen.

[Charlie Reis, 2015-09-28 21:08:51]

On 2015/09/28 20:56:31, Avi wrote:
> On 2015/09/28 19:21:41, Charlie Reis wrote:
> > One of many fun cases I encountered in the epic https://crbug.com/9682.
> 
> Jeez. Can you at least put explanations like that in the comments? It's not
> immediately obvious from the code how that could happen.

The explanation was there already; I just restated it to answer your question. 
The old code said:
// 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).	

The new code says:
// 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.

I can try to add more context and a breadcrumb to 9682 in the final version, if
you want.

[Avi (use Gerrit), 2015-09-28 21:12:20]

On 2015/09/28 21:08:51, Charlie Reis wrote:
> On 2015/09/28 20:56:31, Avi wrote:
> > On 2015/09/28 19:21:41, Charlie Reis wrote:
> > > One of many fun cases I encountered in the epic https://crbug.com/9682.
> > 
> > Jeez. Can you at least put explanations like that in the comments? It's not
> > immediately obvious from the code how that could happen.
> 
> The explanation was there already; I just restated it to answer your question.

> The old code said:
> // 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).	
> 
> The new code says:
> // 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.
> 
> I can try to add more context and a breadcrumb to 9682 in the final version,
if
> you want.

Gah. I missed that, complained to you, and now you point out to me what I missed
:) Never mind.