Handle correctly start and commit of non-latest navigations.

Handle correctly start and commit of non-latest navigations.

Notable changes:
 - didStartProvisionalNavigation: is no op if passed navigation is not the latest 
    navigation (this way didStartProvisionalNavigation: does not mess up with
    pending item)
 - didCommitNavigation: does not commit pending entry if if passed navigation
    is not the latest navigation. Instead it changes last committed item to one
    that matches with URL, which may not be correct, but it's just a minor
    temporary inconsistency which does not have any security implications
    (last committed URL is still correct).

BUG=711465
Review-Url: https://codereview.chromium.org/2821173002
Cr-Commit-Position: refs/heads/master@{#465669}
Committed: https://chromium.googlesource.com/chromium/src/+/79d9b86732316df528fbfe72409465e9bf2668b1

[Eugene But (OOO till 7-30), 2017-04-18 22:39:40]

Description was changed from

==========
Handle correctly start and commit of non-latest navigations.

BUG=711465
==========

to

==========
Handle correctly start and commit of non-latest navigations.

Notable changes:
 - didStartProvisionalNavigation: is no op if passed navigation is not the
latest 
    navigation (this way didStartProvisionalNavigation: does not mess up with
    pending item)
 - didCommitNavigation: does not commit pending entry if if passed navigation
    is not the latest navigation. Instead it changes last committed item to one
    that matches with URL, which may not be correct, but it's just a minor
    temporary inconsistency which does not have any security implications
    (last committed URL is still correct).

BUG=711465
==========

[Eugene But (OOO till 7-30), 2017-04-18 22:39:52]

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

[kkhorimoto, 2017-04-18 23:03:16]

The changes that you mention in the CL description make sense to me, but I don't
understand why we aren't discarding non-committed items for the
multiple-history-navigation case.

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/visible_url_egtest.mm (right):

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/visible_url_egtest.mm:523:
chrome_test_util::RunCommandWithActiveViewController(forwardCommand);
Why are we using commands to go forward but UI elements to go back?  Can you add
a comment explaining the reason?

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/visible_url_egtest.mm:530: [[EarlGrey
selectElementWithMatcher:OmniboxText("chrome://version")]
Can we not use kChromeUIVersionURL?

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2122: _documentURL.spec() ==
url::kAboutBlankURL) /* invalid URL load */)
Some of these are line comments, but some are block comments.  Do you want to
choose one and stick with it?

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4566: _documentURL.spec() ==
url::kAboutBlankURL) /* invalid URL load */);
ditto

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4603: [self.sessionController
goToItemAtIndex:i discardNonCommittedItems:NO];
Could you explain why we aren't discarding the non-committed items here?

[Eugene But (OOO till 7-30), 2017-04-19 00:26:46]

I explained why non-committed items are not discarded in the code comments. Does
it make sense now?

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/visible_url_egtest.mm (right):

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/visible_url_egtest.mm:523:
chrome_test_util::RunCommandWithActiveViewController(forwardCommand);
On 2017/04/18 23:03:15, kkhorimoto_ wrote:
> Why are we using commands to go forward but UI elements to go back?  Can you
add
> a comment explaining the reason?

Done.

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/visible_url_egtest.mm:530: [[EarlGrey
selectElementWithMatcher:OmniboxText("chrome://version")]
On 2017/04/18 23:03:15, kkhorimoto_ wrote:
> Can we not use kChromeUIVersionURL?
No, kChromeUIVersionURL has a trailing slash :(

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:2122: _documentURL.spec() ==
url::kAboutBlankURL) /* invalid URL load */)
On 2017/04/18 23:03:15, kkhorimoto_ wrote:
> Some of these are line comments, but some are block comments.  Do you want to
> choose one and stick with it?

Done.

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4566: _documentURL.spec() ==
url::kAboutBlankURL) /* invalid URL load */);
On 2017/04/18 23:03:15, kkhorimoto_ wrote:
> ditto

Done.

https://codereview.chromium.org/2821173002/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4603: [self.sessionController
goToItemAtIndex:i discardNonCommittedItems:NO];
On 2017/04/18 23:03:15, kkhorimoto_ wrote:
> Could you explain why we aren't discarding the non-committed items here?

Done.

[kkhorimoto, 2017-04-19 00:34:41]

Thanks, the extra comments help. lgtm!

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/visible_url_egtest.mm (right):

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/visible_url_egtest.mm:530: [[EarlGrey
selectElementWithMatcher:OmniboxText("chrome://version")]
On 2017/04/19 00:26:46, Eugene But wrote:
> On 2017/04/18 23:03:15, kkhorimoto_ wrote:
> > Can we not use kChromeUIVersionURL?
> No, kChromeUIVersionURL has a trailing slash :(

I think you can use web::GetDisplayTitleForUrl() to convert to the string that
is used.

https://codereview.chromium.org/2821173002/diff/80001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2821173002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4596: // WKWebView has more than one
in progress navigaiton, and committed
s/navigaiton/navigation

https://codereview.chromium.org/2821173002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4597: // navigation was not the
latests. It is critical to keep last committed
s/latests/latest.

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

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
File ios/chrome/browser/web/visible_url_egtest.mm (right):

https://codereview.chromium.org/2821173002/diff/60001/ios/chrome/browser/web/...
ios/chrome/browser/web/visible_url_egtest.mm:530: [[EarlGrey
selectElementWithMatcher:OmniboxText("chrome://version")]
On 2017/04/19 00:34:41, kkhorimoto_ wrote:
> On 2017/04/19 00:26:46, Eugene But wrote:
> > On 2017/04/18 23:03:15, kkhorimoto_ wrote:
> > > Can we not use kChromeUIVersionURL?
> > No, kChromeUIVersionURL has a trailing slash :(
> 
> I think you can use web::GetDisplayTitleForUrl() to convert to the string that
> is used.
I can. Thanks!

https://codereview.chromium.org/2821173002/diff/80001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (right):

https://codereview.chromium.org/2821173002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4596: // WKWebView has more than one
in progress navigaiton, and committed
On 2017/04/19 00:34:41, kkhorimoto_ wrote:
> s/navigaiton/navigation

Done.

https://codereview.chromium.org/2821173002/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:4597: // navigation was not the
latests. It is critical to keep last committed
On 2017/04/19 00:34:41, kkhorimoto_ wrote:
> s/latests/latest.

Done.

[Eugene But (OOO till 7-30), 2017-04-19 16:56:26]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-04-19 16:56:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
kkhorimoto@chromium.org
Link to the patchset: https://codereview.chromium.org/2821173002/#ps100001
(title: "Use web::GetDisplayTitleForUrl")

[commit-bot: I haz the power, 2017-04-19 16:57:01]

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-04-19 18:07:26]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1492620986932230,
"parent_rev": "70dab6f4ff7ddadd4b1da803bfb0baf93a8f0512", "commit_rev":
"79d9b86732316df528fbfe72409465e9bf2668b1"}

[commit-bot: I haz the power, 2017-04-19 18:07:40]

Description was changed from

==========
Handle correctly start and commit of non-latest navigations.

Notable changes:
 - didStartProvisionalNavigation: is no op if passed navigation is not the
latest 
    navigation (this way didStartProvisionalNavigation: does not mess up with
    pending item)
 - didCommitNavigation: does not commit pending entry if if passed navigation
    is not the latest navigation. Instead it changes last committed item to one
    that matches with URL, which may not be correct, but it's just a minor
    temporary inconsistency which does not have any security implications
    (last committed URL is still correct).

BUG=711465
==========

to

==========
Handle correctly start and commit of non-latest navigations.

Notable changes:
 - didStartProvisionalNavigation: is no op if passed navigation is not the
latest 
    navigation (this way didStartProvisionalNavigation: does not mess up with
    pending item)
 - didCommitNavigation: does not commit pending entry if if passed navigation
    is not the latest navigation. Instead it changes last committed item to one
    that matches with URL, which may not be correct, but it's just a minor
    temporary inconsistency which does not have any security implications
    (last committed URL is still correct).

BUG=711465

Review-Url: https://codereview.chromium.org/2821173002
Cr-Commit-Position: refs/heads/master@{#465669}
Committed:
https://chromium.googlesource.com/chromium/src/+/79d9b86732316df528fbfe724094...
==========

[commit-bot: I haz the power, 2017-04-19 18:07:41]

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