Fix flickering in new tab title after r444582.

Fix flickering in new tab title after r444582.

DidStartNavigation is called too late to prevent flickering with PlzNavigate disabled. So bring back overriding of DidStartNavigationToPendingEntry for the non-PlzNavigate case.

BUG=682774
Review-Url: https://codereview.chromium.org/2644963004
Cr-Commit-Position: refs/heads/master@{#445055}
Committed: https://chromium.googlesource.com/chromium/src/+/b1c7a2ae38ea453f101480ab91ee98e6acebf56f

[jam, 2017-01-20 06:45:10]

The CQ bit was checked by jam@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-20 06:45:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-20 06:45:38]

jam@chromium.org changed reviewers:
+ treib@chromium.org

[commit-bot: I haz the power, 2017-01-20 08:08:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-20 08:08:12]

Dry run: This issue passed the CQ dry run.

[Marc Treib, 2017-01-20 09:26:47]

LGTM with nits

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_tab_helper.cc:228: // case.
Is there a bug you could reference here?
(I'm seeing myself a few months in the future, wondering whether it's safe to
delete this now)

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_tab_helper.cc:232: if (search::IsNTPURL(url,
profile())) {
Put this block into a new helper, so it doesn't have to be repeated?

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_tab_helper.cc:246: if
(!content::IsBrowserSideNavigationEnabled())
Once the non-PlzNavigate hack above is gone, this check needs to be removed too,
right? Please add a comment saying that.

[jam, 2017-01-20 15:09:14]

The CQ bit was checked by jam@chromium.org

[commit-bot: I haz the power, 2017-01-20 15:09:21]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jam, 2017-01-20 15:11:35]

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_tab_helper.cc:228: // case.
On 2017/01/20 09:26:47, Marc Treib wrote:
> Is there a bug you could reference here?
> (I'm seeing myself a few months in the future, wondering whether it's safe to
> delete this now)

When PlzNavigate is turned on, we will delete this method and all usages in the
codebase (i.e. no need to label each usage).

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_tab_helper.cc:232: if (search::IsNTPURL(url,
profile())) {
On 2017/01/20 09:26:47, Marc Treib wrote:
> Put this block into a new helper, so it doesn't have to be repeated?

given that this is only 3 lines of code, I thought it's ok to have it repeated.
Otherwise if I create a function for that now, it'll get folded back in a few
months anyways when this code is removed.

https://codereview.chromium.org/2644963004/diff/1/chrome/browser/ui/search/se...
chrome/browser/ui/search/search_tab_helper.cc:246: if
(!content::IsBrowserSideNavigationEnabled())
On 2017/01/20 09:26:47, Marc Treib wrote:
> Once the non-PlzNavigate hack above is gone, this check needs to be removed
too,
> right? Please add a comment saying that.

(same as first comment)

[Marc Treib, 2017-01-20 15:12:01]

On 2017/01/20 15:09:21, commit-bot: I haz the power wrote:
> CQ is trying da patch. Follow status at
>  
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

I added some (small) comments above - did you see them?

[commit-bot: I haz the power, 2017-01-20 15:12:48]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484924954511280, "parent_rev":
"643d85c8fd7fb89a8aea1af3f9ac0dc2eb7ba89e", "commit_rev":
"b1c7a2ae38ea453f101480ab91ee98e6acebf56f"}

[Marc Treib, 2017-01-20 15:13:12]

On 2017/01/20 15:12:01, Marc Treib wrote:
> On 2017/01/20 15:09:21, commit-bot: I haz the power wrote:
> > CQ is trying da patch. Follow status at
> >  
> >
>
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> 
> I added some (small) comments above - did you see them?

Ah, and now your answers arrived. Thanks, carry on :)

[commit-bot: I haz the power, 2017-01-20 15:13:18]

Description was changed from

==========
Fix flickering in new tab title after r444582.

DidStartNavigation is called too late to prevent flickering with PlzNavigate
disabled. So bring back overriding of DidStartNavigationToPendingEntry for the
non-PlzNavigate case.

BUG=682774
==========

to

==========
Fix flickering in new tab title after r444582.

DidStartNavigation is called too late to prevent flickering with PlzNavigate
disabled. So bring back overriding of DidStartNavigationToPendingEntry for the
non-PlzNavigate case.

BUG=682774

Review-Url: https://codereview.chromium.org/2644963004
Cr-Commit-Position: refs/heads/master@{#445055}
Committed:
https://chromium.googlesource.com/chromium/src/+/b1c7a2ae38ea453f101480ab91ee...
==========

[commit-bot: I haz the power, 2017-01-20 15:13:19]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/b1c7a2ae38ea453f101480ab91ee...