Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2650563002:
Pass WebState to NativeAppNavigationController (Closed)
|
Created:
3 years, 11 months ago by pkl (ping after 24h if needed) Modified:
3 years, 11 months ago CC:
chromium-reviews, marq+watch_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPass WebState to NativeAppNavigationController
This is the initial step of getting rid of Tab from
NativeAppNavigationController. In the interim, both WebState and Tab
are passed and NativeAppNavigationController will prefer the use
of WebState over that of Tab whenever possible.
BUG=681867
Review-Url: https://codereview.chromium.org/2650563002
Cr-Commit-Position: refs/heads/master@{#446577}
Committed: https://chromium.googlesource.com/chromium/src/+/b28df93e8b400bb452521c8e9a21a41de8771df9
Patch Set 1 #Patch Set 2 : Get NavigationManager from WebState #Patch Set 3 : mostly indentation #Patch Set 4 : Add chrome_web_test support #
Total comments: 18
Patch Set 5 : IsLinkNavigation() is now a util function w/ unit tests #
Total comments: 51
Patch Set 6 : addressed another round of comments. #Patch Set 7 : removed unnecessary DEPS #Patch Set 8 : fixed import vs. include #Messages
Total messages: 23 (11 generated)
pkl@chromium.org changed reviewers: + eugenebut@google.com, rohitrao@chromium.org
This removed most of the _tab references in NativeAppNavigationController.
eugenebut@chromium.org changed reviewers: + eugenebut@chromium.org
https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm (right): https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:23: #include "ios/web/public/web_state/web_state.h" s/include/import https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:187: // TODO(pkl): Preferred method is to add a helper object to WebState Please use |TODO(crbug.com/):| format https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:218: web::NavigationManager* navigationManager = _webState->GetNavigationManager(); Should we move this code to utility function, so it can be tested? The logic here is quite non trivial... https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:219: if (navigationManager) { nit: do you want to keep original logic with early return to avoid indentations? https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:220: NSInteger index = navigationManager->GetCurrentItemIndex(); Maybe s/NSInteger/int ?
Description was changed from ========== Pass WebState to NativeAppNavigationController This is the initial step of getting rid of Tab from NativeAppNavigationController. In the interim, both WebState and Tab are passed and NativeAppNavigationController will prefer the use of WebState over that of Tab whenever possible. ========== to ========== Pass WebState to NativeAppNavigationController This is the initial step of getting rid of Tab from NativeAppNavigationController. In the interim, both WebState and Tab are passed and NativeAppNavigationController will prefer the use of WebState over that of Tab whenever possible. ==========
eugenebut@chromium.org changed reviewers: - eugenebut@google.com
Description was changed from ========== Pass WebState to NativeAppNavigationController This is the initial step of getting rid of Tab from NativeAppNavigationController. In the interim, both WebState and Tab are passed and NativeAppNavigationController will prefer the use of WebState over that of Tab whenever possible. ========== to ========== Pass WebState to NativeAppNavigationController This is the initial step of getting rid of Tab from NativeAppNavigationController. In the interim, both WebState and Tab are passed and NativeAppNavigationController will prefer the use of WebState over that of Tab whenever possible. BUG=681867 ==========
lgtm https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h (right): https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h:30: - (instancetype)init NS_UNAVAILABLE; We generally put the unavailable initializers after the designated one. That should also make the comment unnecessary. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm (right): https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:64: - (instancetype)initWithWebState:(web::WebState*)webState I can't decide whether we should pass both WebState and Tab, or if we should just pass Tab and get the WebState from it. Either way, we'll need to change callers at some point in the future. If you do leave it this way, we should at least DCHECK_EQ(webState, tab.webState). https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:188: // and use the helper object to launch Store Kit. The intent is to do this in a separate CL? https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (left): https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:47: @class WebControllerSnapshotHelper; These changes look unrelated, could be removed from this CL.
Addressed comments from eugenebut & rohitrao. Please let me know if there's a better way to do the unit tests for IsLinkNavigation(). Thanks! https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h (right): https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.h:30: - (instancetype)init NS_UNAVAILABLE; On 2017/01/24 14:24:56, rohitrao wrote: > We generally put the unavailable initializers after the designated one. That > should also make the comment unnecessary. Done. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm (right): https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:23: #include "ios/web/public/web_state/web_state.h" On 2017/01/21 02:30:08, Eugene But wrote: > s/include/import Since web_state.h is pure C++ class WebState, shouldn't #include be used? https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:64: - (instancetype)initWithWebState:(web::WebState*)webState On 2017/01/24 14:24:56, rohitrao wrote: > I can't decide whether we should pass both WebState and Tab, or if we should > just pass Tab and get the WebState from it. Either way, we'll need to change > callers at some point in the future. > > If you do leave it this way, we should at least DCHECK_EQ(webState, > tab.webState). My intent is that we can stop passing in |tab| eventually. NativeAppNavigationController should not need a |tab| as ivar. Everything can be done through |webState|. Added a couple of DCHECKs here. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:187: // TODO(pkl): Preferred method is to add a helper object to WebState On 2017/01/21 02:30:08, Eugene But wrote: > Please use |TODO(crbug.com/):| format Done. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:188: // and use the helper object to launch Store Kit. On 2017/01/24 14:24:56, rohitrao wrote: > The intent is to do this in a separate CL? Yes. I've updated the comment to include a crbug reference to check. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:218: web::NavigationManager* navigationManager = _webState->GetNavigationManager(); On 2017/01/21 02:30:08, Eugene But wrote: > Should we move this code to utility function, so it can be tested? The logic > here is quite non trivial... Done. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:219: if (navigationManager) { On 2017/01/21 02:30:08, Eugene But wrote: > nit: do you want to keep original logic with early return to avoid indentations? Reverted. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:220: NSInteger index = navigationManager->GetCurrentItemIndex(); On 2017/01/21 02:30:08, Eugene But wrote: > Maybe s/NSInteger/int ? Done. https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.h (left): https://codereview.chromium.org/2650563002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.h:47: @class WebControllerSnapshotHelper; On 2017/01/24 14:24:56, rohitrao wrote: > These changes look unrelated, could be removed from this CL. Reverted.
Thanks for the tests! lgtm https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:57: "native_app_navigation_utils.h", Optional nit: util. suffix is much more common than utils. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/DEPS (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/DEPS:2: # TODO(crbug.com/619984): Remove this exception. crbug.com/619984 is about using GetLastUserItem, which you just removed. Can we remove native_app_navigation_controller\.mm$ exception now? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/DEPS:7: "+ios/web/navigation/navigation_manager_impl.h", Do we need this exception? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/DEPS:9: "^native_app_navigation_utils_unittest\.mm$": [ Could you please file a bug to remove private API usage from unittest. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:22: #include "ios/web/public/web_state/web_state.h" s/include/import https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:132: onLinkNavigation:native_app_launcher::IsLinkNavigation( nit: Do you want to use local variable for native_app_launcher::IsLinkNavigation? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils.h (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.h:14: // Returns whether the current state is Link Navigation in the sense of Native Should this be "Returns whether the last navigation is Link Navigation..."? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.h:17: bool IsLinkNavigation(web::WebState* webState); s/webState/web_state https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:8: #import "ios/web/navigation/navigation_manager_impl.h" do we need this include? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:10: #include "ios/web/public/web_state/web_state.h" s/include/import https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:14: bool IsLinkNavigation(web::WebState* webState) { Please use C++ Style for variables https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:17: return false; Can this ever happen? I think NavigationManager lifetime is the same as WebState lifetime. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:19: // Walks backward on the navigation items for the first item that is not a nit: s/Walks/Walk https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:8: #include "ios/web/navigation/navigation_manager_impl.h" s/include/import https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:15: namespace { Optional nit: There is no need to put tests into anonymous namespace. Conceptually they belong to the same package as code. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:31: web_state_.reset(); Is there a reason for destroying web_state before destructor? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:35: web::WebState* web_state() { return web_state_->GetWebState(); } return web_state_.get(); https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:37: void AddPendingEntry(const std::string& URL, ui::PageTransition transition) { AddCommittedItem? (s/Pending/Committed, s/Entry/Item) https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:37: void AddPendingEntry(const std::string& URL, ui::PageTransition transition) { s/URL/url_spec ? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:38: CRWSessionController* sessionController = s/sessionController/session_controller https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:57: TEST_F(NativeAppNavigationUtilsTest, TestTypedURL) { Optional nit: According to C++ Style Guide this should be TestTypedUrl though we extremely inconsistent with URL capitalization, so it's up to you. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:72: EXPECT_TRUE(native_app_launcher::IsLinkNavigation(web_state())); Sorry, missed in early reviews, when we did not have unit tests. Is it reasonable to return true for Bookmarks, which are not a link click?
Thank you! Addressed more comments. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/BUILD.gn (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/BUILD.gn:57: "native_app_navigation_utils.h", On 2017/01/26 01:04:38, Eugene But wrote: > Optional nit: util. suffix is much more common than utils. Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/DEPS (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/DEPS:2: # TODO(crbug.com/619984): Remove this exception. On 2017/01/26 01:04:38, Eugene But wrote: > crbug.com/619984 is about using GetLastUserItem, which you just removed. Can we > remove native_app_navigation_controller\.mm$ exception now? Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/DEPS:7: "+ios/web/navigation/navigation_manager_impl.h", On 2017/01/26 01:04:38, Eugene But wrote: > Do we need this exception? Yes, it is needed. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/DEPS:9: "^native_app_navigation_utils_unittest\.mm$": [ On 2017/01/26 01:04:38, Eugene But wrote: > Could you please file a bug to remove private API usage from unittest. I recycled the crbug quoted above because it is really the same violation but moved to a different file. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:22: #include "ios/web/public/web_state/web_state.h" On 2017/01/26 01:04:38, Eugene But wrote: > s/include/import Since web_state.h defines a C++ class, isn't #include correct? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:132: onLinkNavigation:native_app_launcher::IsLinkNavigation( On 2017/01/26 01:04:38, Eugene But wrote: > nit: Do you want to use local variable for > native_app_launcher::IsLinkNavigation? I think so since WebState is not going to change between the 2 calls. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils.h (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.h:14: // Returns whether the current state is Link Navigation in the sense of Native On 2017/01/26 01:04:38, Eugene But wrote: > Should this be "Returns whether the last navigation is Link Navigation..."? Clarified. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.h:17: bool IsLinkNavigation(web::WebState* webState); On 2017/01/26 01:04:38, Eugene But wrote: > s/webState/web_state Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:8: #import "ios/web/navigation/navigation_manager_impl.h" On 2017/01/26 01:04:38, Eugene But wrote: > do we need this include? Not this one, but we do need ios/web/public/navigation_manager.h. Thanks for catching this. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:10: #include "ios/web/public/web_state/web_state.h" On 2017/01/26 01:04:38, Eugene But wrote: > s/include/import web_state.h is all C++ code, so should be #include (not #import). https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:14: bool IsLinkNavigation(web::WebState* webState) { On 2017/01/26 01:04:39, Eugene But wrote: > Please use C++ Style for variables Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:17: return false; On 2017/01/26 01:04:38, Eugene But wrote: > Can this ever happen? I think NavigationManager lifetime is the same as WebState > lifetime. web_state.h says "Can never return null". Cool! Turning this into a DCHECK. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:19: // Walks backward on the navigation items for the first item that is not a On 2017/01/26 01:04:38, Eugene But wrote: > nit: s/Walks/Walk I recall that comments should be third-person singular, e.g. "Returns foo" and not "Return foo". In this case, "Walks". https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:8: #include "ios/web/navigation/navigation_manager_impl.h" On 2017/01/26 01:04:39, Eugene But wrote: > s/include/import But navigation_manager_impl defines a C++ class, so should be #include, right? https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:15: namespace { On 2017/01/26 01:04:39, Eugene But wrote: > Optional nit: There is no need to put tests into anonymous namespace. > Conceptually they belong to the same package as code. There are differing opinions on putting test code inside anonymous namespace. There are some marginal benefits, e.g. 2 tests that defined to have the same name (via accidental cut and paste, for example). https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:31: web_state_.reset(); On 2017/01/26 01:04:39, Eugene But wrote: > Is there a reason for destroying web_state before destructor? 1. WebTestWithWebState does that. 2. It feels right to destroy it before superclass' TearDown is called. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:35: web::WebState* web_state() { return web_state_->GetWebState(); } On 2017/01/26 01:04:39, Eugene But wrote: > return web_state_.get(); web_state_.get() returns a WebStateImpl* which is too much information. GetWebState() returns a WebState* which is exactly what is needed. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:37: void AddPendingEntry(const std::string& URL, ui::PageTransition transition) { On 2017/01/26 01:04:39, Eugene But wrote: > AddCommittedItem? (s/Pending/Committed, s/Entry/Item) Yes, good point! Renamed to just AddItem. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:37: void AddPendingEntry(const std::string& URL, ui::PageTransition transition) { On 2017/01/26 01:04:39, Eugene But wrote: > s/URL/url_spec ? Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:38: CRWSessionController* sessionController = On 2017/01/26 01:04:39, Eugene But wrote: > s/sessionController/session_controller Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:57: TEST_F(NativeAppNavigationUtilsTest, TestTypedURL) { On 2017/01/26 01:04:39, Eugene But wrote: > Optional nit: According to C++ Style Guide this should be TestTypedUrl > > though we extremely inconsistent with URL capitalization, so it's up to you. Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:72: EXPECT_TRUE(native_app_launcher::IsLinkNavigation(web_state())); On 2017/01/26 01:04:39, Eugene But wrote: > Sorry, missed in early reviews, when we did not have unit tests. Is it > reasonable to return true for Bookmarks, which are not a link click? Done.
Still lgtm! Replaying since you asked questions. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_controller.mm:22: #include "ios/web/public/web_state/web_state.h" On 2017/01/26 02:26:28, pklpkl wrote: > On 2017/01/26 01:04:38, Eugene But wrote: > > s/include/import > > Since web_state.h defines a C++ class, isn't #include correct? web_state.h is an Objective-C header (it has @class CRWJSInjectionReceiver;) and per Style Guide supposed to be imported. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:10: #include "ios/web/public/web_state/web_state.h" On 2017/01/26 02:26:28, pklpkl wrote: > On 2017/01/26 01:04:38, Eugene But wrote: > > s/include/import > > web_state.h is all C++ code, so should be #include (not #import). Line 26 is Objective-C code. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:19: // Walks backward on the navigation items for the first item that is not a On 2017/01/26 02:26:28, pklpkl wrote: > On 2017/01/26 01:04:38, Eugene But wrote: > > nit: s/Walks/Walk > > I recall that comments should be third-person singular, e.g. "Returns foo" and > not "Return foo". In this case, "Walks". I guess you referring to Style Guide: "Use descriptive form ("Opens the file") rather than imperative form ("Open the file") for method and function comments.". So we normally use descriptive for functions and method, but it's not an issue. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:8: #include "ios/web/navigation/navigation_manager_impl.h" On 2017/01/26 02:26:29, pklpkl wrote: > On 2017/01/26 01:04:39, Eugene But wrote: > > s/include/import > > But navigation_manager_impl defines a C++ class, so should be #include, right? Line 19 is Objective-C code.
Thanks! Submitting. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:10: #include "ios/web/public/web_state/web_state.h" On 2017/01/26 03:10:02, Eugene But wrote: > On 2017/01/26 02:26:28, pklpkl wrote: > > On 2017/01/26 01:04:38, Eugene But wrote: > > > s/include/import > > > > web_state.h is all C++ code, so should be #include (not #import). > Line 26 is Objective-C code. Done. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils.mm:19: // Walks backward on the navigation items for the first item that is not a On 2017/01/26 03:10:02, Eugene But wrote: > On 2017/01/26 02:26:28, pklpkl wrote: > > On 2017/01/26 01:04:38, Eugene But wrote: > > > nit: s/Walks/Walk > > > > I recall that comments should be third-person singular, e.g. "Returns foo" and > > not "Return foo". In this case, "Walks". > I guess you referring to Style Guide: "Use descriptive form ("Opens the file") > rather than imperative form ("Open the file") for method and function > comments.". So we normally use descriptive for functions and method, but it's > not an issue. Acknowledged. https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... File ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm (right): https://codereview.chromium.org/2650563002/diff/80001/ios/chrome/browser/nati... ios/chrome/browser/native_app_launcher/native_app_navigation_utils_unittest.mm:8: #include "ios/web/navigation/navigation_manager_impl.h" On 2017/01/26 03:10:02, Eugene But wrote: > On 2017/01/26 02:26:29, pklpkl wrote: > > On 2017/01/26 01:04:39, Eugene But wrote: > > > s/include/import > > > > But navigation_manager_impl defines a C++ class, so should be #include, right? > Line 19 is Objective-C code. Done.
The CQ bit was checked by pkl@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rohitrao@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2650563002/#ps140001 (title: "fixed import vs. include")
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: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by pkl@chromium.org
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": 1485481974463840,
"parent_rev": "7169140ae183cec72446399946a4cd0b79ea6dec", "commit_rev":
"b28df93e8b400bb452521c8e9a21a41de8771df9"}
Message was sent while issue was closed.
Description was changed from ========== Pass WebState to NativeAppNavigationController This is the initial step of getting rid of Tab from NativeAppNavigationController. In the interim, both WebState and Tab are passed and NativeAppNavigationController will prefer the use of WebState over that of Tab whenever possible. BUG=681867 ========== to ========== Pass WebState to NativeAppNavigationController This is the initial step of getting rid of Tab from NativeAppNavigationController. In the interim, both WebState and Tab are passed and NativeAppNavigationController will prefer the use of WebState over that of Tab whenever possible. BUG=681867 Review-Url: https://codereview.chromium.org/2650563002 Cr-Commit-Position: refs/heads/master@{#446577} Committed: https://chromium.googlesource.com/chromium/src/+/b28df93e8b400bb452521c8e9a21... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/b28df93e8b400bb452521c8e9a21... |
