Remove the concept of currentItemIndex

Remove the concept of currentItemIndex

In NavigationManager and SessionController, the concept of current item
index is simply a duplicate of last committed item, and what's worse,
this results in GetCurrentItemIndex != GetItemIndex(GetCurrentItem), 
which is counter-intuitive.

This CL removes the concept of currentItemIndex and converts callers to
use lastCommittedItemIndex instead.

This CL will diverges us from web content, but I think it's totally
worth it given that APIs containing "current" have become quite
untrustworthy, and removing this API will make our web code less
confusing.

NOTE: this CL doesn't touch the concept of currentItem.

BUG=703863
Review-Url: https://codereview.chromium.org/2766063002
Cr-Commit-Position: refs/heads/master@{#459097}
Committed: https://chromium.googlesource.com/chromium/src/+/c833f1b7fee45abebea16b04eeaf62d198af3157

[liaoyuke, 2017-03-21 23:40:36]

liaoyuke@chromium.org changed reviewers:
+ eugenebut@chromium.org, kkhorimoto@chromium.org

[liaoyuke, 2017-03-21 23:40:37]

Hey Eugene, Kurt,

PTAL.

Thank you very much!

[kkhorimoto, 2017-03-21 23:54:17]

In addition to my comments, could you also replace |_currentNavigationIndex|
with |_lastCommittedItemIndex| in CRWSessionController

https://codereview.chromium.org/2766063002/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list_unittest.mm (left):

https://codereview.chromium.org/2766063002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_unittest.mm:116: int
GetLastCommittedItemIndex() const override { return current_item_index_; }
As long as you're touching this, can we
s/current_item_index_/last_committed_item_index_?

https://codereview.chromium.org/2766063002/diff/1/ios/web/public/test/fakes/t...
File ios/web/public/test/fakes/test_navigation_manager.mm (left):

https://codereview.chromium.org/2766063002/diff/1/ios/web/public/test/fakes/t...
ios/web/public/test/fakes/test_navigation_manager.mm:86: void
TestNavigationManager::SetCurrentItemIndex(const int index) {
Let's also change this to SetLastCommittedItemIndex()

[liaoyuke, 2017-03-22 18:37:01]

Description was changed from

==========
Remove GetCurrentItemIndex in NavigationManager

In NavigationManager, |GetCurrentItemIndex| has the exactly same
functionality as |GetLastCommittedItemIndex|, which is confusing.

This CL removes |GetCurrentItemIndex| and converts callers to call
|GetLastCommittedItemIndex| instead.

This CL will diverges us from web content, but I think it's totally
worth it given that APIs containing "current" have become quite
untrustworthy, and removing this API will make our web code less
confusing.

BUG=703863
==========

to

==========
Remove the concept of currentItemIndex

In NavigationManager and SessionController, the concept of current item
index is simply a duplicate of last committed item, and what's worse,
this results in GetCurrentItemIndex != GetItemIndex(GetCurrentItem), 
which is counter-intuitive.

This CL removes the concept of currentItemIndex and converts callers to
use lastCommittedItemIndex instead.

This CL will diverges us from web content, but I think it's totally
worth it given that APIs containing "current" have become quite
untrustworthy, and removing this API will make our web code less
confusing.

BUG=703863
==========

[liaoyuke, 2017-03-22 18:38:16]

Description was changed from

==========
Remove the concept of currentItemIndex

In NavigationManager and SessionController, the concept of current item
index is simply a duplicate of last committed item, and what's worse,
this results in GetCurrentItemIndex != GetItemIndex(GetCurrentItem), 
which is counter-intuitive.

This CL removes the concept of currentItemIndex and converts callers to
use lastCommittedItemIndex instead.

This CL will diverges us from web content, but I think it's totally
worth it given that APIs containing "current" have become quite
untrustworthy, and removing this API will make our web code less
confusing.

BUG=703863
==========

to

==========
Remove the concept of currentItemIndex

In NavigationManager and SessionController, the concept of current item
index is simply a duplicate of last committed item, and what's worse,
this results in GetCurrentItemIndex != GetItemIndex(GetCurrentItem), 
which is counter-intuitive.

This CL removes the concept of currentItemIndex and converts callers to
use lastCommittedItemIndex instead.

This CL will diverges us from web content, but I think it's totally
worth it given that APIs containing "current" have become quite
untrustworthy, and removing this API will make our web code less
confusing.

NOTE: this CL doesn't touch the concept of currentItem.

BUG=703863
==========

[liaoyuke, 2017-03-22 18:39:08]

Patchset #2 (id:20001) has been deleted

[liaoyuke, 2017-03-22 18:50:12]

Patchset #2 (id:40001) has been deleted

[liaoyuke, 2017-03-22 18:50:37]

PTAL.

Thank you very much!

https://codereview.chromium.org/2766063002/diff/1/ios/shared/chrome/browser/t...
File ios/shared/chrome/browser/tabs/web_state_list_unittest.mm (left):

https://codereview.chromium.org/2766063002/diff/1/ios/shared/chrome/browser/t...
ios/shared/chrome/browser/tabs/web_state_list_unittest.mm:116: int
GetLastCommittedItemIndex() const override { return current_item_index_; }
On 2017/03/21 23:54:16, kkhorimoto_ wrote:
> As long as you're touching this, can we
> s/current_item_index_/last_committed_item_index_?

Done.

https://codereview.chromium.org/2766063002/diff/1/ios/web/public/test/fakes/t...
File ios/web/public/test/fakes/test_navigation_manager.mm (left):

https://codereview.chromium.org/2766063002/diff/1/ios/web/public/test/fakes/t...
ios/web/public/test/fakes/test_navigation_manager.mm:86: void
TestNavigationManager::SetCurrentItemIndex(const int index) {
On 2017/03/21 23:54:16, kkhorimoto_ wrote:
> Let's also change this to SetLastCommittedItemIndex()

Done.

[Eugene But (OOO till 7-30), 2017-03-22 20:19:13]

Thanks for doing this! lgtm

https://codereview.chromium.org/2766063002/diff/60001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2766063002/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:492: // |sourceLastCommittedIndex|,
the |source|'s current item, and |self|'s
s/sourceLastCommittedIndex/sourceLastCommittedItemIndex

[liaoyuke, 2017-03-22 22:35:08]

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

[commit-bot: I haz the power, 2017-03-22 22:36:08]

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

[liaoyuke, 2017-03-22 22:40:22]

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

[liaoyuke, 2017-03-22 22:40:23]

Thank you for reviewing!

Hi Scott,

Could you please do a quick owner's approval?

Thanks!

https://codereview.chromium.org/2766063002/diff/60001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2766063002/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:492: // |sourceLastCommittedIndex|,
the |source|'s current item, and |self|'s
On 2017/03/22 20:19:13, Eugene But wrote:
> s/sourceLastCommittedIndex/sourceLastCommittedItemIndex

Done.

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

LGTM

[liaoyuke, 2017-03-22 23:59:04]

Thank you for the quick response!

On Wed, Mar 22, 2017 at 4:49 PM <sky@chromium.org> wrote:

> LGTM
>
> https://codereview.chromium.org/2766063002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[commit-bot: I haz the power, 2017-03-23 00:17:56]

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

[commit-bot: I haz the power, 2017-03-23 00:17:57]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[liaoyuke, 2017-03-23 05:46:17]

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

[commit-bot: I haz the power, 2017-03-23 05:46:30]

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-23 06:02:21]

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

[commit-bot: I haz the power, 2017-03-23 06:02:22]

Dry run: This issue passed the CQ dry run.

[liaoyuke, 2017-03-23 15:48:34]

The CQ bit was checked by liaoyuke@chromium.org

[liaoyuke, 2017-03-23 15:48:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org, sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2766063002/#ps100001
(title: "fix unit tests")

[commit-bot: I haz the power, 2017-03-23 15:49:00]

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 15:56:41]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1490284114736860,
"parent_rev": "ad1bc7494fbd31e58569ae50362563e87306e5f5", "commit_rev":
"c833f1b7fee45abebea16b04eeaf62d198af3157"}

[commit-bot: I haz the power, 2017-03-23 15:58:13]

Description was changed from

==========
Remove the concept of currentItemIndex

In NavigationManager and SessionController, the concept of current item
index is simply a duplicate of last committed item, and what's worse,
this results in GetCurrentItemIndex != GetItemIndex(GetCurrentItem), 
which is counter-intuitive.

This CL removes the concept of currentItemIndex and converts callers to
use lastCommittedItemIndex instead.

This CL will diverges us from web content, but I think it's totally
worth it given that APIs containing "current" have become quite
untrustworthy, and removing this API will make our web code less
confusing.

NOTE: this CL doesn't touch the concept of currentItem.

BUG=703863
==========

to

==========
Remove the concept of currentItemIndex

In NavigationManager and SessionController, the concept of current item
index is simply a duplicate of last committed item, and what's worse,
this results in GetCurrentItemIndex != GetItemIndex(GetCurrentItem), 
which is counter-intuitive.

This CL removes the concept of currentItemIndex and converts callers to
use lastCommittedItemIndex instead.

This CL will diverges us from web content, but I think it's totally
worth it given that APIs containing "current" have become quite
untrustworthy, and removing this API will make our web code less
confusing.

NOTE: this CL doesn't touch the concept of currentItem.

BUG=703863

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

[commit-bot: I haz the power, 2017-03-23 15:58:14]

Committed patchset #4 (id:100001) as
https://chromium.googlesource.com/chromium/src/+/c833f1b7fee45abebea16b04eeaf...