Gracefully handle didCommitNavigation: call after didFinishNavigation:

Gracefully handle didCommitNavigation: call after didFinishNavigation:

It is possible to get WKNavigationDelegate callbacks in the following
order: didFinishNavigation, didCommitNavigation:.

In this case didFinishNavigation should not remove navigation from
CRWWKNavigationStates storage, because that navigation will be used in
didCommitNavigation.

Also didCommitNavigation should remove navigation and signal
didFinishNavigation: which is expected to be called only after
navigation is committed.

BUG=727289
Review-Url: https://codereview.chromium.org/2950853002
Cr-Commit-Position: refs/heads/master@{#481584}
Committed: https://chromium.googlesource.com/chromium/src/+/5c9be243c093b5410523730bdd377dd4cd68578d

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

Description was changed from

==========
Gracefully handle didCommitNavigation: call after didFinishNavigation:

It is possible to get WKNavigationDelegate callbacks in the following
order: didFinishNavigation, didCommitNavigation:.

In this case didFinishNavigation should not remove navigation from
CRWWKNavigationStates storage, because that navogation will be used in
didCommitNavigation.

Also didCommitNavigation should remove navigation and signal
didFinishNavigation: which is expected to be called only after
navigation is committed.

BUG=727289
==========

to

==========
Gracefully handle didCommitNavigation: call after didFinishNavigation:

It is possible to get WKNavigationDelegate callbacks in the following
order: didFinishNavigation, didCommitNavigation:.

In this case didFinishNavigation should not remove navigation from
CRWWKNavigationStates storage, because that navigation will be used in
didCommitNavigation.

Also didCommitNavigation should remove navigation and signal
didFinishNavigation: which is expected to be called only after
navigation is committed.

BUG=727289
==========

[Eugene But (OOO till 7-30), 2017-06-20 16:16:43]

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

[commit-bot: I haz the power, 2017-06-20 16:17:02]

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-06-20 16:30:05]

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

[commit-bot: I haz the power, 2017-06-20 16:30:05]

Dry run: This issue passed the CQ dry run.

[Eugene But (OOO till 7-30), 2017-06-20 17:48:56]

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

[Eugene But (OOO till 7-30), 2017-06-21 21:24:44]

Kurt?

[kkhorimoto, 2017-06-22 00:49:02]

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

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4647: bool navigationCommitted =
Should we just early return if the navigation isn't committed yet?  Are we sure
that if we receive this, we'll always get a subsequent call to
didCommitNavigation where we can call into this method again?  This solution
changes the order in which the DidCommitNavigation() and DidFinishNavigation()
WSO callbacks are received, which may break chrome or components code.

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4658: web::ExecuteJavaScript(webView,
@"__gCrWeb.didFinishNavigation()", nil);
What does it mean to execute JS on a page when the navigation is finished, but
not committed?  Previously we assumed that if the page is committed, then JS
will be executed on that page's DOM, but now I'm not so sure.

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_wk...
File ios/web/web_state/ui/crw_wk_navigation_states.mm (right):

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_wk...
ios/web/web_state/ui/crw_wk_navigation_states.mm:129: state ==
web::WKNavigationState::COMMITTED));
Can you add some comments for this DCHECK explaining why the latter two
exceptions are there?

[Eugene But (OOO till 7-30), 2017-06-22 01:58:07]

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

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4647: bool navigationCommitted =
On 2017/06/22 00:49:01, kkhorimoto_ wrote:
> Should we just early return if the navigation isn't committed yet?  Are we
sure
> that if we receive this, we'll always get a subsequent call to
> didCommitNavigation where we can call into this method again?  This solution
> changes the order in which the DidCommitNavigation() and DidFinishNavigation()
> WSO callbacks are received, which may break chrome or components code.
There is no change in NavigationItemCommitted/DidFinishNavigation order.
DidFinishNavigation is called when navigation is committed (it is Chromium
concept which is different from WKWebView).

I think we still should attempt to fetch favicons by calling 
__gCrWeb.didFinishNavigation.


This CL does 2 things:
  1.) Removes navigation on webView:didCommitNavigation: if
webView:didFinishNavigaiton: was called
  2.) Signals that page finished loading in webView:didCommitNavigation: if
webView:didFinishNavigaiton: was called

I don't know if it is possible to have webView:didFinishNavigaiton: call w/o
webView:didCommitNavigation:, but if it happens, then progress indicator and
network spinner will not be removed. That's something that we will likely will
notice.

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4658: web::ExecuteJavaScript(webView,
@"__gCrWeb.didFinishNavigation()", nil);
On 2017/06/22 00:49:02, kkhorimoto_ wrote:
> What does it mean to execute JS on a page when the navigation is finished, but
> not committed?  Previously we assumed that if the page is committed, then JS
> will be executed on that page's DOM, but now I'm not so sure.
This fetches favicons, which still should work and should still be useful.

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_wk...
File ios/web/web_state/ui/crw_wk_navigation_states.mm (right):

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_wk...
ios/web/web_state/ui/crw_wk_navigation_states.mm:129: state ==
web::WKNavigationState::COMMITTED));
On 2017/06/22 00:49:02, kkhorimoto_ wrote:
> Can you add some comments for this DCHECK explaining why the latter two
> exceptions are there?

Done.

[kkhorimoto, 2017-06-22 02:09:48]

lgtm

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

https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_we...
ios/web/web_state/ui/crw_web_controller.mm:4647: bool navigationCommitted =
On 2017/06/22 01:58:06, Eugene But wrote:
> On 2017/06/22 00:49:01, kkhorimoto_ wrote:
> > Should we just early return if the navigation isn't committed yet?  Are we
> sure
> > that if we receive this, we'll always get a subsequent call to
> > didCommitNavigation where we can call into this method again?  This solution
> > changes the order in which the DidCommitNavigation() and
DidFinishNavigation()
> > WSO callbacks are received, which may break chrome or components code.
> There is no change in NavigationItemCommitted/DidFinishNavigation order.
> DidFinishNavigation is called when navigation is committed (it is Chromium
> concept which is different from WKWebView).
> 
> I think we still should attempt to fetch favicons by calling 
> __gCrWeb.didFinishNavigation.
> 
> 
> This CL does 2 things:
>   1.) Removes navigation on webView:didCommitNavigation: if
> webView:didFinishNavigaiton: was called
>   2.) Signals that page finished loading in webView:didCommitNavigation: if
> webView:didFinishNavigaiton: was called
> 
> I don't know if it is possible to have webView:didFinishNavigaiton: call w/o
> webView:didCommitNavigation:, but if it happens, then progress indicator and
> network spinner will not be removed. That's something that we will likely will
> notice.

whoops, I thought that the WSO didFinishNavigation was sent from
CRWWebController's |-didfinishNavigtion|.

[Eugene But (OOO till 7-30), 2017-06-22 17:25:00]

On 2017/06/22 02:09:48, kkhorimoto_ wrote:
> lgtm
> 
>
https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_we...
> File ios/web/web_state/ui/crw_web_controller.mm (right):
> 
>
https://codereview.chromium.org/2950853002/diff/1/ios/web/web_state/ui/crw_we...
> ios/web/web_state/ui/crw_web_controller.mm:4647: bool navigationCommitted =
> On 2017/06/22 01:58:06, Eugene But wrote:
> > On 2017/06/22 00:49:01, kkhorimoto_ wrote:
> > > Should we just early return if the navigation isn't committed yet?  Are we
> > sure
> > > that if we receive this, we'll always get a subsequent call to
> > > didCommitNavigation where we can call into this method again?  This
solution
> > > changes the order in which the DidCommitNavigation() and
> DidFinishNavigation()
> > > WSO callbacks are received, which may break chrome or components code.
> > There is no change in NavigationItemCommitted/DidFinishNavigation order.
> > DidFinishNavigation is called when navigation is committed (it is Chromium
> > concept which is different from WKWebView).
> > 
> > I think we still should attempt to fetch favicons by calling 
> > __gCrWeb.didFinishNavigation.
> > 
> > 
> > This CL does 2 things:
> >   1.) Removes navigation on webView:didCommitNavigation: if
> > webView:didFinishNavigaiton: was called
> >   2.) Signals that page finished loading in webView:didCommitNavigation: if
> > webView:didFinishNavigaiton: was called
> > 
> > I don't know if it is possible to have webView:didFinishNavigaiton: call w/o
> > webView:didCommitNavigation:, but if it happens, then progress indicator and
> > network spinner will not be removed. That's something that we will likely
will
> > notice.
> 
> whoops, I thought that the WSO didFinishNavigation was sent from
> CRWWebController's |-didfinishNavigtion|.
Well. WKWebView and Chromium API incompatibility is confusing :(

[Eugene But (OOO till 7-30), 2017-06-22 17:25:14]

Thanks for review!

[Eugene But (OOO till 7-30), 2017-06-22 17:25:28]

The CQ bit was checked by eugenebut@chromium.org

[commit-bot: I haz the power, 2017-06-22 17:25:52]

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-06-22 17:39:15]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1498152328959350,
"parent_rev": "62018a1d715eade310d3b07770dfb72f8ca0336c", "commit_rev":
"5c9be243c093b5410523730bdd377dd4cd68578d"}

[commit-bot: I haz the power, 2017-06-22 17:39:31]

Description was changed from

==========
Gracefully handle didCommitNavigation: call after didFinishNavigation:

It is possible to get WKNavigationDelegate callbacks in the following
order: didFinishNavigation, didCommitNavigation:.

In this case didFinishNavigation should not remove navigation from
CRWWKNavigationStates storage, because that navigation will be used in
didCommitNavigation.

Also didCommitNavigation should remove navigation and signal
didFinishNavigation: which is expected to be called only after
navigation is committed.

BUG=727289
==========

to

==========
Gracefully handle didCommitNavigation: call after didFinishNavigation:

It is possible to get WKNavigationDelegate callbacks in the following
order: didFinishNavigation, didCommitNavigation:.

In this case didFinishNavigation should not remove navigation from
CRWWKNavigationStates storage, because that navigation will be used in
didCommitNavigation.

Also didCommitNavigation should remove navigation and signal
didFinishNavigation: which is expected to be called only after
navigation is committed.

BUG=727289

Review-Url: https://codereview.chromium.org/2950853002
Cr-Commit-Position: refs/heads/master@{#481584}
Committed:
https://chromium.googlesource.com/chromium/src/+/5c9be243c093b5410523730bdd37...
==========

[commit-bot: I haz the power, 2017-06-22 17:39:33]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/5c9be243c093b5410523730bdd37...