Remove CRWSessionEntry.

Remove CRWSessionEntry.

Since all clients of CRWSessionController have been updated to use
NavigationItems, it is now safe to remove CRWSessionEntry.  This CL
updates CRWSessionController implementation to directly own
NavigationItems rather than owning them transitively through
CRWSessionEntry.

BUG=454984
Review-Url: https://codereview.chromium.org/2737203002
Cr-Commit-Position: refs/heads/master@{#455923}
Committed: https://chromium.googlesource.com/chromium/src/+/8c2eb6f1104b23158bb846bd6d4f8bdfa8125a56

[kkhorimoto, 2017-03-09 02:56:42]

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

[commit-bot: I haz the power, 2017-03-09 02:57:02]

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

[kkhorimoto, 2017-03-09 02:58:48]

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

[kkhorimoto, 2017-03-09 02:58:49]

Reland of my previous CL: https://codereview.chromium.org/2672953005

The code is mostly the same except for changes due to other CLs having landed. 
The difference is that this time, all the callers have already been converted to
use NavigationItems instead of CRWSessionEntries, so the transition should be
smoother this time.

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

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

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

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...)

[Eugene But (OOO till 7-30), 2017-03-09 16:10:58]

Woohoo!

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (left):

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:146: // Prior to M34, 0 was used as
"no index" instead of -1; adjust for that.
This comment still looks like a valid one, because the code adjusts for 0. Maybe
remove both code and adjusting code?

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:433: _pendingEntry.reset();
Do we need to clear the reference to pendingItem?

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

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:63:
std::unique_ptr<web::NavigationItemImpl> _transientItem;
Old comments for transientEntry looked useful. Is there a reason for removing
them?

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:288: new
web::NavigationItemImpl(*self.items[index].get()));
nit: Do you need |.get()|?

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:288: new
web::NavigationItemImpl(*self.items[index].get()));
nit: s/new/base::MakeUnique

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:403:
_navigationManager->OnNavigationItemsPruned(self.items.size() -
Calling OnNavigationItemsPruned before removing items does not feel right.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:521: new
web::NavigationItemImpl(*sourceItems[i].get()));
nit: s/new/base::MakeUnique

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller_unittest.mm (left):

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:312: // Setup committed
item.
Why? I think this test suppose to commit if there is no pending item, but one
committed item exists.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller_unittest.mm (right):

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:120:
TEST_F(CRWSessionControllerTest, addPendingItemWithExisingPendingItemIndex) {
Revert back? Exising vs Existing?

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:975:
web::NavigationItemImpl* visible_item = [session_controller_ visibleItem];
Why? Using web::NavigationItem* seems like a sufficient thing.

Same comment for other places.

[kkhorimoto, 2017-03-09 21:33:48]

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

[kkhorimoto, 2017-03-09 21:33:54]

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller.mm (left):

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:146: // Prior to M34, 0 was used as
"no index" instead of -1; adjust for that.
On 2017/03/09 16:10:58, Eugene But wrote:
> This comment still looks like a valid one, because the code adjusts for 0.
Maybe
> remove both code and adjusting code?

I removed the empty() check.  The same result is accomplished in the min()
below.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:433: _pendingEntry.reset();
On 2017/03/09 16:10:58, Eugene But wrote:
> Do we need to clear the reference to pendingItem?

Nope, _pendingItem only holds a pointer for non-history navigations, which would
enter the condition above.  The std::move() will replace |_pendingItem| with a
nullptr.  I've added a DCHECK here that the ivar is indeed null at this point
though.

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

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:63:
std::unique_ptr<web::NavigationItemImpl> _transientItem;
On 2017/03/09 16:10:58, Eugene But wrote:
> Old comments for transientEntry looked useful. Is there a reason for removing
> them?

I've moved the comment to the interface property declaration.  Since the ivars
are an implementation detail, I prefer to use comments in the interface.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:288: new
web::NavigationItemImpl(*self.items[index].get()));
On 2017/03/09 16:10:58, Eugene But wrote:
> nit: s/new/base::MakeUnique

Nope, turns out unique_ptr overrides the * operator.  Updated to MakeUnique as
well.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:403:
_navigationManager->OnNavigationItemsPruned(self.items.size() -
On 2017/03/09 16:10:58, Eugene But wrote:
> Calling OnNavigationItemsPruned before removing items does not feel right.

Fixed

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:521: new
web::NavigationItemImpl(*sourceItems[i].get()));
On 2017/03/09 16:10:58, Eugene But wrote:
> nit: s/new/base::MakeUnique

Done.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller_unittest.mm (left):

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:312: // Setup committed
item.
On 2017/03/09 16:10:58, Eugene But wrote:
> Why? I think this test suppose to commit if there is no pending item, but one
> committed item exists.

Yep you're right.  I had fixed this, but must have accidentally lost the change.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
File ios/web/navigation/crw_session_controller_unittest.mm (right):

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:120:
TEST_F(CRWSessionControllerTest, addPendingItemWithExisingPendingItemIndex) {
On 2017/03/09 16:10:58, Eugene But wrote:
> Revert back? Exising vs Existing?

Done.

https://codereview.chromium.org/2737203002/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:975:
web::NavigationItemImpl* visible_item = [session_controller_ visibleItem];
On 2017/03/09 16:10:58, Eugene But wrote:
> Why? Using web::NavigationItem* seems like a sufficient thing.
> 
> Same comment for other places.

I've updated to use the more appropriate class.  Note that some cases still
require NavigationItemImpl, such as when tracking IsCreatedFromPushState() info.

[commit-bot: I haz the power, 2017-03-09 21:34:18]

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-09 21:50:51]

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

[commit-bot: I haz the power, 2017-03-09 21:50:52]

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

[Eugene But (OOO till 7-30), 2017-03-09 22:07:35]

lgtm!

[kkhorimoto, 2017-03-09 23:04:40]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-03-09 23:04:41]

The patchset sent to the CQ was uploaded after l-g-t-m from
eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2737203002/#ps60001
(title: "fix XCode-clang build")

[commit-bot: I haz the power, 2017-03-09 23:05:15]

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-10 00:31:44]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489100680796130,
"parent_rev": "98fd3234c0100a359e8bfbaf32d164a092b12a31", "commit_rev":
"8c2eb6f1104b23158bb846bd6d4f8bdfa8125a56"}

[commit-bot: I haz the power, 2017-03-10 00:33:33]

Description was changed from

==========
Remove CRWSessionEntry.

Since all clients of CRWSessionController have been updated to use
NavigationItems, it is now safe to remove CRWSessionEntry.  This CL
updates CRWSessionController implementation to directly own
NavigationItems rather than owning them transitively through
CRWSessionEntry.

BUG=454984
==========

to

==========
Remove CRWSessionEntry.

Since all clients of CRWSessionController have been updated to use
NavigationItems, it is now safe to remove CRWSessionEntry.  This CL
updates CRWSessionController implementation to directly own
NavigationItems rather than owning them transitively through
CRWSessionEntry.

BUG=454984

Review-Url: https://codereview.chromium.org/2737203002
Cr-Commit-Position: refs/heads/master@{#455923}
Committed:
https://chromium.googlesource.com/chromium/src/+/8c2eb6f1104b23158bb846bd6d4f...
==========

[commit-bot: I haz the power, 2017-03-10 00:33:34]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/8c2eb6f1104b23158bb846bd6d4f...