Correctly reload URLs in web views if a native controller is displayed.

Correctly reload URLs in web views if a native controller is displayed.

If a page has failed to load, a native controller is used to display
the error page.  However, if NavigationManager::Reload() is called while
an error page is displayed, we want to attempt to reload the current
NavigationItem's URL, not the native error page.

BUG=703707
Review-Url: https://codereview.chromium.org/2765903003
Cr-Commit-Position: refs/heads/master@{#458499}
Committed: https://chromium.googlesource.com/chromium/src/+/2d163839906a68671aea3291a7606babbfc72999

[kkhorimoto, 2017-03-21 17:45:55]

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

[Eugene But (OOO till 7-30), 2017-03-21 17:48:43]

Description was changed from

==========
Correctly reload URLs in web views if a native controller.

If a page has failed to load, a native controller is used to display
the error page.  However, if NavigationManager::Reload() is called while
an error page is displayed, we want to attempt to reload the current
NavigationItem's URL, not the native error page.

BUG=703707
==========

to

==========
Correctly reload URLs in web views if a native controller is displayed.

If a page has failed to load, a native controller is used to display
the error page.  However, if NavigationManager::Reload() is called while
an error page is displayed, we want to attempt to reload the current
NavigationItem's URL, not the native error page.

BUG=703707
==========

[Eugene But (OOO till 7-30), 2017-03-21 17:50:10]

Thanks for fixing this! lgtm. I kicked of eg-simulator build to run tests.

[Olivier, 2017-03-21 17:50:26]

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

[Olivier, 2017-03-21 17:50:26]

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

https://codereview.chromium.org/2765903003/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1983: if ([self
shouldLoadURLInNativeView:self.currentNavItem->GetURL()]) {
I think this won't handle the Reading List loading correctly as  self
shouldLoadURLInNativeView:self.currentNavItem->GetURL()] will be true but not
yet presented in a native item.

Is it possible to have both test ?
like 
if ([self shouldLoadURLInNativeView:self.currentNavItem->GetURL()] &&
self.nativeController) {
 ?

[kkhorimoto, 2017-03-21 17:59:46]

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

https://codereview.chromium.org/2765903003/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:1983: if ([self
shouldLoadURLInNativeView:self.currentNavItem->GetURL()]) {
On 2017/03/21 17:50:26, Olivier Robin wrote:
> I think this won't handle the Reading List loading correctly as  self
> shouldLoadURLInNativeView:self.currentNavItem->GetURL()] will be true but not
> yet presented in a native item.
> 
> Is it possible to have both test ?
> like 
> if ([self shouldLoadURLInNativeView:self.currentNavItem->GetURL()] &&
> self.nativeController) {
>  ?

Ahh you're right, I didn't think that through; the reading lists will still go
through the CRWNativeContent |-reload| path.  Your suggestion would still have
the same effect because the ErrorPageContent will be non-nil, so it'd still
reload the error page.  My CL is an improvement in its current form, but it
won't actually help your bug.  I think it'd probably be best to land your chrome
layer workaround in addition to this CL.  If possible, we should intercept the
error message and display any available offline content immediately instead of
the ErrorPageContents; this will eliminate the need for calling |-reload| here
to begin with.

[kkhorimoto, 2017-03-21 17:59:56]

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

[commit-bot: I haz the power, 2017-03-21 18:00:42]

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-21 18:15:14]

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

[commit-bot: I haz the power, 2017-03-21 18:15:15]

Dry run: This issue passed the CQ dry run.

[kkhorimoto, 2017-03-21 18:37:54]

The CQ bit was checked by kkhorimoto@chromium.org

[commit-bot: I haz the power, 2017-03-21 18:38: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-03-21 19:09:26]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1490121474982050, "parent_rev":
"ad84c7704a0def426d12b8501371ea0dac37134a", "commit_rev":
"2d163839906a68671aea3291a7606babbfc72999"}

[commit-bot: I haz the power, 2017-03-21 19:10:09]

Description was changed from

==========
Correctly reload URLs in web views if a native controller is displayed.

If a page has failed to load, a native controller is used to display
the error page.  However, if NavigationManager::Reload() is called while
an error page is displayed, we want to attempt to reload the current
NavigationItem's URL, not the native error page.

BUG=703707
==========

to

==========
Correctly reload URLs in web views if a native controller is displayed.

If a page has failed to load, a native controller is used to display
the error page.  However, if NavigationManager::Reload() is called while
an error page is displayed, we want to attempt to reload the current
NavigationItem's URL, not the native error page.

BUG=703707

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

[commit-bot: I haz the power, 2017-03-21 19:10:10]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/2d163839906a68671aea3291a760...