Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931833004:
Always set the navigation pending item for page reload. (Closed)
DescriptionAlways set the navigation pending item for page reload.
Set the pendingItemIndex to the last committed item in case of page reload
and on the case of application open/tab out of memory reopened.
This doesn't solve the PendingItem being null problem entirely there are still
couple cases when that can happen and they are not page reloads.
CL 1/2
BUG=676129
Review-Url: https://codereview.chromium.org/2931833004
Cr-Commit-Position: refs/heads/master@{#486777}
Committed: https://chromium.googlesource.com/chromium/src/+/bc2e0a45e23c739bb3c04d119d43abbb01398289
Patch Set 1 : Initial #Patch Set 2 : Merge branch 'master' of https://chromium.googlesource.com/chromium/src into pendingItem2 #Patch Set 3 : only reload cases #
Total comments: 16
Patch Set 4 : comments #Patch Set 5 : only ios web #Patch Set 6 : move transietItem to different CL #
Messages
Total messages: 42 (26 generated)
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. BUG=676129 ========== to ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. This doesn't solve the PendingItem being null problem entirely there are still couple cases when that can happen and they are not page reloads. BUG=676129 ==========
mrefaat@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Description was changed from ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. This doesn't solve the PendingItem being null problem entirely there are still couple cases when that can happen and they are not page reloads. BUG=676129 ========== to ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. This doesn't solve the PendingItem being null problem entirely there are still couple cases when that can happen and they are not page reloads. BUG=676129 ==========
https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (left): https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:253: } You're removing this fallback here, but not above. Why is that? https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:296: item = navigationManager->GetLastCommittedItem(); Same question here. https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:165: item = manager->GetLastCommittedItem(); By removing the |is_reload| calculation here, we're changing the behavior for other scenarios in which there might not be a pending item. What scenarios remain where this can happen? Do they affect this flow?
Could you please move changes in chrome layer to separate CLs. If they introduce regressions, then reverts will be simpler. https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:243: DCHECK(item); Per Chromium Style Guide we should either have DCHECK or early return, but not both: https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2931833004/diff/100001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.mm:152: DCHECK(item); nit: No need for DCHECK before dereferencing the pointer. https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2046: self.sessionController.pendingItemIndex = Wouldn't |reload| eventually call |registerLoadRequestForURL:| which also sets pendingItemIndex? Or this code is really necessary?
I'm not sure i understand the reason for moving the chrome changes to different CL, however lets get everything reviewed in this CL then i can move https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (left): https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:253: } On 2017/06/14 22:48:26, kkhorimoto_ wrote: > You're removing this fallback here, but not above. Why is that? I thought this is not going to be reachable in case of the reload but this is actually not correct. i'll change it back https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:296: item = navigationManager->GetLastCommittedItem(); On 2017/06/14 22:48:26, kkhorimoto_ wrote: > Same question here. ditto https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... File ios/chrome/browser/reading_list/reading_list_web_state_observer.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:165: item = manager->GetLastCommittedItem(); On 2017/06/14 22:48:26, kkhorimoto_ wrote: > By removing the |is_reload| calculation here, we're changing the behavior for > other scenarios in which there might not be a pending item. What scenarios > remain where this can happen? Do they affect this flow? from my testing & understanding nothing should change. The reload calculation here was wrong, as sometimes the pending item is null and its not a reload. https://codereview.chromium.org/2931833004/diff/100001/ios/chrome/browser/rea... ios/chrome/browser/reading_list/reading_list_web_state_observer.mm:243: DCHECK(item); On 2017/06/15 00:08:11, Eugene But wrote: > Per Chromium Style Guide we should either have DCHECK or early return, but not > both: > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Acknowledged. https://codereview.chromium.org/2931833004/diff/100001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.mm:152: DCHECK(item); On 2017/06/15 00:08:11, Eugene But wrote: > nit: No need for DCHECK before dereferencing the pointer. Done. https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2046: self.sessionController.pendingItemIndex = On 2017/06/15 00:08:11, Eugene But wrote: > Wouldn't |reload| eventually call |registerLoadRequestForURL:| which also sets > pendingItemIndex? Or this code is really necessary? This will not happen in every case, only when the the url is going to be open natively not on web view. I can move setting the pending item open in webview condition but i think this is much simpler and easier to understand.
Patchset #4 (id:120001) has been deleted
lgtm! Thanks for fixing this. The reason for split is to make small independent CLs, rather than large one which touch different features. This CL can potentially cause regressions and bisecting and reverting small CLs is easier than large CLs. https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2046: self.sessionController.pendingItemIndex = On 2017/06/15 18:35:35, mrefaat wrote: > On 2017/06/15 00:08:11, Eugene But wrote: > > Wouldn't |reload| eventually call |registerLoadRequestForURL:| which also sets > > pendingItemIndex? Or this code is really necessary? > > This will not happen in every case, only when the the url is going to be open > natively not on web view. > I can move setting the pending item open in webview condition but i think this > is much simpler and easier to understand. Thanks. So this code is needed to cover NativeContent reload. But then how could it happen that Line 1527 can execute (Reload Transition and no pending item)?
lgtm. +1 to splitting up the CL
On 2017/06/16 03:25:46, Eugene But wrote: > lgtm! Thanks for fixing this. > > The reason for split is to make small independent CLs, rather than large one > which touch different features. This CL can potentially cause regressions and > bisecting and reverting small CLs is easier than large CLs. > > https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller.mm:2046: > self.sessionController.pendingItemIndex = > On 2017/06/15 18:35:35, mrefaat wrote: > > On 2017/06/15 00:08:11, Eugene But wrote: > > > Wouldn't |reload| eventually call |registerLoadRequestForURL:| which also > sets > > > pendingItemIndex? Or this code is really necessary? > > > > This will not happen in every case, only when the the url is going to be open > > natively not on web view. > > I can move setting the pending item open in webview condition but i think this > > is much simpler and easier to understand. > Thanks. So this code is needed to cover NativeContent reload. > > But then how could it happen that Line 1527 can execute (Reload Transition and > no pending item)? I understand why usually we should split CLs if they can cause regressions, however i don't understand why this one can cause that :) ?
https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2046: self.sessionController.pendingItemIndex = On 2017/06/16 03:25:46, Eugene But wrote: > On 2017/06/15 18:35:35, mrefaat wrote: > > On 2017/06/15 00:08:11, Eugene But wrote: > > > Wouldn't |reload| eventually call |registerLoadRequestForURL:| which also > sets > > > pendingItemIndex? Or this code is really necessary? > > > > This will not happen in every case, only when the the url is going to be open > > natively not on web view. > > I can move setting the pending item open in webview condition but i think this > > is much simpler and easier to understand. > Thanks. So this code is needed to cover NativeContent reload. > > But then how could it happen that Line 1527 can execute (Reload Transition and > no pending item)? This will happen in case the reload is triggered by the content itself not by the browser.
This CL removes fallback to last committed item in a few places. Every place can potentially be a crash (a separate crash). So that's why it is better to split the CL. In ideal world we would rely on tests for that, but unfortunately web layer does not have the best test coverage. https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2046: self.sessionController.pendingItemIndex = On 2017/06/16 19:28:04, mrefaat wrote: > On 2017/06/16 03:25:46, Eugene But wrote: > > On 2017/06/15 18:35:35, mrefaat wrote: > > > On 2017/06/15 00:08:11, Eugene But wrote: > > > > Wouldn't |reload| eventually call |registerLoadRequestForURL:| which also > > sets > > > > pendingItemIndex? Or this code is really necessary? > > > > > > This will not happen in every case, only when the the url is going to be > open > > > natively not on web view. > > > I can move setting the pending item open in webview condition but i think > this > > > is much simpler and easier to understand. > > Thanks. So this code is needed to cover NativeContent reload. > > > > But then how could it happen that Line 1527 can execute (Reload Transition and > > no pending item)? > > This will happen in case the reload is triggered by the content itself not by > the browser. But reload triggered by renderer calls |reload| as well: https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.... Did I miss something?
https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:2046: self.sessionController.pendingItemIndex = On 2017/06/17 02:57:14, Eugene But wrote: > On 2017/06/16 19:28:04, mrefaat wrote: > > On 2017/06/16 03:25:46, Eugene But wrote: > > > On 2017/06/15 18:35:35, mrefaat wrote: > > > > On 2017/06/15 00:08:11, Eugene But wrote: > > > > > Wouldn't |reload| eventually call |registerLoadRequestForURL:| which > also > > > sets > > > > > pendingItemIndex? Or this code is really necessary? > > > > > > > > This will not happen in every case, only when the the url is going to be > > open > > > > natively not on web view. > > > > I can move setting the pending item open in webview condition but i think > > this > > > > is much simpler and easier to understand. > > > Thanks. So this code is needed to cover NativeContent reload. > > > > > > But then how could it happen that Line 1527 can execute (Reload Transition > and > > > no pending item)? > > > > This will happen in case the reload is triggered by the content itself not by > > the browser. > But reload triggered by renderer calls |reload| as well: > https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.... > > Did I miss something? I'm not sure, from what i see it seems that goDelta is only called for history forward/backward. and during my tests i have not passed through this function once.
On 2017/06/17 11:33:08, mrefaat wrote: > https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... > File ios/web/web_state/ui/crw_web_controller.mm (right): > > https://codereview.chromium.org/2931833004/diff/100001/ios/web/web_state/ui/c... > ios/web/web_state/ui/crw_web_controller.mm:2046: > self.sessionController.pendingItemIndex = > On 2017/06/17 02:57:14, Eugene But wrote: > > On 2017/06/16 19:28:04, mrefaat wrote: > > > On 2017/06/16 03:25:46, Eugene But wrote: > > > > On 2017/06/15 18:35:35, mrefaat wrote: > > > > > On 2017/06/15 00:08:11, Eugene But wrote: > > > > > > Wouldn't |reload| eventually call |registerLoadRequestForURL:| which > > also > > > > sets > > > > > > pendingItemIndex? Or this code is really necessary? > > > > > > > > > > This will not happen in every case, only when the the url is going to be > > > open > > > > > natively not on web view. > > > > > I can move setting the pending item open in webview condition but i > think > > > this > > > > > is much simpler and easier to understand. > > > > Thanks. So this code is needed to cover NativeContent reload. > > > > > > > > But then how could it happen that Line 1527 can execute (Reload Transition > > and > > > > no pending item)? > > > > > > This will happen in case the reload is triggered by the content itself not > by > > > the browser. > > But reload triggered by renderer calls |reload| as well: > > > https://cs.chromium.org/chromium/src/ios/web/web_state/ui/crw_web_controller.... > > > > Did I miss something? > > I'm not sure, from what i see it seems that goDelta is only called for history > forward/backward. and during my tests i have not passed through this function > once. Oh, that's because Chrome for iOS does not override window.reload. Only window.history.go(0), which creates the discrepancy. Thanks for explanation.
updated with changes for ios web only
Description was changed from ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. This doesn't solve the PendingItem being null problem entirely there are still couple cases when that can happen and they are not page reloads. BUG=676129 ========== to ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. This doesn't solve the PendingItem being null problem entirely there are still couple cases when that can happen and they are not page reloads. CL 1/2 BUG=676129 ==========
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Thanks!
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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...)
Patchset #6 (id:180001) has been deleted
Patchset #6 (id:200001) has been deleted
The CQ bit was checked by mrefaat@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by mrefaat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2931833004/#ps220001 (title: "move transietItem to different CL")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1500050058558730,
"parent_rev": "4c88e567ae4f72adba1f6e818eee9141ecfdbb35", "commit_rev":
"bc2e0a45e23c739bb3c04d119d43abbb01398289"}
Message was sent while issue was closed.
Description was changed from ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. This doesn't solve the PendingItem being null problem entirely there are still couple cases when that can happen and they are not page reloads. CL 1/2 BUG=676129 ========== to ========== Always set the navigation pending item for page reload. Set the pendingItemIndex to the last committed item in case of page reload and on the case of application open/tab out of memory reopened. This doesn't solve the PendingItem being null problem entirely there are still couple cases when that can happen and they are not page reloads. CL 1/2 BUG=676129 Review-Url: https://codereview.chromium.org/2931833004 Cr-Commit-Position: refs/heads/master@{#486777} Committed: https://chromium.googlesource.com/chromium/src/+/bc2e0a45e23c739bb3c04d119d43... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://chromium.googlesource.com/chromium/src/+/bc2e0a45e23c739bb3c04d119d43...
Message was sent while issue was closed.
A revert of this CL (patchset #6 id:220001) has been created in https://codereview.chromium.org/2998463002/ by mrefaat@chromium.org. The reason for reverting is: introduced crbug/750775. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
