Updated ownership of NavigationItems within CRWSessionController.

Updated ownership of NavigationItems within CRWSessionController.

After this CL, CRWSessionController owns a list of NavigationItemImpl
objects.  The CRWSessionEntries returned by its interface contain weak
references to the NavigationItems.

BUG=454984
Review-Url: https://codereview.chromium.org/2672953005
Cr-Commit-Position: refs/heads/master@{#449812}
Committed: https://chromium.googlesource.com/chromium/src/+/6e8bfa3991a15be001ac300469bc5e7c49e34d57

[kkhorimoto, 2017-02-04 00:01:45]

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

[kkhorimoto, 2017-02-04 00:01:45]

This CL includes logic changes from https://codereview.chromium.org/2672723003/

[Eugene But (OOO till 7-30), 2017-02-04 01:50:19]

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm
(right):

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:35: void
SetUp() override {
nit: Do you want to move this code to constructor? It's quite easy to miss
super::SetUp call.

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:56: void
TearDown() override {
nit: Move to destructor?

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:114:
std::string longURL =
s/longURL/long_url

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

https://codereview.chromium.org/2672953005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:80: NSMutableArray* _entries;
Managing both lists is very error prone. Can we only keep _items and have
|entries| method which dynamically builds the array?

https://codereview.chromium.org/2672953005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:81:
std::unique_ptr<web::NavigationItemImpl> _pendingItem;
Please add comments. This ivar is different from the property.

https://codereview.chromium.org/2672953005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:83: SessionEntryMap
_sessionEntryMap;
Can we ditch this and create new CRWSessionEntry with NavigationItemImpl every
time we need it?

[kkhorimoto, 2017-02-07 06:58:04]

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
File ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm
(right):

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:35: void
SetUp() override {
On 2017/02/04 01:50:18, Eugene But wrote:
> nit: Do you want to move this code to constructor? It's quite easy to miss
> super::SetUp call.

Done.

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:56: void
TearDown() override {
On 2017/02/04 01:50:19, Eugene But wrote:
> nit: Move to destructor?

Done.

https://codereview.chromium.org/2672953005/diff/20001/ios/chrome/browser/ui/h...
ios/chrome/browser/ui/history/tab_history_popup_controller_unittest.mm:114:
std::string longURL =
On 2017/02/04 01:50:18, Eugene But wrote:
> s/longURL/long_url

Done.

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

https://codereview.chromium.org/2672953005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:80: NSMutableArray* _entries;
On 2017/02/04 01:50:19, Eugene But wrote:
> Managing both lists is very error prone. Can we only keep _items and have
> |entries| method which dynamically builds the array?

Done.

https://codereview.chromium.org/2672953005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:81:
std::unique_ptr<web::NavigationItemImpl> _pendingItem;
On 2017/02/04 01:50:19, Eugene But wrote:
> Please add comments. This ivar is different from the property.

Done.

https://codereview.chromium.org/2672953005/diff/20001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:83: SessionEntryMap
_sessionEntryMap;
On 2017/02/04 01:50:19, Eugene But wrote:
> Can we ditch this and create new CRWSessionEntry with NavigationItemImpl every
> time we need it?

Yeah, that'd be easier.  I originally wanted to keep a 1:1 relationship between
CRWSessionEntries and their respective NavigationItems, but since
CRWSessionEntries are simply wrappers around NavigationItems at this point,
there really isn't a great reason to not just lazily instantiate them when the
accessors are called.

[Eugene But (OOO till 7-30), 2017-02-07 17:44:23]

lgtm

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

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:76: // |_pendingItem| only contains
a NavigationItem for non-history navigations.
nit: Maybe extend comment with "when pendingEntryIndex == -1" ?

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:259: web::NavigationItemList items;
Construct with size?

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:268: web::NavigationItemList items;
ditto

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:482:
self.pendingItem->ResetForCommit();
Is there a reason for calling |ResetForCommit| earlier?

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:585: for (NSInteger i = 0; i <=
lastIndexToCopy; ++i)
Can you insert all items at once? Maybe like this:

_items.insert(_items.begin(), sourceItems.begin(), sourceItems.end());

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:594: // Update |sourceSession|'s
state to reflect the ownership transfer of its
Invalidating |sourceSession| is quite unexpected side effect. Do you want to
copy the items instead?

[kkhorimoto, 2017-02-08 02:22:35]

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

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:76: // |_pendingItem| only contains
a NavigationItem for non-history navigations.
On 2017/02/07 17:44:23, Eugene But wrote:
> nit: Maybe extend comment with "when pendingEntryIndex == -1" ?

Done.

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:259: web::NavigationItemList items;
On 2017/02/07 17:44:23, Eugene But wrote:
> Construct with size?

The removal of redirect items makes it impossible to calculate the size before
iterating through the list, unfortunately.

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:268: web::NavigationItemList items;
On 2017/02/07 17:44:23, Eugene But wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:482:
self.pendingItem->ResetForCommit();
On 2017/02/07 17:44:23, Eugene But wrote:
> Is there a reason for calling |ResetForCommit| earlier?

Previously, |_pendingEntry| still contained a reference to the session entry
that was committed.  After this CL, |self.pendingItem| performs logic in the
getter, so resetting for commit after moving the unique_ptrs around and updating
the state would not work here.  I could have changed it so that we instead call
ResetForCommit() on the last committed item later, but it seemed more logical to
call it before the code below, which is when the item is actually "committed".

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:585: for (NSInteger i = 0; i <=
lastIndexToCopy; ++i)
On 2017/02/07 17:44:23, Eugene But wrote:
> Can you insert all items at once? Maybe like this:
> 
> _items.insert(_items.begin(), sourceItems.begin(), sourceItems.end());

Not possible now that we're creating copies of the source items.

https://codereview.chromium.org/2672953005/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:594: // Update |sourceSession|'s
state to reflect the ownership transfer of its
On 2017/02/07 17:44:23, Eugene But wrote:
> Invalidating |sourceSession| is quite unexpected side effect. Do you want to
> copy the items instead?

Done.

[kkhorimoto, 2017-02-08 02:22:39]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-08 02:22:39]

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/2672953005/#ps80001
(title: "Eugene's comments")

[commit-bot: I haz the power, 2017-02-08 02:23:23]

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-02-08 02:57:49]

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

[commit-bot: I haz the power, 2017-02-08 02:57:50]

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

[kkhorimoto, 2017-02-10 23:48:37]

fix unittests

[kkhorimoto, 2017-02-10 23:48:56]

The CQ bit was checked by kkhorimoto@chromium.org

[kkhorimoto, 2017-02-10 23:48:56]

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/2672953005/#ps100001
(title: "fix unittests")

[commit-bot: I haz the power, 2017-02-10 23:49:48]

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-02-11 01:08:23]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1486770536282680,
"parent_rev": "7682c4d0bc1f5ccde371417af4bf96e64a94f4ed", "commit_rev":
"6e8bfa3991a15be001ac300469bc5e7c49e34d57"}

[commit-bot: I haz the power, 2017-02-11 01:10:13]

Description was changed from

==========
Updated ownership of NavigationItems within CRWSessionController.

After this CL, CRWSessionController owns a list of NavigationItemImpl
objects.  The CRWSessionEntries returned by its interface contain weak
references to the NavigationItems.

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

to

==========
Updated ownership of NavigationItems within CRWSessionController.

After this CL, CRWSessionController owns a list of NavigationItemImpl
objects.  The CRWSessionEntries returned by its interface contain weak
references to the NavigationItems.

BUG=454984

Review-Url: https://codereview.chromium.org/2672953005
Cr-Commit-Position: refs/heads/master@{#449812}
Committed:
https://chromium.googlesource.com/chromium/src/+/6e8bfa3991a15be001ac300469bc...
==========

[commit-bot: I haz the power, 2017-02-11 01:10:14]

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