[ios] Correctly determine last seen WKNavigation object.

[ios] Correctly determine last seen WKNavigation object.

When double tapping on Forward navigation button the following happens:
- loadRequest: is called for the first Forward Tap (WKNavigation #1)
- loadRequest: is called for the second Forward Tap (WKNavigation #2)
- didStartProvisionalNavigation: is called for WKNavigation #1
- didFailProvisionalNavigation: is called for WKNavigation #1

CRWWebController used to record WKNavigation #1 as latest navigation
in didStartProvisionalNavigation: overriding WKNavigation #2.

Later inside didFailProvisionalNavigation: CRWWebController cancelled
the load because WKNavigation #1 was determined as latest.

This CL replaces _latestWKNavigation object with CRWWKNavigationStates
object which allows better tracking of latest WKNavigation object.

BUG=677327
Committed: https://crrev.com/5f16753d7bb612d6afb2e290df9299a34a01a696
Cr-Commit-Position: refs/heads/master@{#441287}

[Eugene But (OOO till 7-30), 2016-12-29 02:12:13]

Description was changed from

==========
[ios] Correctly determine last seen WKNavigation object.

When douple tapping on Forward navigation button the following happens:
loadRequest: is called for the first Forward Tap (WKNavigation #1)
loadRequest: is called for the second Forward Tap (WKNavigation #2)
didStartProvisionalNavigation: is called for WKNavigation #1
didFailProvisionalNavigation: is called for WKNavigation #1

CRWWebController used to record WKNavigation #1 as latest navigation
in didStartProvisionalNavigation: overriding WKNavigation #2.

Later inside didFailProvisionalNavigation: CRWWebController cancelled
the load because WKNavigation #1 was determined as latest.


BUG=677327
==========

to

==========
[ios] Correctly determine last seen WKNavigation object.

When double tapping on Forward navigation button the following happens:
- loadRequest: is called for the first Forward Tap (WKNavigation #1)
- loadRequest: is called for the second Forward Tap (WKNavigation #2)
- didStartProvisionalNavigation: is called for WKNavigation #1
- didFailProvisionalNavigation: is called for WKNavigation #1

CRWWebController used to record WKNavigation #1 as latest navigation
in didStartProvisionalNavigation: overriding WKNavigation #2.

Later inside didFailProvisionalNavigation: CRWWebController cancelled
the load because WKNavigation #1 was determined as latest.

This CL replaces _latestWKNavigation object with CRWWKNavigationStates
object which allows better tracking of latest WKNavigation object.

BUG=677327
==========

[Eugene But (OOO till 7-30), 2016-12-29 02:17:15]

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

[commit-bot: I haz the power, 2016-12-29 02:17:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Eugene But (OOO till 7-30), 2016-12-29 02:18:50]

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

[Eugene But (OOO till 7-30), 2016-12-29 02:18:51]

https://codereview.chromium.org/2601083003/diff/40001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_web_controller.mm (left):

https://codereview.chromium.org/2601083003/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:3589: if
(self.sessionController.pendingEntryIndex != -1 &&
This is not needed with better tracking of last WKNavigation object

[commit-bot: I haz the power, 2016-12-29 02:24:13]

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

[commit-bot: I haz the power, 2016-12-29 02:24:14]

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 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...)

[Eugene But (OOO till 7-30), 2016-12-29 17:37:13]

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

[commit-bot: I haz the power, 2016-12-29 17:37:27]

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, 2016-12-29 18:37:54]

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

[commit-bot: I haz the power, 2016-12-29 18:37:55]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2016-12-29 19:38:07]

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

[commit-bot: I haz the power, 2016-12-29 19:38:19]

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, 2016-12-29 20:32:36]

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

[commit-bot: I haz the power, 2016-12-29 20:32:36]

Dry run: This issue passed the CQ dry run.

[kkhorimoto, 2017-01-03 22:29:31]

looks good, but have some nits.

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_navigation_states.h (right):

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_navigation_states.h:14: enum WKNavigationState {
Optional nit: Unless we're using integer logic/bitmasking, I prefer using enum
classes over plain enums, since they add type safety and are a little nicer
style-wise.

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_navigation_states.h:47: -
(void)addNavigation:(WKNavigation*)navigation
Can we rename this to |-setState:forNavigation:|?  That makes it more similar to
NSMutableDictionary/NSMapTable's API, which seems more in line with the behavior
of this function.  It's difficult to understand the nuances of this function's
behavior and hits effect on |-lastAddedNavigation| without reading these
comments thoroughly, and I think that the name change will help.

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

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_navigation_states.mm:97: NSUInteger lastAddedIndex =
0;  // record indices srart with 1.
s/srart/start

[Eugene But (OOO till 7-30), 2017-01-04 00:09:21]

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

[Eugene But (OOO till 7-30), 2017-01-04 00:09:27]

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
File ios/web/web_state/ui/crw_wk_navigation_states.h (right):

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_navigation_states.h:14: enum WKNavigationState {
On 2017/01/03 22:29:31, kkhorimoto_ wrote:
> Optional nit: Unless we're using integer logic/bitmasking, I prefer using enum
> classes over plain enums, since they add type safety and are a little nicer
> style-wise.

Done.

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_navigation_states.h:47: -
(void)addNavigation:(WKNavigation*)navigation
On 2017/01/03 22:29:31, kkhorimoto_ wrote:
> Can we rename this to |-setState:forNavigation:|?  That makes it more similar
to
> NSMutableDictionary/NSMapTable's API, which seems more in line with the
behavior
> of this function.  It's difficult to understand the nuances of this function's
> behavior and hits effect on |-lastAddedNavigation| without reading these
> comments thoroughly, and I think that the name change will help.
Good point. Done.

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

https://codereview.chromium.org/2601083003/diff/80001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_wk_navigation_states.mm:97: NSUInteger lastAddedIndex =
0;  // record indices srart with 1.
On 2017/01/03 22:29:31, kkhorimoto_ wrote:
> s/srart/start

Done.

[commit-bot: I haz the power, 2017-01-04 00:10:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[kkhorimoto, 2017-01-04 00:13:58]

lgtm with a couple nits

https://codereview.chromium.org/2601083003/diff/100001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_wk_navigation_states.h (right):

https://codereview.chromium.org/2601083003/diff/100001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_wk_navigation_states.h:44: // should have either
WKNavigationState::REQUESTED or WKNavigationState::STARTED
Just noticed now, but this should be updated to also include
WKNavigationState::COMMITTED, since that is also allowed in your DCHECK.

https://codereview.chromium.org/2601083003/diff/100001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_wk_navigation_states.h:55: // Returns state of the
given navigation or 0 if navigation does not exist.
What is returned here using enum classes?  Not sure what default value would be
returned; we might want to add a NONE = 0 to be used for this scenario.

[Eugene But (OOO till 7-30), 2017-01-04 00:20:11]

Thanks!

https://codereview.chromium.org/2601083003/diff/100001/ios/web/web_state/ui/c...
File ios/web/web_state/ui/crw_wk_navigation_states.h (right):

https://codereview.chromium.org/2601083003/diff/100001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_wk_navigation_states.h:44: // should have either
WKNavigationState::REQUESTED or WKNavigationState::STARTED
On 2017/01/04 00:13:57, kkhorimoto_ wrote:
> Just noticed now, but this should be updated to also include
> WKNavigationState::COMMITTED, since that is also allowed in your DCHECK.

Done.

https://codereview.chromium.org/2601083003/diff/100001/ios/web/web_state/ui/c...
ios/web/web_state/ui/crw_wk_navigation_states.h:55: // Returns state of the
given navigation or 0 if navigation does not exist.
On 2017/01/04 00:13:57, kkhorimoto_ wrote:
> What is returned here using enum classes?  Not sure what default value would
be
> returned; we might want to add a NONE = 0 to be used for this scenario.
0 of int type. Added |NONE = 0|.

[Eugene But (OOO till 7-30), 2017-01-04 00:20:15]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-01-04 00:20:15]

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/2601083003/#ps120001
(title: "Addressed more comments")

[commit-bot: I haz the power, 2017-01-04 00:20:41]

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-01-04 01:35:42]

CQ is committing da patch.

Bot data: {"patchset_id": 120001, "attempt_start_ts": 1483489215460270,
"parent_rev": "6227a9756c6eea4cd5b00c0ebf2920e84822ce43", "commit_rev":
"29ea9f78911ec99a18ad65d1e7f03481261114bc"}

[commit-bot: I haz the power, 2017-01-04 01:36:15]

Description was changed from

==========
[ios] Correctly determine last seen WKNavigation object.

When double tapping on Forward navigation button the following happens:
- loadRequest: is called for the first Forward Tap (WKNavigation #1)
- loadRequest: is called for the second Forward Tap (WKNavigation #2)
- didStartProvisionalNavigation: is called for WKNavigation #1
- didFailProvisionalNavigation: is called for WKNavigation #1

CRWWebController used to record WKNavigation #1 as latest navigation
in didStartProvisionalNavigation: overriding WKNavigation #2.

Later inside didFailProvisionalNavigation: CRWWebController cancelled
the load because WKNavigation #1 was determined as latest.

This CL replaces _latestWKNavigation object with CRWWKNavigationStates
object which allows better tracking of latest WKNavigation object.

BUG=677327
==========

to

==========
[ios] Correctly determine last seen WKNavigation object.

When double tapping on Forward navigation button the following happens:
- loadRequest: is called for the first Forward Tap (WKNavigation #1)
- loadRequest: is called for the second Forward Tap (WKNavigation #2)
- didStartProvisionalNavigation: is called for WKNavigation #1
- didFailProvisionalNavigation: is called for WKNavigation #1

CRWWebController used to record WKNavigation #1 as latest navigation
in didStartProvisionalNavigation: overriding WKNavigation #2.

Later inside didFailProvisionalNavigation: CRWWebController cancelled
the load because WKNavigation #1 was determined as latest.

This CL replaces _latestWKNavigation object with CRWWKNavigationStates
object which allows better tracking of latest WKNavigation object.

BUG=677327

Review-Url: https://codereview.chromium.org/2601083003
==========

[commit-bot: I haz the power, 2017-01-04 01:36:17]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2017-01-04 01:38:26]

Description was changed from

==========
[ios] Correctly determine last seen WKNavigation object.

When double tapping on Forward navigation button the following happens:
- loadRequest: is called for the first Forward Tap (WKNavigation #1)
- loadRequest: is called for the second Forward Tap (WKNavigation #2)
- didStartProvisionalNavigation: is called for WKNavigation #1
- didFailProvisionalNavigation: is called for WKNavigation #1

CRWWebController used to record WKNavigation #1 as latest navigation
in didStartProvisionalNavigation: overriding WKNavigation #2.

Later inside didFailProvisionalNavigation: CRWWebController cancelled
the load because WKNavigation #1 was determined as latest.

This CL replaces _latestWKNavigation object with CRWWKNavigationStates
object which allows better tracking of latest WKNavigation object.

BUG=677327

Review-Url: https://codereview.chromium.org/2601083003
==========

to

==========
[ios] Correctly determine last seen WKNavigation object.

When double tapping on Forward navigation button the following happens:
- loadRequest: is called for the first Forward Tap (WKNavigation #1)
- loadRequest: is called for the second Forward Tap (WKNavigation #2)
- didStartProvisionalNavigation: is called for WKNavigation #1
- didFailProvisionalNavigation: is called for WKNavigation #1

CRWWebController used to record WKNavigation #1 as latest navigation
in didStartProvisionalNavigation: overriding WKNavigation #2.

Later inside didFailProvisionalNavigation: CRWWebController cancelled
the load because WKNavigation #1 was determined as latest.

This CL replaces _latestWKNavigation object with CRWWKNavigationStates
object which allows better tracking of latest WKNavigation object.

BUG=677327

Committed: https://crrev.com/5f16753d7bb612d6afb2e290df9299a34a01a696
Cr-Commit-Position: refs/heads/master@{#441287}
==========

[commit-bot: I haz the power, 2017-01-04 01:38:27]

Patchset 7 (id:??) landed as
https://crrev.com/5f16753d7bb612d6afb2e290df9299a34a01a696
Cr-Commit-Position: refs/heads/master@{#441287}