Created NavigationManager::CopyStateFromAndPrune().

Created NavigationManager::CopyStateFromAndPrune().

This mirrors NavigationController's analogous API.

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

[kkhorimoto, 2017-03-10 22:50:50]

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

[Eugene But (OOO till 7-30), 2017-03-11 00:27:48]

https://codereview.chromium.org/2745873002/diff/1/ios/web/public/navigation_m...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2745873002/diff/1/ios/web/public/navigation_m...
ios/web/public/navigation_manager.h:166: virtual void
InsertStateFromManager(const NavigationManager* manager) = 0;
Per my previous comments can we mirror content? Maybe CopyStateFromAndPrune is
the right API?

[kkhorimoto, 2017-03-13 23:37:45]

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

[commit-bot: I haz the power, 2017-03-13 23:38: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-13 23:42:12]

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

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

Dry run: Try jobs failed on following builders:
  android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
  ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[kkhorimoto, 2017-03-15 18:28:10]

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

[kkhorimoto, 2017-03-15 18:28:42]

PTAL

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

https://codereview.chromium.org/2745873002/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:558: // Tests inserting
session controller state to empty session controller.
These tests are no longer needed after copying NavigationControllerImpl's
behavior around CanPruneAllButLastCommittedItem().

[commit-bot: I haz the power, 2017-03-15 18:28:54]

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-15 18:44:03]

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

[commit-bot: I haz the power, 2017-03-15 18:44:04]

Dry run: This issue passed the CQ dry run.

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

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

https://codereview.chromium.org/2745873002/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:558: // Tests inserting
session controller state to empty session controller.
On 2017/03/15 18:28:42, kkhorimoto_ wrote:
> These tests are no longer needed after copying NavigationControllerImpl's
> behavior around CanPruneAllButLastCommittedItem().
First test checks insertion into empty SessionController. This test is still
useful, because insertion into empty session controller is handled as no op in
the new code and it should not crash.

Second tests tests that pending index is still valid, which sounds to me like a
useful thing to test.

Am I missing something here?

https://codereview.chromium.org/2745873002/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2745873002/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3681: BOOL canPruneItems =
s/BOOL/bool

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

https://codereview.chromium.org/2745873002/diff/80001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:507: return;
Can we even get here? if there is no |lastCommittedItem| then
|self.currentNavigationIndex| will be -1 and |canPruneAllButLastCommittedItem|
will return NO, unless I'm missing something.

https://codereview.chromium.org/2745873002/diff/80001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:513:
web::ScopedNavigationItemImplList mergedItems(sourceCurrentIndex + 2);
Why +2 ?

https://codereview.chromium.org/2745873002/diff/80001/ios/web/public/navigati...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2745873002/diff/80001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:187: virtual bool
CanPruneAllButLastCommittedItem() const = 0;
Please add tests for this new method.

[kkhorimoto, 2017-03-15 21:04:31]

Description was changed from

==========
Created NavigationManager::InsertStateFromManager().

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

to

==========
Created NavigationManager::CopyStateFromAndPrune().

This mirrors NavigationController's analogous API.

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

[kkhorimoto, 2017-03-15 21:04:39]

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

https://codereview.chromium.org/2745873002/diff/60001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller_unittest.mm:558: // Tests inserting
session controller state to empty session controller.
On 2017/03/15 19:54:50, Eugene But wrote:
> On 2017/03/15 18:28:42, kkhorimoto_ wrote:
> > These tests are no longer needed after copying NavigationControllerImpl's
> > behavior around CanPruneAllButLastCommittedItem().
> First test checks insertion into empty SessionController. This test is still
> useful, because insertion into empty session controller is handled as no op in
> the new code and it should not crash.
> 
> Second tests tests that pending index is still valid, which sounds to me like
a
> useful thing to test.
> 
> Am I missing something here?

CanPruneAllButLastCommittedItem() returns false while there's a pending history
navigation, so it will be a no-op.  I've added tests for all three conditions in
which this will be a no-op.

https://codereview.chromium.org/2745873002/diff/80001/ios/chrome/browser/ui/b...
File ios/chrome/browser/ui/browser_view_controller.mm (right):

https://codereview.chromium.org/2745873002/diff/80001/ios/chrome/browser/ui/b...
ios/chrome/browser/ui/browser_view_controller.mm:3681: BOOL canPruneItems =
On 2017/03/15 19:54:51, Eugene But wrote:
> s/BOOL/bool

Done.

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

https://codereview.chromium.org/2745873002/diff/80001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:507: return;
On 2017/03/15 19:54:51, Eugene But wrote:
> Can we even get here? if there is no |lastCommittedItem| then
> |self.currentNavigationIndex| will be -1 and |canPruneAllButLastCommittedItem|
> will return NO, unless I'm missing something.

We're checking |canPruneAllButLastCommittedItem| on |self|, but we're checking
|source| here.  As far as I can tell, our current codebase never has an instance
where the item count is non-zero (condition above), but the current navigation
index is -1, but it's still technically possible given the CRWSessionController
interface.

https://codereview.chromium.org/2745873002/diff/80001/ios/web/navigation/crw_...
ios/web/navigation/crw_session_controller.mm:513:
web::ScopedNavigationItemImplList mergedItems(sourceCurrentIndex + 2);
On 2017/03/15 19:54:51, Eugene But wrote:
> Why +2 ?

Big enough for |source|'s session up to and including the current item
(|sourceCurrentIndex| + 1), plus |self|'s current item ( + 1).  I've added a
comment.

https://codereview.chromium.org/2745873002/diff/80001/ios/web/public/navigati...
File ios/web/public/navigation_manager.h (right):

https://codereview.chromium.org/2745873002/diff/80001/ios/web/public/navigati...
ios/web/public/navigation_manager.h:187: virtual bool
CanPruneAllButLastCommittedItem() const = 0;
On 2017/03/15 19:54:51, Eugene But wrote:
> Please add tests for this new method.

The CRWSessionController implementation is already tested by its unittests. 
I'll move the tests and the implementations to NavigationManager at the same
time.

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

Thanks for more tests. lgtm

https://codereview.chromium.org/2745873002/diff/100001/ios/web/navigation/crw...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2745873002/diff/100001/ios/web/navigation/crw...
ios/web/navigation/crw_session_controller.mm:521: mergedItems.back() =
std::move(_items[self.currentNavigationIndex]);
Should this be push_back?

[kkhorimoto, 2017-03-15 23:28:33]

https://codereview.chromium.org/2745873002/diff/100001/ios/web/navigation/crw...
File ios/web/navigation/crw_session_controller.mm (right):

https://codereview.chromium.org/2745873002/diff/100001/ios/web/navigation/crw...
ios/web/navigation/crw_session_controller.mm:521: mergedItems.back() =
std::move(_items[self.currentNavigationIndex]);
On 2017/03/15 23:22:10, Eugene But wrote:
> Should this be push_back?

No, push_back() will add an extra pointer at the end, whereas using the back()
reference will reset the last unique_ptr to the moved value.

[kkhorimoto, 2017-03-15 23:28:47]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-03-15 23:29:25]

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-15 23:44:59]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1489620527968620,
"parent_rev": "f454af4e7e194ee671928790dea1e443319e86bc", "commit_rev":
"d804c573275af73443053a7e848ce43e655009bb"}

[commit-bot: I haz the power, 2017-03-15 23:45:51]

Description was changed from

==========
Created NavigationManager::CopyStateFromAndPrune().

This mirrors NavigationController's analogous API.

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

to

==========
Created NavigationManager::CopyStateFromAndPrune().

This mirrors NavigationController's analogous API.

BUG=454984

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

[commit-bot: I haz the power, 2017-03-15 23:45:52]

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