Do not use GetPendingEntryIndex in TabRestoreServiceHelper::PopulateTab.

Do not use GetPendingEntryIndex in TabRestoreServiceHelper::PopulateTab.

TabRestoreServiceHelper::PopulateTab never restores pending item
and there is no need to call GetPendingEntryIndex. This CL removes dead code
and does not change the app behavior. 

Specifically |(entry_count == 0 && pending_index == 0)| can never be true, because
if entry_count is 0, the pending_index is always -1.
Also live_tab->GetEntryAtIndex(i) always return correct value even it is a pending 
entry.

BUG=None

Review-Url: https://codereview.chromium.org/2762363002
Cr-Commit-Position: refs/heads/master@{#459481}
Committed: https://chromium.googlesource.com/chromium/src/+/00544941366aa3b491b9f134b8c4f6b8c2ea880f

[Eugene But (OOO till 7-30), 2017-03-21 21:12:55]

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

[commit-bot: I haz the power, 2017-03-21 21:13:19]

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

[commit-bot: I haz the power, 2017-03-21 22:11:09]

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

[commit-bot: I haz the power, 2017-03-21 22:11:10]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-03-21 22:31:14]

eugenebut@chromium.org changed reviewers:
+ sky@chromium.org

[Eugene But (OOO till 7-30), 2017-03-21 22:31:15]

Hi Scott,

You listed as an owner for components/sessions, so I thought that you would be
the best person to ask this question. 

LiveTab::GetPendingEntryIndex() returns incorrect value on iOS platform. Before
fixing it I wanted to make sure that none of the callers rely on existing bug
and TabRestoreServiceHelper::PopulateTab is one of those callers.

From what I see TabRestoreServiceHelper::PopulateTab never restores pending item
and there is no need to call GetPendingEntryIndex. In this CL I removed some
code, which I think never gets executed. Could you please take a look if these
changes look sane to you and do not change current behavior.

Alternatively we may want to change this code to actually restore pending item,
if this is something that we want.

Thanks!

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
File components/sessions/core/tab_restore_service_helper.cc (left):

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
components/sessions/core/tab_restore_service_helper.cc:416: if (entry_count == 0
&& pending_index == 0)
When entry_count is 0, then pending_index is always -1, so this should never be
true:
https://cs.chromium.org/chromium/src/content/public/browser/navigation_contro...

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
components/sessions/core/tab_restore_service_helper.cc:417:
SerializedNavigationEntry entry = live_tab->GetEntryAtIndex(i);
Because |entry_count| does not include pending item, there is no need to restore
pending item.

[sky, 2017-03-22 03:20:16]

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
components/sessions/core/tab_restore_service_helper.cc:417:
SerializedNavigationEntry entry = live_tab->GetEntryAtIndex(i);
On 2017/03/21 22:31:15, Eugene But wrote:
> Because |entry_count| does not include pending item, there is no need to
restore
> pending item.

Are you sure about that? I thought reload and possibly other things making the
pending index point to an existing one.

[Eugene But (OOO till 7-30), 2017-03-22 14:38:39]

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

[Eugene But (OOO till 7-30), 2017-03-22 14:38:40]

Charlie, do you think my change in this CL is sane and does not change the
behavior?

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
components/sessions/core/tab_restore_service_helper.cc:417:
SerializedNavigationEntry entry = live_tab->GetEntryAtIndex(i);
On 2017/03/22 03:20:16, sky wrote:
> On 2017/03/21 22:31:15, Eugene But wrote:
> > Because |entry_count| does not include pending item, there is no need to
> restore
> > pending item.
> 
> Are you sure about that? I thought reload and possibly other things making the
> pending index point to an existing one.
It is true that reload and back-forward navigation make pending index point to
existing one. However in that case |entry_count| will not be 0, so the previous
2 lines I deleted will not execute. Also |i == pending_index)| will be true iff
item is both bending and committed, so |live_tab->GetEntryAtIndex(i)| and
|live_tab->GetPendingEntry()| will return the same result.

[Charlie Reis, 2017-03-22 22:49:49]

LGTM.  sky@, do you agree with the reasoning below?  (I can't think of a case
that NavigationController's pending_entry_ would not be identical to entries_[i]
when i != -1.)

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
File components/sessions/core/tab_restore_service_helper.cc (left):

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
components/sessions/core/tab_restore_service_helper.cc:416: if (entry_count == 0
&& pending_index == 0)
On 2017/03/21 22:31:14, Eugene But wrote:
> When entry_count is 0, then pending_index is always -1, so this should never
be
> true:
>
https://cs.chromium.org/chromium/src/content/public/browser/navigation_contro...

Yes, I can't think of a case that both entry_count and pending_index would be 0.

I thought about the case of opening a slow URL in a new tab (when entry_count is
0 but there's a pending entry), but that indeed has pending_index -1.  (Note
that we *do* get here in that case, because we don't filter out tabs with zero
entries until AddEntry/FilterEntry/IsTabInteresting after this call.  But
entry_count is zero, so the loop below does nothing.)

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
File components/sessions/core/tab_restore_service_helper.cc (right):

https://codereview.chromium.org/2762363002/diff/1/components/sessions/core/ta...
components/sessions/core/tab_restore_service_helper.cc:417:
SerializedNavigationEntry entry = live_tab->GetEntryAtIndex(i);
On 2017/03/22 14:38:40, Eugene But wrote:
> On 2017/03/22 03:20:16, sky wrote:
> > On 2017/03/21 22:31:15, Eugene But wrote:
> > > Because |entry_count| does not include pending item, there is no need to
> > restore
> > > pending item.
> > 
> > Are you sure about that? I thought reload and possibly other things making
the
> > pending index point to an existing one.
> It is true that reload and back-forward navigation make pending index point to
> existing one. However in that case |entry_count| will not be 0, so the
previous
> 2 lines I deleted will not execute. Also |i == pending_index)| will be true
iff
> item is both bending and committed, so |live_tab->GetEntryAtIndex(i)| and
> |live_tab->GetPendingEntry()| will return the same result.

Yeah, pointing to an existing one is fine-- your CL wouldn't affect that.  It
would only matter if GetPendingEntry() didn't return GetEntryAtIndex(i) when
pending_entry_index_ is i.  That seems like it can only happen if i is -1.

Since i can't be -1 here (given the loop bounds), I think your change is
probably safe.

[sky, 2017-03-22 23:49:50]

Indeed. LGTM

[Eugene But (OOO till 7-30), 2017-03-23 17:19:15]

Description was changed from

==========
Do not use GetPendingEntryIndex in TabRestoreServiceHelper::PopulateTab.

BUG=None
==========

to

==========
Do not use GetPendingEntryIndex in TabRestoreServiceHelper::PopulateTab.

TabRestoreServiceHelper::PopulateTab never restores pending item
and there is no need to call GetPendingEntryIndex. This CL removes dead code
and does not change the app behavior. 

Specifically |(entry_count == 0 && pending_index == 0)| can never be true,
because
if entry_count is 0, the pending_index is always -1.
Also live_tab->GetEntryAtIndex(i) always return correct value even it is a
pending 
entry.

BUG=None
==========

[Eugene But (OOO till 7-30), 2017-03-23 18:27:41]

Thank you! I've updated CL description as initially this CL was only for
discussion, not for landing.

[Eugene But (OOO till 7-30), 2017-03-23 18:27:47]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-03-23 18:28:31]

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

[commit-bot: I haz the power, 2017-03-23 19:03:41]

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

[commit-bot: I haz the power, 2017-03-23 19:03:42]

Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Eugene But (OOO till 7-30), 2017-03-23 20:20:14]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-03-24 17:42:37]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-03-24 17:42:51]

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

[commit-bot: I haz the power, 2017-03-24 18:25:09]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490377357069360, "parent_rev":
"6872e8fc7767fd2edb8752bd55e85330ad5e84c3", "commit_rev":
"00544941366aa3b491b9f134b8c4f6b8c2ea880f"}

[commit-bot: I haz the power, 2017-03-24 18:25:58]

Description was changed from

==========
Do not use GetPendingEntryIndex in TabRestoreServiceHelper::PopulateTab.

TabRestoreServiceHelper::PopulateTab never restores pending item
and there is no need to call GetPendingEntryIndex. This CL removes dead code
and does not change the app behavior. 

Specifically |(entry_count == 0 && pending_index == 0)| can never be true,
because
if entry_count is 0, the pending_index is always -1.
Also live_tab->GetEntryAtIndex(i) always return correct value even it is a
pending 
entry.

BUG=None
==========

to

==========
Do not use GetPendingEntryIndex in TabRestoreServiceHelper::PopulateTab.

TabRestoreServiceHelper::PopulateTab never restores pending item
and there is no need to call GetPendingEntryIndex. This CL removes dead code
and does not change the app behavior. 

Specifically |(entry_count == 0 && pending_index == 0)| can never be true,
because
if entry_count is 0, the pending_index is always -1.
Also live_tab->GetEntryAtIndex(i) always return correct value even it is a
pending 
entry.

BUG=None

Review-Url: https://codereview.chromium.org/2762363002
Cr-Commit-Position: refs/heads/master@{#459481}
Committed:
https://chromium.googlesource.com/chromium/src/+/00544941366aa3b491b9f134b8c4...
==========

[commit-bot: I haz the power, 2017-03-24 18:26:00]

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