Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1546603005:
[ios] Make sure that |didFinishNavigation| is signalled for fast goBack. (Closed)
|
Created:
5 years ago by Eugene But (OOO till 7-30) Modified:
4 years, 11 months ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Make sure that |didFinishNavigation| is signalled for fast goBack.
A fast back/forward may not call |webView:didFinishNavigation:|, so
finishing the navigation should be signalled explicitely when WKWebView
isLoading is changed to NO.
BUG=571584
Committed: https://crrev.com/7ce600bd5b62b2c79b288a5bf493251b8e8aa59a
Cr-Commit-Position: refs/heads/master@{#367390}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixed spelling #Messages
Total messages: 12 (3 generated)
eugenebut@chromium.org changed reviewers: + jyquinn@chromium.org, stuartmorgan@chromium.org
This CL can wait for Stuart's review.
https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1621: // A fast back/forward may not call |webView:didFinishNavigation:|, so Is is "may not" or "will not"? Any chance we'd run into timing issues here if didFinishNavigation is called too early? I know calling didFinishNavigation multiple times is fine, just wondering if calling it early may mess something up. https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1622: // finishing the navigation should be signalled explicitely. s/explicitely/explicitly
https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1621: // A fast back/forward may not call |webView:didFinishNavigation:|, so On 2015/12/22 23:41:35, Jackie Quinn wrote: > Is is "may not" or "will not"? Any chance we'd run into timing issues here if > didFinishNavigation is called too early? I know calling didFinishNavigation > multiple times is fine, just wondering if calling it early may mess something > up. It is actually "may not" (I think depending on caching). I don't see how calling |didFinishNavigation| can mess up things, but I can't prove that :( This place is the last chance to call |didFinishNavigation| for BF navigation and should probably be done here anyways. https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1622: // finishing the navigation should be signalled explicitely. On 2015/12/22 23:41:35, Jackie Quinn wrote: > s/explicitely/explicitly Done.
lgtm https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1621: // A fast back/forward may not call |webView:didFinishNavigation:|, so On 2015/12/22 23:56:29, eugenebut wrote: > On 2015/12/22 23:41:35, Jackie Quinn wrote: > > Is is "may not" or "will not"? Any chance we'd run into timing issues here if > > didFinishNavigation is called too early? I know calling didFinishNavigation > > multiple times is fine, just wondering if calling it early may mess something > > up. > It is actually "may not" (I think depending on caching). I don't see how calling > |didFinishNavigation| can mess up things, but I can't prove that :( > > This place is the last chance to call |didFinishNavigation| for BF navigation > and should probably be done here anyways. That was my suspicion... So long as it's not called before didCommit, I think it's fine, and I don't think there's any way for it to be called before didCommit.
Thanks for quick review! Will wait for Stuart. https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1546603005/diff/1/ios/web/web_state/ui/crw_wk... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1621: // A fast back/forward may not call |webView:didFinishNavigation:|, so On 2015/12/23 00:13:03, Jackie Quinn wrote: > On 2015/12/22 23:56:29, eugenebut wrote: > > On 2015/12/22 23:41:35, Jackie Quinn wrote: > > > Is is "may not" or "will not"? Any chance we'd run into timing issues here > if > > > didFinishNavigation is called too early? I know calling didFinishNavigation > > > multiple times is fine, just wondering if calling it early may mess > something > > > up. > > It is actually "may not" (I think depending on caching). I don't see how > calling > > |didFinishNavigation| can mess up things, but I can't prove that :( > > > > This place is the last chance to call |didFinishNavigation| for BF navigation > > and should probably be done here anyways. > > That was my suspicion... So long as it's not called before didCommit, I think > it's fine, and I don't think there's any way for it to be called before > didCommit. You are right it is not called before |didCommit|.
LGTM, but I wish we weren't accumulating special cases so quickly :P I'd really like us to prioritize making The Big Matrix of Navigation Types/States/Callbacks once M48 goes out, so we aren't doing so much of this by trial and error.
The CQ bit was checked by eugenebut@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1546603005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1546603005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== [ios] Make sure that |didFinishNavigation| is signalled for fast goBack. A fast back/forward may not call |webView:didFinishNavigation:|, so finishing the navigation should be signalled explicitely when WKWebView isLoading is changed to NO. BUG=571584 ========== to ========== [ios] Make sure that |didFinishNavigation| is signalled for fast goBack. A fast back/forward may not call |webView:didFinishNavigation:|, so finishing the navigation should be signalled explicitely when WKWebView isLoading is changed to NO. BUG=571584 Committed: https://crrev.com/7ce600bd5b62b2c79b288a5bf493251b8e8aa59a Cr-Commit-Position: refs/heads/master@{#367390} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/7ce600bd5b62b2c79b288a5bf493251b8e8aa59a Cr-Commit-Position: refs/heads/master@{#367390} |
