Cleaned up old navigation code that did not use pending navigation item.

Cleaned up old navigation code that did not use pending navigation item.

Also removed PendingIndexNavigationDisabled experimental setting.

BUG=692331, 701717
TEST=Back forward navigation works correctly.

Review-Url: https://codereview.chromium.org/2751793002
Cr-Commit-Position: refs/heads/master@{#457459}
Committed: https://chromium.googlesource.com/chromium/src/+/2feaa058530953a2232e6128e16deb31192ad9d2

[Eugene But (OOO till 7-30), 2017-03-14 20:50:25]

Description was changed from

==========
Cleaned up old navigation code that did not use pending navigation item.

BUG=None
TEST=Back forward navigation works correctly.
==========

to

==========
Cleaned up old navigation code that did not use pending navigation item.

BUG=692331
TEST=Back forward navigation works correctly.
==========

[Eugene But (OOO till 7-30), 2017-03-14 20:50:55]

Description was changed from

==========
Cleaned up old navigation code that did not use pending navigation item.

BUG=692331
TEST=Back forward navigation works correctly.
==========

to

==========
Cleaned up old navigation code that did not use pending navigation item.

Also removed PendingIndexNavigationDisabled experimental setting.

BUG=692331
TEST=Back forward navigation works correctly.
==========

[Eugene But (OOO till 7-30), 2017-03-14 20:51:22]

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

[commit-bot: I haz the power, 2017-03-14 20:52:32]

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-03-14 21:09:56]

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

[commit-bot: I haz the power, 2017-03-14 21:09:57]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-03-15 18:31:54]

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

[Eugene But (OOO till 7-30), 2017-03-15 18:38:51]

Description was changed from

==========
Cleaned up old navigation code that did not use pending navigation item.

Also removed PendingIndexNavigationDisabled experimental setting.

BUG=692331
TEST=Back forward navigation works correctly.
==========

to

==========
Cleaned up old navigation code that did not use pending navigation item.

Also removed PendingIndexNavigationDisabled experimental setting.

BUG=692331,701717
TEST=Back forward navigation works correctly.
==========

[kkhorimoto, 2017-03-15 18:50:30]

https://codereview.chromium.org/2751793002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (left):

https://codereview.chromium.org/2751793002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1870: DCHECK(!params.post_data);
Removing this changes behavior, as in the new code, we're creating a pending
NavigationItem for history traversals and reloads.  Additionally, the more I
think about this code, the less sense it makes.  Previously, if there was a
foward/back navigation, the current URL would be loaded.  I don't think that
this codepath was ever executed since our implementation for back/forward
navigations goes through GoToItemAtIndex() and our reload implementation (before
Yuke's CL) called directly through to CRWWebController's |-reload|.  Should we
just add a DCHECK verifying that back/forward navigations aren't passed to this
method?

https://codereview.chromium.org/2751793002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:2265: [self
URLForHistoryNavigationToItem:currentItem previousURL:fromURL];
This is the only place where |-URLForHistoryNavigationToItem:previousURL:| was
called.  Do we want to use this for the pending item index implementation as
well?  If not, you can remove that also in this CL

[Eugene But (OOO till 7-30), 2017-03-15 20:06:57]

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

[commit-bot: I haz the power, 2017-03-15 20:07:50]

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-03-15 20:37:46]

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

[commit-bot: I haz the power, 2017-03-15 20:37:47]

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

[Eugene But (OOO till 7-30), 2017-03-16 00:50:42]

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

[Eugene But (OOO till 7-30), 2017-03-16 00:50:49]

The CQ bit was unchecked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-03-16 00:51:05]

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

[commit-bot: I haz the power, 2017-03-16 00:51:49]

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), 2017-03-16 00:52:23]

Thanks! PTAL

https://codereview.chromium.org/2751793002/diff/1/ios/web/web_state/ui/crw_we...
File ios/web/web_state/ui/crw_web_controller.mm (left):

https://codereview.chromium.org/2751793002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1870: DCHECK(!params.post_data);
On 2017/03/15 18:50:30, kkhorimoto_ wrote:
> Removing this changes behavior, as in the new code, we're creating a pending
> NavigationItem for history traversals and reloads.  Additionally, the more I
> think about this code, the less sense it makes.  Previously, if there was a
> foward/back navigation, the current URL would be loaded.  I don't think that
> this codepath was ever executed since our implementation for back/forward
> navigations goes through GoToItemAtIndex() and our reload implementation
(before
> Yuke's CL) called directly through to CRWWebController's |-reload|.  Should we
> just add a DCHECK verifying that back/forward navigations aren't passed to
this
> method?
I removed this code because I believe it's a dead code and throwing it away does
not change the behavior. Added DCHECK that transition is not back forward.

https://codereview.chromium.org/2751793002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:2265: [self
URLForHistoryNavigationToItem:currentItem previousURL:fromURL];
On 2017/03/15 18:50:30, kkhorimoto_ wrote:
> This is the only place where |-URLForHistoryNavigationToItem:previousURL:| was
> called.  Do we want to use this for the pending item index implementation as
> well?  If not, you can remove that also in this CL
Good catch. I don't know why we needed this code in the first place. Pending
navigation was implemented more than 2 months ago and we did not have any
problems w/o this method. So I'm just removing it.

[commit-bot: I haz the power, 2017-03-16 01:06:15]

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

[commit-bot: I haz the power, 2017-03-16 01:06:16]

Dry run: This issue passed the CQ dry run.

[kkhorimoto, 2017-03-16 01:08:09]

lgtm

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

https://codereview.chromium.org/2751793002/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:1804: DCHECK(!(transition &
ui::PAGE_TRANSITION_FORWARD_BACK));
Can we also include the RELOAD transition type?  If a reload went through this
method, it would incorrectly add a pending item.  Currently, Tab directly calls
CRWWebController's |-reload|, so this never happens in practice, but calling
NavigationManager::Reload() before Yuke's CL lands would cause broken behavior.

[Eugene But (OOO till 7-30), 2017-03-16 15:14:09]

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

[commit-bot: I haz the power, 2017-03-16 15:14:32]

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-03-16 15:28:32]

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

[commit-bot: I haz the power, 2017-03-16 15:28:33]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-03-16 16:46:42]

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

https://codereview.chromium.org/2751793002/diff/40001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:1804: DCHECK(!(transition &
ui::PAGE_TRANSITION_FORWARD_BACK));
On 2017/03/16 01:08:09, kkhorimoto_ wrote:
> Can we also include the RELOAD transition type?  If a reload went through this
> method, it would incorrectly add a pending item.  Currently, Tab directly
calls
> CRWWebController's |-reload|, so this never happens in practice, but calling
> NavigationManager::Reload() before Yuke's CL lands would cause broken
behavior.
Good point. Done.

[Eugene But (OOO till 7-30), 2017-03-16 16:46:46]

The CQ bit was checked by eugenebut@chromium.org

[Eugene But (OOO till 7-30), 2017-03-16 16:46:46]

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/2751793002/#ps60001
(title: "Moar DCHECKs")

[commit-bot: I haz the power, 2017-03-16 16:47: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-03-16 16:55:22]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489682806238320,
"parent_rev": "829403a1084830dd160dedf24c50735e293c0bc1", "commit_rev":
"2feaa058530953a2232e6128e16deb31192ad9d2"}

[commit-bot: I haz the power, 2017-03-16 16:56:08]

Description was changed from

==========
Cleaned up old navigation code that did not use pending navigation item.

Also removed PendingIndexNavigationDisabled experimental setting.

BUG=692331,701717
TEST=Back forward navigation works correctly.
==========

to

==========
Cleaned up old navigation code that did not use pending navigation item.

Also removed PendingIndexNavigationDisabled experimental setting.

BUG=692331,701717
TEST=Back forward navigation works correctly.

Review-Url: https://codereview.chromium.org/2751793002
Cr-Commit-Position: refs/heads/master@{#457459}
Committed:
https://chromium.googlesource.com/chromium/src/+/2feaa058530953a2232e6128e16d...
==========

[commit-bot: I haz the power, 2017-03-16 16:56:09]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2feaa058530953a2232e6128e16d...

[rohitrao (ping after 24h), 2017-03-16 19:36:54]

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

[rohitrao (ping after 24h), 2017-03-16 19:36:54]

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

https://codereview.chromium.org/2751793002/diff/60001/ios/web/web_state/ui/cr...
ios/web/web_state/ui/crw_web_controller.mm:1805: DCHECK(!(transition &
ui::PAGE_TRANSITION_RELOAD));
Looks like this DCHECK is firing during an internal test:

[0316/104559.588473:FATAL:crw_web_controller.mm(1805)] Check failed:
!(transition & ui::PAGE_TRANSITION_RELOAD).
0   ios_internal_chrome_external_url_egtests 0x0157cec6
base::debug::StackTrace::StackTrace(unsigned long) + 134
1   ios_internal_chrome_external_url_egtests 0x0157cf44
base::debug::StackTrace::StackTrace(unsigned long) + 36
2   ios_internal_chrome_external_url_egtests 0x0157bbb7
base::debug::StackTrace::StackTrace() + 39
3   ios_internal_chrome_external_url_egtests 0x015e299c
logging::LogMessage::~LogMessage() + 92
4   ios_internal_chrome_external_url_egtests 0x015e06d7
logging::LogMessage::~LogMessage() + 23
5   ios_internal_chrome_external_url_egtests 0x003758d4 -[CRWWebController
loadWithParams:] + 660
6   ios_internal_chrome_external_url_egtests 0x0037728d -[CRWWebController
reloadInternal] + 493
7   ios_internal_chrome_external_url_egtests 0x003773bb -[CRWWebController
reload] + 91
8   ios_internal_chrome_external_url_egtests 0x00e3ebb5 -[Tab reload] + 69
9   ios_internal_chrome_external_url_egtests 0x00cb61f3 -[BrowserViewController
chromeExecuteCommand:] + 3011
10  ios_internal_chrome_external_url_egtests 0x00629eb5
-[UIResponder(ChromeExecuteCommand) chromeExecuteCommand:] + 133
11  ios_internal_chrome_external_url_egtests 0x00629eb5
-[UIResponder(ChromeExecuteCommand) chromeExecuteCommand:] + 133
12  ios_internal_chrome_external_url_egtests 0x00629eb5
-[UIResponder(ChromeExecuteCommand) chromeExecuteCommand:] + 133
13  ios_internal_chrome_external_url_egtests 0x00629eb5
-[UIResponder(ChromeExecuteCommand) chromeExecuteCommand:] + 133
14  ios_internal_chrome_external_url_egtests 0x00c26216
-[ToolsMenuViewController buttonPressed:] + 758

[rohitrao (ping after 24h), 2017-03-16 19:37:41]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/2759483002/ by rohitrao@chromium.org.

The reason for reverting is: testReloadInterstitial is failing on an internal
bot.  For an example, see build 4404 on the internal iphone10-simulator bot..