Extract NavigationManagerImpl interface for navigation experiment.

Extract NavigationManagerImpl interface for navigation experiment.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=734150
Review-Url: https://codereview.chromium.org/2944093002
Cr-Commit-Position: refs/heads/master@{#481242}
Committed: https://chromium.googlesource.com/chromium/src/+/9818f983cfc8c8f6455a89ff7f50eccd2bb89669

[danyao, 2017-06-19 22:17:35]

Description was changed from

==========
Extract NavigationManagerImpl interface for navigation experiment.

Moved all real logic of NavigationManagerImpl into LegacyNavigationManagerImpl
so the former is a pure interface that we can use to inject the experimental
navigation manager later. Methods that are not expected to be implemented in the
experimental version are hidden behind GetLegacyInterface() for the time being
so unit tests and CRWWebController continue to work.

BUG=crbug.com/734150
==========

to

==========
Extract NavigationManagerImpl interface for navigation experiment.

Methods that are not expected to be implemented in the new manager are
hidden behind GetLatencyInterface() for the time being to support
existing dependencies.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=crbug.com/734150
==========

[danyao, 2017-06-19 22:21:17]

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

[commit-bot: I haz the power, 2017-06-19 22:21:44]

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-06-20 00:15:13]

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

[commit-bot: I haz the power, 2017-06-20 00:15:14]

Dry run: This issue passed the CQ dry run.

[danyao, 2017-06-20 14:25:33]

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

[danyao, 2017-06-20 14:25:33]

Hi Eugene,

I uploaded this CL here instead of Gerrit in order to run the ios-simulator-eg
tryjob. All tests but one passed, and the failing one has been disabled (for
crbug.com/734856) since I ran the tryjob. So I'm confident that this CL has not
broken anything.

Thanks,
Danyao

[Eugene But (OOO till 7-30), 2017-06-20 16:25:35]

Do we really need GetLegacyInterface() method? I feel that its existence will
lead to creating multiple conditions (if (GetLegacyInterface())) across multiple
files. Would it be better to use polymorphism and inherit
WKNavigationManagerImpl from NavigationManagerImpl? WKNavigationManagerImpl can
override some of WKNavigationManagerImpl mathods as no op, but all other classes
will keep using WKNavigationManagerImpl interface without changing their code.

[danyao, 2017-06-20 21:19:27]

Description was changed from

==========
Extract NavigationManagerImpl interface for navigation experiment.

Methods that are not expected to be implemented in the new manager are
hidden behind GetLatencyInterface() for the time being to support
existing dependencies.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=crbug.com/734150
==========

to

==========
Extract NavigationManagerImpl interface for navigation experiment.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=crbug.com/734150
==========

[danyao, 2017-06-20 21:20:28]

On 2017/06/20 16:25:35, Eugene But wrote:
> Do we really need GetLegacyInterface() method? I feel that its existence will
> lead to creating multiple conditions (if (GetLegacyInterface())) across
multiple
> files. Would it be better to use polymorphism and inherit
> WKNavigationManagerImpl from NavigationManagerImpl? WKNavigationManagerImpl
can
> override some of WKNavigationManagerImpl mathods as no op, but all other
classes
> will keep using WKNavigationManagerImpl interface without changing their code.

My original intent for GetLegacyInterface() was to make dependency on
to-be-deprecated methods super clear (mainly set/get session controller and
GetIndexForOffset). But I see your point about the additional boilerplate.
Updated. PTAL.

[Eugene But (OOO till 7-30), 2017-06-20 22:49:04]

lgtm!

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.h (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:15: #include
"ios/web/navigation/navigation_item_impl.h"
nit: s/include/import

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:52: ~NavigationManagerImpl()
override{};
Drop ; or just |~NavigationManagerImpl() override = default;|

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:1408:
::testing::Values(true /* test_legacy_navigation_manager */));
Is it possible to pass NavigationManagerImpl instance instead of bool?

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_util_unittest.mm (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_util_unittest.mm:86:
::testing::Values(true /* test_legacy_navigation_manager */));
ditto

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
File ios/web/navigation/session_storage_builder.mm (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
ios/web/navigation/session_storage_builder.mm:14: #include
"ios/web/navigation/legacy_navigation_manager_impl.h"
s/include/import

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
ios/web/navigation/session_storage_builder.mm:49:
(NSInteger)navigation_manager->GetPreviousItemIndex();
Per C++ Style Guide:
static_cast<NSInteger>(navigation_manager->GetPreviousItemIndex())

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
ios/web/navigation/session_storage_builder.mm:52: for (size_t index = 0; index <
(size_t)navigation_manager->GetItemCount();
ditto

[danyao, 2017-06-20 23:00:45]

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl.h (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:15: #include
"ios/web/navigation/navigation_item_impl.h"
On 2017/06/20 22:49:03, Eugene But wrote:
> nit: s/include/import

Done.

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:52: ~NavigationManagerImpl()
override{};
On 2017/06/20 22:49:03, Eugene But wrote:
> Drop ; or just |~NavigationManagerImpl() override = default;|

Done.

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:1408:
::testing::Values(true /* test_legacy_navigation_manager */));
On 2017/06/20 22:49:03, Eugene But wrote:
> Is it possible to pass NavigationManagerImpl instance instead of bool?

It has to be a copyable type. We could use raw pointers, but then we'd have to
separately manage the lifespan of the underlying object. Passing a bool is a lot
easier...

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_util_unittest.mm (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_util_unittest.mm:86:
::testing::Values(true /* test_legacy_navigation_manager */));
On 2017/06/20 22:49:03, Eugene But wrote:
> ditto

Please see comment in previous file.

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
File ios/web/navigation/session_storage_builder.mm (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
ios/web/navigation/session_storage_builder.mm:14: #include
"ios/web/navigation/legacy_navigation_manager_impl.h"
On 2017/06/20 22:49:04, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
ios/web/navigation/session_storage_builder.mm:49:
(NSInteger)navigation_manager->GetPreviousItemIndex();
On 2017/06/20 22:49:03, Eugene But wrote:
> Per C++ Style Guide:
> static_cast<NSInteger>(navigation_manager->GetPreviousItemIndex())

Done.

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/sess...
ios/web/navigation/session_storage_builder.mm:52: for (size_t index = 0; index <
(size_t)navigation_manager->GetItemCount();
On 2017/06/20 22:49:03, Eugene But wrote:
> ditto

Done.

[danyao, 2017-06-20 23:15:01]

The CQ bit was checked by danyao@chromium.org

[danyao, 2017-06-20 23:15:02]

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/2944093002/#ps100001
(title: "Patch for landing")

[commit-bot: I haz the power, 2017-06-20 23:15:21]

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-06-20 23:31:07]

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

[commit-bot: I haz the power, 2017-06-20 23:31:08]

Try jobs failed on following builders:
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)

[kkhorimoto, 2017-06-20 23:41:34]

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

[kkhorimoto, 2017-06-20 23:41:35]

lgtm! This is exciting work!  Can you change the bug= line on the CL description
to just have the number instead of the crbug.com link?

[Eugene But (OOO till 7-30), 2017-06-21 00:15:37]

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_impl_unittest.mm (right):

https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:1408:
::testing::Values(true /* test_legacy_navigation_manager */));
On 2017/06/20 23:00:45, danyao wrote:
> On 2017/06/20 22:49:03, Eugene But wrote:
> > Is it possible to pass NavigationManagerImpl instance instead of bool?
> 
> It has to be a copyable type. We could use raw pointers, but then we'd have to
> separately manage the lifespan of the underlying object. Passing a bool is a
lot
> easier...
Thanks! I thought it is possible to pass std::unique_ptr using move constructor.

[danyao, 2017-06-21 03:48:28]

On 2017/06/21 00:15:37, Eugene But wrote:
>
https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
> File ios/web/navigation/navigation_manager_impl_unittest.mm (right):
> 
>
https://codereview.chromium.org/2944093002/diff/80001/ios/web/navigation/navi...
> ios/web/navigation/navigation_manager_impl_unittest.mm:1408:
> ::testing::Values(true /* test_legacy_navigation_manager */));
> On 2017/06/20 23:00:45, danyao wrote:
> > On 2017/06/20 22:49:03, Eugene But wrote:
> > > Is it possible to pass NavigationManagerImpl instance instead of bool?
> > 
> > It has to be a copyable type. We could use raw pointers, but then we'd have
to
> > separately manage the lifespan of the underlying object. Passing a bool is a
> lot
> > easier...
> Thanks! I thought it is possible to pass std::unique_ptr using move
constructor.

std::unique_ptr is movable but not copyable.

[danyao, 2017-06-21 03:48:44]

Description was changed from

==========
Extract NavigationManagerImpl interface for navigation experiment.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=crbug.com/734150
==========

to

==========
Extract NavigationManagerImpl interface for navigation experiment.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=734150
==========

[danyao, 2017-06-21 03:51:13]

On 2017/06/20 23:41:35, kkhorimoto_ wrote:
> lgtm! This is exciting work!  Can you change the bug= line on the CL
description
> to just have the number instead of the http://crbug.com link?

Done!

[danyao, 2017-06-21 17:08:29]

The CQ bit was checked by danyao@chromium.org

[commit-bot: I haz the power, 2017-06-21 17:08: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-06-21 18:14:15]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1498064909684540,
"parent_rev": "81095e19266ad3da6479a5889793842fe49a437b", "commit_rev":
"9818f983cfc8c8f6455a89ff7f50eccd2bb89669"}

[commit-bot: I haz the power, 2017-06-21 18:14:29]

Description was changed from

==========
Extract NavigationManagerImpl interface for navigation experiment.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=734150
==========

to

==========
Extract NavigationManagerImpl interface for navigation experiment.

Added GetNavigationItemImplAtIndex() and GetPreviousItemIndex() as
private pass-through APIs on NavigationManagerImpl and removed direct
access to CRWSessionController in SessionStorageBuilder::BuildStorage().

BUG=734150

Review-Url: https://codereview.chromium.org/2944093002
Cr-Commit-Position: refs/heads/master@{#481242}
Committed:
https://chromium.googlesource.com/chromium/src/+/9818f983cfc8c8f6455a89ff7f50...
==========

[commit-bot: I haz the power, 2017-06-21 18:14:32]

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