[Navigation Experiment] Add WKBasedNavigationManagerImpl.

[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

Introduced WebViewNavigationProxy protocol to expose navigation related
WKWebView APIs to WKBasedNavigationManagerImpl.

Moved WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

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

[Eugene But (OOO till 7-30), 2017-06-27 22:01:59]

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

[Eugene But (OOO till 7-30), 2017-06-27 22:02:01]

WKBasedNavigationManagerImpl::SetWebViewNavigationProxy suggestion is optional

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_delegate.h:50: virtual
WebViewNavigationProxy* GetWebViewNavigationProxy() const = 0;
Would it make sense to have
WKBasedNavigationManagerImpl::SetWebViewNavigationProxy instead? We probably not
going to use NavigationManagerDelegate inside WKBasedNavigationManagerImpl
anyway, so at some point the delegate may be removed.

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/web_view...
File ios/web/navigation/web_view_navigation_proxy_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/web_view...
ios/web/navigation/web_view_navigation_proxy_impl.mm:48: return
web_view_.backForwardList.currentItem;
nit: No need for |if (web_view_) {| check. it's ok to call methods on nil. They
will return nil.

Same comment for other places where |web_view_| nil check is unnecessary.

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/wk_based...
File ios/web/navigation/wk_based_navigation_manager_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/wk_based...
ios/web/navigation/wk_based_navigation_manager_impl.mm:6: 
FYI: I did not look at this file, which I guess is what you wanted

[danyao, 2017-06-28 00:05:15]

Description was changed from

==========
[Navigation Experiment] Add WKBasedNavigationManagerImpl.

DO NOT REVIEW YET: this is still work-in-progress.

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

to

==========
[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

WebViewNavigationProxy is a new abstraction to expose navigation related methods
and properties on WKWebView to WKBasedNavigationManagerImpl. It will also act as
a testing seam for injecting fake data.

Move WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

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

[danyao, 2017-06-28 00:10:24]

Description was changed from

==========
[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

WebViewNavigationProxy is a new abstraction to expose navigation related methods
and properties on WKWebView to WKBasedNavigationManagerImpl. It will also act as
a testing seam for injecting fake data.

Move WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

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

to

==========
[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

WebViewNavigationProxy is a new abstraction to expose navigation related methods
and properties on WKWebView to WKBasedNavigationManagerImpl. It will also act as
a testing seam for injecting fake data.

Moved WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

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

[danyao, 2017-06-28 00:11:18]

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

[commit-bot: I haz the power, 2017-06-28 00:11:41]

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-28 00:21:59]

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

[commit-bot: I haz the power, 2017-06-28 00:22:00]

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

[danyao, 2017-06-28 22:11:05]

HI Eugene,

This CL is ready for review now. PTAL.

Thanks,
Danyao

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_delegate.h:50: virtual
WebViewNavigationProxy* GetWebViewNavigationProxy() const = 0;
On 2017/06/27 22:02:00, Eugene But wrote:
> Would it make sense to have
> WKBasedNavigationManagerImpl::SetWebViewNavigationProxy instead? We probably
not
> going to use NavigationManagerDelegate inside WKBasedNavigationManagerImpl
> anyway, so at some point the delegate may be removed. 

I prefer this over SetWebViewNavigationProxy for a couple of reasons:
1) We still need NavigationManagerDelegate inside the new navigation manager for
notifying observers.
2) CRWWebController might not always have the same WKWebView object. The getter
provides a convenient place to update the point before giving the proxy to
WKBasedNavigationManagerImpl.

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/web_view...
File ios/web/navigation/web_view_navigation_proxy_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/web_view...
ios/web/navigation/web_view_navigation_proxy_impl.mm:48: return
web_view_.backForwardList.currentItem;
On 2017/06/27 22:02:00, Eugene But wrote:
> nit: No need for |if (web_view_) {| check. it's ok to call methods on nil.
They
> will return nil.
> 
> Same comment for other places where |web_view_| nil check is unnecessary.

Done.

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/wk_based...
File ios/web/navigation/wk_based_navigation_manager_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/wk_based...
ios/web/navigation/wk_based_navigation_manager_impl.mm:6: 
On 2017/06/27 22:02:00, Eugene But wrote:
> FYI: I did not look at this file, which I guess is what you wanted

Thanks. I've stripped down this file to just a skeleton for this CL. Will send
the more substantial implementation in a followup.

[Eugene But (OOO till 7-30), 2017-06-29 01:48:19]

Thank you for splitting the CL!

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/navigati...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2957163002/diff/1/ios/web/navigation/navigati...
ios/web/navigation/navigation_manager_delegate.h:50: virtual
WebViewNavigationProxy* GetWebViewNavigationProxy() const = 0;
On 2017/06/28 22:11:04, danyao wrote:
> On 2017/06/27 22:02:00, Eugene But wrote:
> > Would it make sense to have
> > WKBasedNavigationManagerImpl::SetWebViewNavigationProxy instead? We probably
> not
> > going to use NavigationManagerDelegate inside WKBasedNavigationManagerImpl
> > anyway, so at some point the delegate may be removed. 
> 
> I prefer this over SetWebViewNavigationProxy for a couple of reasons:
> 1) We still need NavigationManagerDelegate inside the new navigation manager
for
> notifying observers.
> 2) CRWWebController might not always have the same WKWebView object. The
getter
> provides a convenient place to update the point before giving the proxy to
> WKBasedNavigationManagerImpl.
It's just somewhat strange that delegate has Get<Something> method (I know it
already has GetWebState, but that's existing code). Not a big deal though.

If we going to keep the delegate, then maybe we should move all methods from
WebViewNavigationProxy to NavigationManagerDelegate? I'm fine with the code as
it is because it is temporary anyway.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_delegate.h:47: // Returns the main
WKWebView used to display web content.
Update comment?

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

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:134: 
Do you want to file a bug and add a TODO to remove all protected methods after
cleaning up legacy navigation code?

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:143: static bool
AreURLsInPageNavigation(const GURL& existing_url,
Could you please rename this to AreUrlsFragmentChangeNavigation

in-page is very confusing and can also mean pushState/replaceState.

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

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:28: enum
NavigationManagerChoice {
Do you want to explain in the comments why we have 2 navigation managers?

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:63: if
(session_controller_) {
No need for this check. It's perfectly fine to call a method on nil.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:91: if (GetParam() ==
TEST_LEGACY_NAVIGATION_MANAGER) {
nit: Should this be switch-case instead of if else?

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
File ios/web/navigation/wk_based_navigation_manager_impl.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.h:28: // WKBackForwardList
based implementation of NavigationManagerImpl.
Do you want to provide more details in the comments? F.e. explain on which
WKWebView API this class relies.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
File ios/web/navigation/wk_based_navigation_manager_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:27:
web::WebViewNavigationProxy* GetWebViewOrDie(
Generally it is uncommon in chromium code to DHECK/CHECK before pointer
dereferencing, so how about just inlining
|delegate->GetWebViewNavigationProxy()| call?

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:42:
WKBasedNavigationManagerImpl::~WKBasedNavigationManagerImpl() {}
nit: WKBasedNavigationManagerImpl::~WKBasedNavigationManagerImpl() = default;

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:171: return
(int)(proxy->GetBackList().count + count_current_page +
Please use C++ casting:

https://google.github.io/styleguide/cppguide.html#Casting

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:277: return std::max(-1,
GetIndexForOffset(-1));
"previous" in this context means "the old" not "current - 1". So you probably
want to use |DLOG(WARNING) << "Not yet implemented.";| here.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
File ios/web/web_state/ui/web_view_navigation_proxy.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_navigation_proxy.h:24: virtual
WKBackForwardListItem* GetCurrentItem() const = 0;
Sorry I just realized that proxy still exposes WK primitives, which I think
defeats the purpose of wrapping WKWebView. WebViewNavigationProxy does not
really help with testing, because WKWebView can be easily mocked and it does not
encapsulate the fact that WKWebView is used. Maybe I'm missing something?

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:92: ? (NavigationManagerImpl*)(new
WKBasedNavigationManagerImpl)
Please use C++ casting. But do you even need to cast here? Can you use
base::MakeUnique instead?

[danyao, 2017-06-29 16:05:26]

Thanks for the review! I addressed all comments, but there's the open question
about whether to keep WebViewNavigationProxy. PTAL.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
File ios/web/navigation/navigation_manager_delegate.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_delegate.h:47: // Returns the main
WKWebView used to display web content.
On 2017/06/29 01:48:19, Eugene But wrote:
> Update comment?

Done.

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

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:134: 
On 2017/06/29 01:48:19, Eugene But wrote:
> Do you want to file a bug and add a TODO to remove all protected methods after
> cleaning up legacy navigation code?

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl.h:143: static bool
AreURLsInPageNavigation(const GURL& existing_url,
On 2017/06/29 01:48:19, Eugene But wrote:
> Could you please rename this to AreUrlsFragmentChangeNavigation
> 
> in-page is very confusing and can also mean pushState/replaceState.

Done.

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

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:28: enum
NavigationManagerChoice {
On 2017/06/29 01:48:19, Eugene But wrote:
> Do you want to explain in the comments why we have 2 navigation managers?

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:63: if
(session_controller_) {
On 2017/06/29 01:48:19, Eugene But wrote:
> No need for this check. It's perfectly fine to call a method on nil.

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/navi...
ios/web/navigation/navigation_manager_impl_unittest.mm:91: if (GetParam() ==
TEST_LEGACY_NAVIGATION_MANAGER) {
On 2017/06/29 01:48:19, Eugene But wrote:
> nit: Should this be switch-case instead of if else?

There are just two cases, and we don't expect more. I think if is a little bit
more compact.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
File ios/web/navigation/wk_based_navigation_manager_impl.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.h:28: // WKBackForwardList
based implementation of NavigationManagerImpl.
On 2017/06/29 01:48:19, Eugene But wrote:
> Do you want to provide more details in the comments? F.e. explain on which
> WKWebView API this class relies.

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
File ios/web/navigation/wk_based_navigation_manager_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:27:
web::WebViewNavigationProxy* GetWebViewOrDie(
On 2017/06/29 01:48:19, Eugene But wrote:
> Generally it is uncommon in chromium code to DHECK/CHECK before pointer
> dereferencing, so how about just inlining
> |delegate->GetWebViewNavigationProxy()| call?

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:42:
WKBasedNavigationManagerImpl::~WKBasedNavigationManagerImpl() {}
On 2017/06/29 01:48:19, Eugene But wrote:
> nit: WKBasedNavigationManagerImpl::~WKBasedNavigationManagerImpl() = default;

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:171: return
(int)(proxy->GetBackList().count + count_current_page +
On 2017/06/29 01:48:19, Eugene But wrote:
> Please use C++ casting:
> 
> https://google.github.io/styleguide/cppguide.html#Casting

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/navigation/wk_b...
ios/web/navigation/wk_based_navigation_manager_impl.mm:277: return std::max(-1,
GetIndexForOffset(-1));
On 2017/06/29 01:48:19, Eugene But wrote:
> "previous" in this context means "the old" not "current - 1". So you probably
> want to use |DLOG(WARNING) << "Not yet implemented.";| here.

Done.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
File ios/web/web_state/ui/web_view_navigation_proxy.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_navigation_proxy.h:24: virtual
WKBackForwardListItem* GetCurrentItem() const = 0;
On 2017/06/29 01:48:19, Eugene But wrote:
> Sorry I just realized that proxy still exposes WK primitives, which I think
> defeats the purpose of wrapping WKWebView. WebViewNavigationProxy does not
> really help with testing, because WKWebView can be easily mocked and it does
not
> encapsulate the fact that WKWebView is used. Maybe I'm missing something?

Ah I didn't realize that I can use OCMockObject. So this proxy is probably not
particularly useful, other than limiting the caller to only the navigation
related calls. I don't think a thicker layer of abstraction makes sense, because
that's essentially what WKBasedNavigationManager is.

I'm tempted to keep this class, because I don't like the alternatives:
- We can expose WKWebView* from CRWWebController, but it makes me uncomfortable
to expose the entire API
- We can push the APIs into NavigationManagerDelegate, but now it adds 5 methods
to several different classes.

The second alternative seems less objectionable to me than the first. What do
you think?

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/web_s...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/web_s...
ios/web/web_state/web_state_impl.mm:92: ? (NavigationManagerImpl*)(new
WKBasedNavigationManagerImpl)
On 2017/06/29 01:48:19, Eugene But wrote:
> Please use C++ casting. But do you even need to cast here? Can you use
> base::MakeUnique instead?

The cast is needed because I want to use the ? : shorthand, which requires the
two arms of : to return the same type.

MakeUnique doesn't work here because we want to selectively make one of the
derived classes. We would still need the conditional selection.

I changed this to a proper if statement to avoid the casting. It's just a little
bit longer. Also tweaked the contract of
SessionStorageBuilder::ExtractSessionState to require the navigation manager to
be already allocated in the passed-in WebStateImpl. This way we only need to do
this conditional instantiation of navigation manager once in this class.

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

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
File ios/web/web_state/ui/web_view_navigation_proxy.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_navigation_proxy.h:13: // Interface to expose
navigation related functionality on WKWebView.
If we end up with keeping the wrapper, could you please explain in the comments
that the purpose of proxy is to limit WKWebView interface.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_navigation_proxy.h:24: virtual
WKBackForwardListItem* GetCurrentItem() const = 0;
On 2017/06/29 16:05:26, danyao wrote:
> On 2017/06/29 01:48:19, Eugene But wrote:
> > Sorry I just realized that proxy still exposes WK primitives, which I think
> > defeats the purpose of wrapping WKWebView. WebViewNavigationProxy does not
> > really help with testing, because WKWebView can be easily mocked and it does
> not
> > encapsulate the fact that WKWebView is used. Maybe I'm missing something?
> 
> Ah I didn't realize that I can use OCMockObject. So this proxy is probably not
> particularly useful, other than limiting the caller to only the navigation
> related calls. I don't think a thicker layer of abstraction makes sense,
because
> that's essentially what WKBasedNavigationManager is.
> 
> I'm tempted to keep this class, because I don't like the alternatives:
> - We can expose WKWebView* from CRWWebController, but it makes me
uncomfortable
> to expose the entire API
> - We can push the APIs into NavigationManagerDelegate, but now it adds 5
methods
> to several different classes.
> 
> The second alternative seems less objectionable to me than the first. What do
> you think?
Yeah, second alternative adds more code to WebController, which is something
that we should try to avoid (so I don't like it either).

WKBasedNavigationManager is a wrapper around WKWebView API. NM is so tightly
coupled with WKWebView API that making a separate abstraction layer does not
really add much. Do you think that exposing whole WKWebView interface to
WKBasedNavigationManager will cause a problems in the future? Maybe instead of
building a wrapper we can do something with Objective-C protocol (protocol will
have small subset of WKWebView APIs) or NSProxy object? I'm almost convinced
that keeping WebViewNavigationProxy wrapper is fine.

[danyao, 2017-06-29 19:31:24]

On 2017/06/29 16:25:04, Eugene But wrote:
>
https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
> File ios/web/web_state/ui/web_view_navigation_proxy.h (right):
> 
>
https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
> ios/web/web_state/ui/web_view_navigation_proxy.h:13: // Interface to expose
> navigation related functionality on WKWebView.
> If we end up with keeping the wrapper, could you please explain in the
comments
> that the purpose of proxy is to limit WKWebView interface.
> 
>
https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
> ios/web/web_state/ui/web_view_navigation_proxy.h:24: virtual
> WKBackForwardListItem* GetCurrentItem() const = 0;
> On 2017/06/29 16:05:26, danyao wrote:
> > On 2017/06/29 01:48:19, Eugene But wrote:
> > > Sorry I just realized that proxy still exposes WK primitives, which I
think
> > > defeats the purpose of wrapping WKWebView. WebViewNavigationProxy does not
> > > really help with testing, because WKWebView can be easily mocked and it
does
> > not
> > > encapsulate the fact that WKWebView is used. Maybe I'm missing something?
> > 
> > Ah I didn't realize that I can use OCMockObject. So this proxy is probably
not
> > particularly useful, other than limiting the caller to only the navigation
> > related calls. I don't think a thicker layer of abstraction makes sense,
> because
> > that's essentially what WKBasedNavigationManager is.
> > 
> > I'm tempted to keep this class, because I don't like the alternatives:
> > - We can expose WKWebView* from CRWWebController, but it makes me
> uncomfortable
> > to expose the entire API
> > - We can push the APIs into NavigationManagerDelegate, but now it adds 5
> methods
> > to several different classes.
> > 
> > The second alternative seems less objectionable to me than the first. What
do
> > you think?
> Yeah, second alternative adds more code to WebController, which is something
> that we should try to avoid (so I don't like it either).
> 
> WKBasedNavigationManager is a wrapper around WKWebView API. NM is so tightly
> coupled with WKWebView API that making a separate abstraction layer does not
> really add much. Do you think that exposing whole WKWebView interface to
> WKBasedNavigationManager will cause a problems in the future? Maybe instead of
> building a wrapper we can do something with Objective-C protocol (protocol
will
> have small subset of WKWebView APIs) or NSProxy object? I'm almost convinced
> that keeping WebViewNavigationProxy wrapper is fine.

I think exposing WKWebView to WKBasedNavigationManager would not be a big
problem due to their tight coupling, but to do so requires exposing WKWebView on
CRWWebController and WebStateImpl. These two classes are used more widely and
I'm more worried about the leakage there. In the long run,
WebViewNavigationProxy can probably be obsorbed into CRWWebController, once the
API surface there is more under control.

Protocol and NSProxy are interesting ideas. Is there a way in Objective-C to
make a framework class conform to a custom defined protocol? If I naively add
the following to CRWWebController:

-(id<WebViewNavigationProtocol>)webViewNavigationProtocol {
  return self.webView;
}

I get a compile error: cannot convert to id<WebViewNavigationProtocol> with
rvalue of type WKWebView*.

NSProxy could work, but it turns accessing a hidden method into a runtime error,
whereas with WebViewNavigationProxy, we get a compile-time error.

I updated the class-level comments for WebViewNavigationProxy in case you think
we can keep it. :)

[danyao, 2017-06-29 21:56:39]

Patchset #8 (id:140001) has been deleted

[danyao, 2017-06-29 21:56:51]

Patchset #7 (id:120001) has been deleted

[danyao, 2017-06-29 21:56:59]

Patchset #6 (id:100001) has been deleted

[danyao, 2017-06-29 21:57:06]

Patchset #5 (id:80001) has been deleted

[danyao, 2017-06-29 21:57:35]

Patchset #2 (id:20001) has been deleted

[danyao, 2017-06-29 21:57:57]

Patchset #2 (id:40001) has been deleted

[danyao, 2017-06-29 22:03:29]

Thanks for the conformsToProtocol tip. It worked! I deleted the wrapper and
switching to using the protocol instead. PTAL.

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
File ios/web/web_state/ui/web_view_navigation_proxy.h (right):

https://codereview.chromium.org/2957163002/diff/60001/ios/web/web_state/ui/we...
ios/web/web_state/ui/web_view_navigation_proxy.h:13: // Interface to expose
navigation related functionality on WKWebView.
On 2017/06/29 16:25:04, Eugene But wrote:
> If we end up with keeping the wrapper, could you please explain in the
comments
> that the purpose of proxy is to limit WKWebView interface.

Acknowledged.

[danyao, 2017-06-29 22:05:37]

Description was changed from

==========
[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

WebViewNavigationProxy is a new abstraction to expose navigation related methods
and properties on WKWebView to WKBasedNavigationManagerImpl. It will also act as
a testing seam for injecting fake data.

Moved WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

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

to

==========
[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

Introduced WebViewNavigationProxy protocol to expose navigation related
WKWebView APIs to WKBasedNavigationManagerImpl.

Moved WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

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

[Eugene But (OOO till 7-30), 2017-06-29 22:28:02]

Generally everything looks great, thank you very much for doing this!  Have a
few minor comments and request for a unittest

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
File ios/web/navigation/wk_based_navigation_manager_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
ios/web/navigation/wk_based_navigation_manager_impl.mm:27:
WKBasedNavigationManagerImpl::WKBasedNavigationManagerImpl()
Could you please add a unit test for this specific file. We can keep
navigation_manager_impl_unittest until LegacyNavigationManager is removed and
write all new tests inside wk_based_navigation_manager_impl_unittest

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
ios/web/navigation/wk_based_navigation_manager_impl.mm:83: return nullptr;
nit: Sorry, missed this previously

s/nullptr/nil

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1431: return
(id<WebViewNavigationProxy>)(self.webView);
Please use C++ casting

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/w...
File ios/web/web_state/ui/web_view_navigation_proxy.h (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/w...
ios/web/web_state/ui/web_view_navigation_proxy.h:13: @protocol
WebViewNavigationProxy
s/WebViewNavigationProxy/CRWWebViewNavigationProxy

Objective-C does not support namespaces, so 3 letter prefix is used to prevent
names collision.

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.mm:86: navigation_manager_ =
base::WrapUnique<NavigationManagerImpl>(
nit: Is it possible to use MakeUnique now?

[danyao, 2017-06-29 23:03:16]

Patchset #4 (id:180001) has been deleted

[danyao, 2017-06-29 23:09:13]

Patchset #4 (id:200001) has been deleted

[danyao, 2017-06-29 23:09:48]

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
File ios/web/navigation/wk_based_navigation_manager_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
ios/web/navigation/wk_based_navigation_manager_impl.mm:27:
WKBasedNavigationManagerImpl::WKBasedNavigationManagerImpl()
On 2017/06/29 22:28:02, Eugene But wrote:
> Could you please add a unit test for this specific file. We can keep
> navigation_manager_impl_unittest until LegacyNavigationManager is removed and
> write all new tests inside wk_based_navigation_manager_impl_unittest

I was planning on adding tests to navigation_manager_impl_unittest so we can
easily verify equivalence between the two implementations. That's why the tests
have been updated in this patch to only use navigation manager APIs. Once all
implementations are filled in (next CL), I'll enable the tests on this class as
well.

Since the two implementations have identical API, and it's usually good to test
only using public APIs, I'm not sure if there's a lot of value adding a separate
unit test.

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
ios/web/navigation/wk_based_navigation_manager_impl.mm:83: return nullptr;
On 2017/06/29 22:28:02, Eugene But wrote:
> nit: Sorry, missed this previously
> 
> s/nullptr/nil 

Done.

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1431: return
(id<WebViewNavigationProxy>)(self.webView);
On 2017/06/29 22:28:02, Eugene But wrote:
> Please use C++ casting

Ah missed this since I thought C++ casting style guides don't apply to
Objective-C objects.

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/w...
File ios/web/web_state/ui/web_view_navigation_proxy.h (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/ui/w...
ios/web/web_state/ui/web_view_navigation_proxy.h:13: @protocol
WebViewNavigationProxy
On 2017/06/29 22:28:02, Eugene But wrote:
> s/WebViewNavigationProxy/CRWWebViewNavigationProxy
> 
> Objective-C does not support namespaces, so 3 letter prefix is used to prevent
> names collision.

Done.

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/web_...
File ios/web/web_state/web_state_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/web_state/web_...
ios/web/web_state/web_state_impl.mm:86: navigation_manager_ =
base::WrapUnique<NavigationManagerImpl>(
On 2017/06/29 22:28:02, Eugene But wrote:
> nit: Is it possible to use MakeUnique now?

Yes. :)

[Eugene But (OOO till 7-30), 2017-06-30 00:59:54]

lgtm!

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
File ios/web/navigation/wk_based_navigation_manager_impl.mm (right):

https://codereview.chromium.org/2957163002/diff/160001/ios/web/navigation/wk_...
ios/web/navigation/wk_based_navigation_manager_impl.mm:27:
WKBasedNavigationManagerImpl::WKBasedNavigationManagerImpl()
On 2017/06/29 23:09:47, danyao wrote:
> On 2017/06/29 22:28:02, Eugene But wrote:
> > Could you please add a unit test for this specific file. We can keep
> > navigation_manager_impl_unittest until LegacyNavigationManager is removed
and
> > write all new tests inside wk_based_navigation_manager_impl_unittest
> 
> I was planning on adding tests to navigation_manager_impl_unittest so we can
> easily verify equivalence between the two implementations. That's why the
tests
> have been updated in this patch to only use navigation manager APIs. Once all
> implementations are filled in (next CL), I'll enable the tests on this class
as
> well.
> 
> Since the two implementations have identical API, and it's usually good to
test
> only using public APIs, I'm not sure if there's a lot of value adding a
separate
> unit test.
If you think you can maintain good coverage for this class and write test code
which works for both classes, then your proposal sounds like a great plan.

https://codereview.chromium.org/2957163002/diff/220001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2957163002/diff/220001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1430: if ([self.webView
conformsToProtocol:@protocol(CRWWebViewNavigationProxy)]) {
Sorry, should have mentioned in the previous round. Should this be a DCHECK
instead?

DCHECK(!self.webView || [self.webView
conformsToProtocol:@protocol(CRWWebViewNavigationProxy)]);

[danyao, 2017-06-30 14:51:43]

Thanks for the thorough review!

https://codereview.chromium.org/2957163002/diff/220001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2957163002/diff/220001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_web_controller.mm:1430: if ([self.webView
conformsToProtocol:@protocol(CRWWebViewNavigationProxy)]) {
On 2017/06/30 00:59:53, Eugene But wrote:
> Sorry, should have mentioned in the previous round. Should this be a DCHECK
> instead?
> 
> DCHECK(!self.webView || [self.webView
> conformsToProtocol:@protocol(CRWWebViewNavigationProxy)]);

Done.

[danyao, 2017-06-30 14:52:03]

The CQ bit was checked by danyao@chromium.org

[danyao, 2017-06-30 14:52:04]

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

[commit-bot: I haz the power, 2017-06-30 14:52:19]

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-30 16:25:13]

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

[commit-bot: I haz the power, 2017-06-30 16:25:14]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[danyao, 2017-06-30 18:06:59]

The CQ bit was checked by danyao@chromium.org

[commit-bot: I haz the power, 2017-06-30 18:07:22]

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-30 18:11:29]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1498846019353490,
"parent_rev": "95f17ec05931807d8bc7af8cdeab5792f8aadde1", "commit_rev":
"fc108a3f8b0a86e98f379d27a8cc525ab1e30b99"}

[commit-bot: I haz the power, 2017-06-30 18:11:47]

Description was changed from

==========
[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

Introduced WebViewNavigationProxy protocol to expose navigation related
WKWebView APIs to WKBasedNavigationManagerImpl.

Moved WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

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

to

==========
[Navigation Experiment] Add skeleton WKBasedNavigationManagerImpl.

Introduced WebViewNavigationProxy protocol to expose navigation related
WKWebView APIs to WKBasedNavigationManagerImpl.

Moved WebLoadParams and AreURLsInPageNavigation from LegacyNavigationManagerImpl
to the parent NavigationManagerImpl to allow sharing with
WKBasedNavigationManagerImpl.

Also replaced CRWSessionController API calls in navigation_manager_impl_unittest
with the equivalent NavigationManager APIs so these unit tests can be shared to
test WKBasedNavigationManagerImpl. A special case is CanGoBackWithTransientItem:
NavigationManager::AddTransientItem() expects a non-null pending (or committed)
item to exist (the session controller version doesn't check this), so updated
the test setup to avoid the error. This matches real-world usage.

BUG=734150

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

[commit-bot: I haz the power, 2017-06-30 18:11:49]

Committed patchset #5 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/fc108a3f8b0a86e98f379d27a8cc...