Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2834413002:
Return NavigationContext from registerLoadRequest. (Closed)
|
Created:
3 years, 8 months ago by Eugene But (OOO till 7-30) Modified:
3 years, 7 months ago Reviewers:
kkhorimoto CC:
chromium-reviews, Eugene But (OOO till 7-30), ios-reviews+web_chromium.org, ios-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionReturn NavigationContext from registerLoadRequest.
This CL (3 of 7) is a part of large refactoring to create
NavigationContext when navigation was started, not when it is finished.
This refactoring will allow to pass the same instance of
NavigationContext to DidStartNavigation and DidFinishNavigation
(crbug.com/713836). Also this will allow to associate NavigationItem
with NavigationContext object to commit non-latest navigation
(crbug.com/712269).
This CL makes registerLoadRequest API return NavigationContext object, which
can be used to store NavigationContext in CRWWKNavigationStates.
BUG=713836, 712269
Review-Url: https://codereview.chromium.org/2834413002
Cr-Commit-Position: refs/heads/master@{#470223}
Committed: https://chromium.googlesource.com/chromium/src/+/f97229b40443d92c57c3a08ebef8aff466932da0
Patch Set 1 #Patch Set 2 : - #
Total comments: 7
Patch Set 3 : Updated comments #Patch Set 4 : Rebased #Messages
Total messages: 14 (8 generated)
Description was changed from ========== Return NavigationContext from registerLoadRequest. This CL (3 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). BUG=713836, 712269 ========== to ========== Return NavigationContext from registerLoadRequest. This CL (3 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). This CL makes registerLoadRequest API return NavigationContext object, which can be used to store NavigationContext in CRWWKNavigationStates. BUG=713836, 712269 ==========
Description was changed from ========== Return NavigationContext from registerLoadRequest. This CL (3 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). This CL makes registerLoadRequest API return NavigationContext object, which can be used to store NavigationContext in CRWWKNavigationStates. BUG=713836, 712269 ========== to ========== Return NavigationContext from registerLoadRequest. This CL (6 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). This CL makes registerLoadRequest API return NavigationContext object, which can be used to store NavigationContext in CRWWKNavigationStates. BUG=713836, 712269 ==========
Description was changed from ========== Return NavigationContext from registerLoadRequest. This CL (6 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). This CL makes registerLoadRequest API return NavigationContext object, which can be used to store NavigationContext in CRWWKNavigationStates. BUG=713836, 712269 ========== to ========== Return NavigationContext from registerLoadRequest. This CL (3 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). This CL makes registerLoadRequest API return NavigationContext object, which can be used to store NavigationContext in CRWWKNavigationStates. BUG=713836, 712269 ==========
eugenebut@chromium.org changed reviewers: + kkhorimoto@chromium.org
https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/js/cr... File ios/web/web_state/js/crw_js_post_request_loader.h (right): https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/js/cr... ios/web/web_state/js/crw_js_post_request_loader.h:21: - (WKNavigation*)loadPOSTRequest:(NSURLRequest*)request This is a change from dependent CL/2840473003, I don't know why it shows up here
I'm hesitant to lgtm until I get to reading your subsequent CLs, as it's somewhat unclear to me how these changes are used. Also, I can't tell if your comment regarding server redirects is implemented in a later CL, or if it's a bug that the implementation doesn't match the comment in this CL. https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1832: transition:self.currentTransition]; I'm assuming that this will be utilized in a later CL? https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4510: // It is fine to ignore returned NavigationContet as it did not s/Contet/Context https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4511: // change after the redirect. I don't really understand this comment. It looks like |-registerLoadRequest.....| creates a new NavigationContextImpl every time it's called, regardless of whether it's a sever redirect. Maybe reword this comment (for this CL), then changing it back to this once you update |-registerLoadRequest|?
>> I'm hesitant to lgtm until I get to reading your subsequent CLs, as it's somewhat unclear to me how these changes are used. This CL should explain more: https://codereview.chromium.org/2842443002/ https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1832: transition:self.currentTransition]; On 2017/04/25 13:45:50, kkhorimoto_ wrote: > I'm assuming that this will be utilized in a later CL? Yes, here: https://codereview.chromium.org/2842443002/diff/1/ios/web/web_state/ui/crw_we... https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4510: // It is fine to ignore returned NavigationContet as it did not On 2017/04/25 13:45:50, kkhorimoto_ wrote: > s/Contet/Context Done. https://codereview.chromium.org/2834413002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:4511: // change after the redirect. On 2017/04/25 13:45:50, kkhorimoto_ wrote: > I don't really understand this comment. It looks like > |-registerLoadRequest.....| creates a new NavigationContextImpl every time it's > called, regardless of whether it's a sever redirect. Maybe reword this comment > (for this CL), then changing it back to this once you update > |-registerLoadRequest|? Updated the comment. Please let me know if it is still confusing.
lgtm thanks
The CQ bit was checked by eugenebut@chromium.org
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/2834413002/#ps60001 (title: "Rebased")
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": 60001, "attempt_start_ts": 1494291022825030,
"parent_rev": "f1c5a372002a6a2367c19a3d6100fc69ef5a7ccc", "commit_rev":
"f97229b40443d92c57c3a08ebef8aff466932da0"}
Message was sent while issue was closed.
Description was changed from ========== Return NavigationContext from registerLoadRequest. This CL (3 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). This CL makes registerLoadRequest API return NavigationContext object, which can be used to store NavigationContext in CRWWKNavigationStates. BUG=713836, 712269 ========== to ========== Return NavigationContext from registerLoadRequest. This CL (3 of 7) is a part of large refactoring to create NavigationContext when navigation was started, not when it is finished. This refactoring will allow to pass the same instance of NavigationContext to DidStartNavigation and DidFinishNavigation (crbug.com/713836). Also this will allow to associate NavigationItem with NavigationContext object to commit non-latest navigation (crbug.com/712269). This CL makes registerLoadRequest API return NavigationContext object, which can be used to store NavigationContext in CRWWKNavigationStates. BUG=713836, 712269 Review-Url: https://codereview.chromium.org/2834413002 Cr-Commit-Position: refs/heads/master@{#470223} Committed: https://chromium.googlesource.com/chromium/src/+/f97229b40443d92c57c3a08ebef8... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/f97229b40443d92c57c3a08ebef8... |
