Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2835463002:
Store NavigationIntext in CRWWKNavigationStates. (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. |
DescriptionStore NavigationContext in CRWWKNavigationStates.
This CL (1 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 adds API to CRWWKNavigationStates to allow associating
NavigationContextImpl with NKNavigation objects. This will allow to understand
which NavigationContextImpl should be used in WKNavigationDelegate callbacks.
BUG=713836, 712269
Review-Url: https://codereview.chromium.org/2835463002
Cr-Commit-Position: refs/heads/master@{#470068}
Committed: https://chromium.googlesource.com/chromium/src/+/5ef672aa5ffed4146ede2c1d22a04fb91dd5f168
Patch Set 1 #Patch Set 2 : Self review #Patch Set 3 : Fixed compilation #Patch Set 4 : Added linebreak #Patch Set 5 : Rebased #
Total comments: 7
Patch Set 6 : Addressed review comments #Patch Set 7 : Rebased #Patch Set 8 : Fixed compilation. #
Messages
Total messages: 33 (21 generated)
The CQ bit was checked by eugenebut@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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
The CQ bit was checked by eugenebut@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.
Description was changed from ========== Store NavigationIntext in CRWWKNavigationStates. BUG= ========== to ========== Store NavigationContext in CRWWKNavigationStates. This CL (1 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 adds API to CRWWKNavigationStates to allow associating NavigationContextImpl with NKNavigation objects. This will allow to understand which NavigationContextImpl should be used in WKNavigationDelegate callbacks. BUG=713836,712269 ==========
Description was changed from ========== Store NavigationContext in CRWWKNavigationStates. This CL (1 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 adds API to CRWWKNavigationStates to allow associating NavigationContextImpl with NKNavigation objects. This will allow to understand which NavigationContextImpl should be used in WKNavigationDelegate callbacks. BUG=713836,712269 ========== to ========== Store NavigationContext in CRWWKNavigationStates. 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 adds API to CRWWKNavigationStates to allow associating NavigationContextImpl with NKNavigation objects. This will allow to understand which NavigationContextImpl should be used in WKNavigationDelegate callbacks. BUG=713836,712269 ==========
Description was changed from ========== Store NavigationContext in CRWWKNavigationStates. 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 adds API to CRWWKNavigationStates to allow associating NavigationContextImpl with NKNavigation objects. This will allow to understand which NavigationContextImpl should be used in WKNavigationDelegate callbacks. BUG=713836,712269 ========== to ========== Store NavigationContext in CRWWKNavigationStates. This CL (1 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 adds API to CRWWKNavigationStates to allow associating NavigationContextImpl with NKNavigation objects. This will allow to understand which NavigationContextImpl should be used in WKNavigationDelegate callbacks. BUG=713836,712269 ==========
eugenebut@chromium.org changed reviewers: + kkhorimoto@chromium.org
https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_navigation_states.h (right): https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_navigation_states.h:62: // affect the result of |lastAddedNavigation| method. Passed |navigation| will Optional: I don't think "Passed" is really necessary here since |navigation| with the lines unambiguously refers to the passed argument. Same goes for the similar comment above. https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_navigation_states.mm (right): https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_navigation_states.mm:35: - (web::NavigationContextImpl*)context; Optional: Personally I prefer classes have readonly properties that exposes pointers to owned C++ objects with an explicitly defined setter that takes a unique_ptr (see CRWSessionStorage). It doesn't change much, but it allows you to use property notation to access what is essentially a property here. https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_navigation_states.mm:42: } Can you move the ivars to the @interface?
https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_navigation_states.h (right): https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_navigation_states.h:62: // affect the result of |lastAddedNavigation| method. Passed |navigation| will On 2017/04/25 13:29:55, kkhorimoto_ wrote: > Optional: I don't think "Passed" is really necessary here since |navigation| > with the lines unambiguously refers to the passed argument. Same goes for the > similar comment above. Done. https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_navigation_states.mm (right): https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_navigation_states.mm:35: - (web::NavigationContextImpl*)context; On 2017/04/25 13:29:55, kkhorimoto_ wrote: > Optional: Personally I prefer classes have readonly properties that exposes > pointers to owned C++ objects with an explicitly defined setter that takes a > unique_ptr (see CRWSessionStorage). It doesn't change much, but it allows you > to use property notation to access what is essentially a property here. Done. https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_navigation_states.mm:42: } On 2017/04/25 13:29:55, kkhorimoto_ wrote: > Can you move the ivars to the @interface? Done.
lgtm
The CQ bit was checked by eugenebut@chromium.org
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
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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/2835463002/#ps120001 (title: "Rebased")
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
Try jobs failed on following builders: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_navigation_states.mm (right): https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_navigation_states.mm:35: - (web::NavigationContextImpl*)context; On 2017/04/25 21:02:58, Eugene But (OOO till May 8) wrote: > On 2017/04/25 13:29:55, kkhorimoto_ wrote: > > Optional: Personally I prefer classes have readonly properties that exposes > > pointers to owned C++ objects with an explicitly defined setter that takes a > > unique_ptr (see CRWSessionStorage). It doesn't change much, but it allows you > > to use property notation to access what is essentially a property here. > > Done. Turns out property does not work on bots: ../../ios/web/web_state/ui/crw_wk_navigation_states.mm:25:68: error: type of property 'context' does not match type of accessor 'setContext:' [-Werror] @property(nonatomic, assign, readonly) web::NavigationContextImpl* context; ^ ../../ios/web/web_state/ui/crw_wk_navigation_states.mm:38:1: note: declared here - (void)setContext:(std::unique_ptr<web::NavigationContextImpl>)context;
On 2017/05/08 18:30:52, Eugene But (OOO till May 8) wrote: > https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... > File ios/web/web_state/ui/crw_wk_navigation_states.mm (right): > > https://codereview.chromium.org/2835463002/diff/80001/ios/web/web_state/ui/cr... > ios/web/web_state/ui/crw_wk_navigation_states.mm:35: - > (web::NavigationContextImpl*)context; > On 2017/04/25 21:02:58, Eugene But (OOO till May 8) wrote: > > On 2017/04/25 13:29:55, kkhorimoto_ wrote: > > > Optional: Personally I prefer classes have readonly properties that exposes > > > pointers to owned C++ objects with an explicitly defined setter that takes a > > > unique_ptr (see CRWSessionStorage). It doesn't change much, but it allows > you > > > to use property notation to access what is essentially a property here. > > > > Done. > Turns out property does not work on bots: > > > ../../ios/web/web_state/ui/crw_wk_navigation_states.mm:25:68: error: type of > property 'context' does not match type of accessor 'setContext:' [-Werror] > @property(nonatomic, assign, readonly) web::NavigationContextImpl* context; > ^ > ../../ios/web/web_state/ui/crw_wk_navigation_states.mm:38:1: note: declared here > - (void)setContext:(std::unique_ptr<web::NavigationContextImpl>)context; Oh interesting. I just realized that I didn't exactly follow ObjC property naming paradigms when I introduced this earlier in CRWSessionStorage, so this wasn't a problem when I tried: https://cs.chromium.org/chromium/src/ios/web/public/crw_session_storage.h?q=c...
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/2835463002/#ps140001 (title: "Fixed compilation.")
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": 140001, "attempt_start_ts": 1494268675342720,
"parent_rev": "17ae9f3ad85e9ad139bd283908ba4c0529659fce", "commit_rev":
"5ef672aa5ffed4146ede2c1d22a04fb91dd5f168"}
Message was sent while issue was closed.
Description was changed from ========== Store NavigationContext in CRWWKNavigationStates. This CL (1 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 adds API to CRWWKNavigationStates to allow associating NavigationContextImpl with NKNavigation objects. This will allow to understand which NavigationContextImpl should be used in WKNavigationDelegate callbacks. BUG=713836,712269 ========== to ========== Store NavigationContext in CRWWKNavigationStates. This CL (1 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 adds API to CRWWKNavigationStates to allow associating NavigationContextImpl with NKNavigation objects. This will allow to understand which NavigationContextImpl should be used in WKNavigationDelegate callbacks. BUG=713836,712269 Review-Url: https://codereview.chromium.org/2835463002 Cr-Commit-Position: refs/heads/master@{#470068} Committed: https://chromium.googlesource.com/chromium/src/+/5ef672aa5ffed4146ede2c1d22a0... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/5ef672aa5ffed4146ede2c1d22a0... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
