Adds NavigationManager util functions to ios/chrome/browser/web/

Adds NavigationManager util functions to ios/chrome/browser/web/

NavigationManager public API is rich enough for most operations. This
is where utility functions built on top of NavigationManager public
API can live and can gradually phase out uses of private
CRWSessionController APIs.

BUG=546365
Review-Url: https://codereview.chromium.org/2756483006
Cr-Commit-Position: refs/heads/master@{#457917}
Committed: https://chromium.googlesource.com/chromium/src/+/eb4147db405401fe0a01527c8b3a1a8a75e68843

[pkl (ping after 24h if needed), 2017-03-17 00:56:07]

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

[pkl (ping after 24h if needed), 2017-03-17 00:56:08]

This is the first of several CLs to remove GetLastUserItem() and GetItems(). I
will also be migrating native_app_launcher::IsLinkNavigation() to
navigation_manager_util.mm as well.

[kkhorimoto, 2017-03-17 01:18:02]

lgtm

https://codereview.chromium.org/2756483006/diff/100001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util.mm (right):

https://codereview.chromium.org/2756483006/diff/100001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.mm:19: // Skips navigation item
if it is for a kind of HTTP Redirect.
"Skips" is a little misleading; I would expect a continue rather than a break
from reading this comment.  Maybe s/Skips/Returns?

[pkl (ping after 24h if needed), 2017-03-17 01:25:08]

https://codereview.chromium.org/2756483006/diff/100001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util.mm (right):

https://codereview.chromium.org/2756483006/diff/100001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.mm:19: // Skips navigation item
if it is for a kind of HTTP Redirect.
On 2017/03/17 01:18:01, kkhorimoto_ wrote:
> "Skips" is a little misleading; I would expect a continue rather than a break
> from reading this comment.  Maybe s/Skips/Returns?

It's not just misleading...it's wrong. Thanks for catching this.
I changed the code, but not the comment. Corrected now.

[Eugene But (OOO till 7-30), 2017-03-17 14:32:04]

lgtm

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:108: #include
"ios/chrome/browser/web/navigation_manager_util.h"
s/include/import

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1512: web::NavigationItem* lastUserItem =
nit: s/lastUserItem/lastNonRedirectedItem ?

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util.h (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.h:14: // Redirect. Returns
nullptr if |nav_manager| is empty or the first item if
It's quite unexpected that GetLastNonRedirectedItem can actually return a
redirect item. Is there a reason for that?

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util.mm (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.mm:5: #include
"ios/chrome/browser/web/navigation_manager_util.h"
s/include/import

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util_unittest.mm (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util_unittest.mm:5: #include
"ios/chrome/browser/web/navigation_manager_util.h"
s/include/import

[kkhorimoto, 2017-03-17 17:16:58]

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util.h (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.h:14: // Redirect. Returns
nullptr if |nav_manager| is empty or the first item if
On 2017/03/17 14:32:04, Eugene But wrote:
> It's quite unexpected that GetLastNonRedirectedItem can actually return a
> redirect item. Is there a reason for that?

The only use case of |-lastUserItem| was for reloading a page using a new user
agent, so if we traverse the whole history and the last one is a REDIRECT type,
we still return it because that's the best idea we have for the URL to attempt
to reload.  You're right that this is unexpected behavior given the new naming. 
Maybe we should just return nullptr in that case, then handle a null return
value when we're actually doing the reload.

[pkl (ping after 24h if needed), 2017-03-17 20:23:53]

Thank you! And CQ'ing.

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/tab...
File ios/chrome/browser/tabs/tab.mm (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:108: #include
"ios/chrome/browser/web/navigation_manager_util.h"
On 2017/03/17 14:32:04, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/tab...
ios/chrome/browser/tabs/tab.mm:1512: web::NavigationItem* lastUserItem =
On 2017/03/17 14:32:04, Eugene But wrote:
> nit: s/lastUserItem/lastNonRedirectedItem ?

Done.

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util.h (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.h:14: // Redirect. Returns
nullptr if |nav_manager| is empty or the first item if
On 2017/03/17 17:16:58, kkhorimoto_ wrote:
> On 2017/03/17 14:32:04, Eugene But wrote:
> > It's quite unexpected that GetLastNonRedirectedItem can actually return a
> > redirect item. Is there a reason for that?
> 
> The only use case of |-lastUserItem| was for reloading a page using a new user
> agent, so if we traverse the whole history and the last one is a REDIRECT
type,
> we still return it because that's the best idea we have for the URL to attempt
> to reload.  You're right that this is unexpected behavior given the new
naming. 
> Maybe we should just return nullptr in that case, then handle a null return
> value when we're actually doing the reload.

I'm inclined to preserve the existing behavior and then clean up separately.

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.h:14: // Redirect. Returns
nullptr if |nav_manager| is empty or the first item if
On 2017/03/17 14:32:04, Eugene But wrote:
> It's quite unexpected that GetLastNonRedirectedItem can actually return a
> redirect item. Is there a reason for that?

Acknowledged.

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util.mm (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util.mm:5: #include
"ios/chrome/browser/web/navigation_manager_util.h"
On 2017/03/17 14:32:04, Eugene But wrote:
> s/include/import

Done.

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
File ios/chrome/browser/web/navigation_manager_util_unittest.mm (right):

https://codereview.chromium.org/2756483006/diff/140001/ios/chrome/browser/web...
ios/chrome/browser/web/navigation_manager_util_unittest.mm:5: #include
"ios/chrome/browser/web/navigation_manager_util.h"
On 2017/03/17 14:32:04, Eugene But wrote:
> s/include/import

Done.

[pkl (ping after 24h if needed), 2017-03-17 20:46:22]

The CQ bit was checked by pkl@chromium.org

[pkl (ping after 24h if needed), 2017-03-17 20:46:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org, eugenebut@chromium.org
Link to the patchset: https://codereview.chromium.org/2756483006/#ps160001
(title: "changed #include to #import and minor renaming.")

[commit-bot: I haz the power, 2017-03-17 20:47:05]

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-17 22:48:26]

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

[commit-bot: I haz the power, 2017-03-17 22:48:27]

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

[pkl (ping after 24h if needed), 2017-03-17 23:22:11]

The CQ bit was checked by pkl@chromium.org

[commit-bot: I haz the power, 2017-03-17 23:23: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-18 01:22:48]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489792931623270,
"parent_rev": "473cc7b571d4ac209d31778effbe21977ac325b7", "commit_rev":
"635ecf5c59770bfcba6bc757c68608123ec7752c"}

[commit-bot: I haz the power, 2017-03-18 01:28:04]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1489792931623270,
"parent_rev": "ece0dcecece6cc6c9bd6f7edcd53ab5179073f2a", "commit_rev":
"eb4147db405401fe0a01527c8b3a1a8a75e68843"}

[commit-bot: I haz the power, 2017-03-18 01:29:03]

Description was changed from

==========
Adds NavigationManager util functions to ios/chrome/browser/web/

NavigationManager public API is rich enough for most operations. This
is where utility functions built on top of NavigationManager public
API can live and can gradually phase out uses of private
CRWSessionController APIs.

BUG=546365
==========

to

==========
Adds NavigationManager util functions to ios/chrome/browser/web/

NavigationManager public API is rich enough for most operations. This
is where utility functions built on top of NavigationManager public
API can live and can gradually phase out uses of private
CRWSessionController APIs.

BUG=546365

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

[commit-bot: I haz the power, 2017-03-18 01:29:04]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/eb4147db405401fe0a01527c8b3a...