Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2698773002:
[iOS] Refactoring web CRWSessionController user agent code. (Closed)
Description[iOS] Refactoring CRWSessionController web user agent code.
This CL refactoring web agent code by moving them from
CRWSessionController to NavigationManager, and updates corresponding
references and unit tests to call methods directly from
NavigationManager instead of CRWSessionController.
This CL also introduces NavigationInitiationType, consisting of
USER_INITIATED and RENDERER_INITIATED, to avoid passing boolean
values as parameters to C++ functions, which is error prone and
un-maintainable.
BUG=678047
Review-Url: https://codereview.chromium.org/2698773002
Cr-Commit-Position: refs/heads/master@{#451698}
Committed: https://chromium.googlesource.com/chromium/src/+/6e5d49d75e64a08cbe73b4a39bb0a333417f3170
Patch Set 1 #
Total comments: 25
Patch Set 2 : Addressed feedback #
Total comments: 15
Patch Set 3 : Addressed feedback #
Total comments: 16
Patch Set 4 : Addressed feedback #Patch Set 5 : Add comments #Patch Set 6 : Rename enum class name and reformatting #
Total comments: 9
Patch Set 7 : Make navigationManagerImpl as a readonly property #Patch Set 8 : Update NavigationItemImpl to use NavigationInitiationType #
Total comments: 6
Patch Set 9 : Update CRWSessionController to use NavigationInitiationType #Patch Set 10 : Fix unit tests and rebase #Messages
Total messages: 62 (32 generated)
Description was changed from ========== Move user agent code from CRWSessionController to NavigationManager BUG= ========== to ========== [iOS] Refactoring web user agent code. This CL refactoring web agent code by moving them from CRWSessionController to NavigationManager, and updates corresponding references and unit tests to call methods directly from NavigationManager instead of CRWSessionController. BUG=678047 ==========
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Hi Eugene, Kurt Please take a look. Thank you very much!
Added a comment. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:377: No structural changes. Just happened to find this piece of code extremely hard to read, so did a little bit refactoring.
https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.h (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.h:102: // pending, and will be lost unless |-commitPendingItem| is called. I think the original comment is less ambiguous here since it's talking directly about the input parameters. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:370: BOOL isPendingItemEqualsCurrentItem = NO; As long as we're refactoring this for readability, why don't we just define this variable later and initialize it directly to its final value? Additionally, it might improve readability by naming the variable in terms of its effect in the overall function (i.e. whether or not to create the new pending item) BOOL shouldCreatePendingItem = !hasSameURL || isPendingTransitionFormSubmit && !isCurrentTransitionFormSubmit; https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:372: BOOL hasSameUrl = item->GetURL() == url; s/hasSameUrl/hasSameURL https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:375: BOOL isCurrentItemPageTransitionFormSubmit = PageTransitionCoreTypeIs( You can make these variable names a little less verbose: isPendingTransitionFormSubmit isCurrentTransitionFormSubmit https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:400: useDesktopUserAgent:NO useDesktopUserAgent is always NO after this change right? We can just remove that parameter from the session entry creation function since it's set only in NavigationManagerImpl. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:133: // Removes this method once crbug.com/692303 is fixed. This comment isn't really necessary since it's already in navigation_manager.h https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:170: void AddPendingItem(const GURL& url, Why not just expose this in the public interface instead of creating two functions above? https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:173: bool rendererInitiated); s/rendererInitiated/renderer_initiated https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.mm:432: bool rendererInitiated) { s/rendererInitiated/renderer_initiated https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl_unittest.mm (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl_unittest.mm:499: EXPECT_FALSE(visible_item->IsOverridingUserAgent()); Does this pass? Shouldn't the second piece of logic from these lines in navigation_manager_impl persist the visible item's UA overriding flag to subsequent pending items? 438 bool use_desktop_user_agent = 439 override_desktop_user_agent_for_next_pending_item_ || 440 (GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent()); https://codereview.chromium.org/2698773002/diff/1/ios/web/public/navigation_m... File ios/web/public/navigation_manager.h (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/public/navigation_m... ios/web/public/navigation_manager.h:154: // Removes this method once crbug.com/692303 is fixed. Please reformat this comment as a todo: TODO(crbug.com/692303): Remove this when overriding the user agent doesn't create a new NavigationItem.
PTAL. Thank you very much! https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.h (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.h:102: // pending, and will be lost unless |-commitPendingItem| is called. On 2017/02/15 23:02:05, kkhorimoto_ wrote: > I think the original comment is less ambiguous here since it's talking directly > about the input parameters. But the implementation details is not consistent with the comments, it not only takes url into consideration, but also page transition types. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:370: BOOL isPendingItemEqualsCurrentItem = NO; On 2017/02/15 23:02:05, kkhorimoto_ wrote: > As long as we're refactoring this for readability, why don't we just define this > variable later and initialize it directly to its final value? Additionally, it > might improve readability by naming the variable in terms of its effect in the > overall function (i.e. whether or not to create the new pending item) > > BOOL shouldCreatePendingItem = !hasSameURL || isPendingTransitionFormSubmit && > !isCurrentTransitionFormSubmit; Makes sense! https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:370: BOOL isPendingItemEqualsCurrentItem = NO; On 2017/02/15 23:02:05, kkhorimoto_ wrote: > As long as we're refactoring this for readability, why don't we just define this > variable later and initialize it directly to its final value? Additionally, it > might improve readability by naming the variable in terms of its effect in the > overall function (i.e. whether or not to create the new pending item) > > BOOL shouldCreatePendingItem = !hasSameURL || isPendingTransitionFormSubmit && > !isCurrentTransitionFormSubmit; Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:372: BOOL hasSameUrl = item->GetURL() == url; On 2017/02/15 23:02:06, kkhorimoto_ wrote: > s/hasSameUrl/hasSameURL Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:375: BOOL isCurrentItemPageTransitionFormSubmit = PageTransitionCoreTypeIs( On 2017/02/15 23:02:05, kkhorimoto_ wrote: > You can make these variable names a little less verbose: > > isPendingTransitionFormSubmit > isCurrentTransitionFormSubmit Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:377: On 2017/02/15 22:26:51, liaoyuke wrote: > No structural changes. Just happened to find this piece of code extremely hard > to read, so did a little bit refactoring. Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/crw_sess... ios/web/navigation/crw_session_controller.mm:400: useDesktopUserAgent:NO On 2017/02/15 23:02:05, kkhorimoto_ wrote: > useDesktopUserAgent is always NO after this change right? We can just remove > that parameter from the session entry creation function since it's set only in > NavigationManagerImpl. Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:133: // Removes this method once crbug.com/692303 is fixed. On 2017/02/15 23:02:06, kkhorimoto_ wrote: > This comment isn't really necessary since it's already in navigation_manager.h Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:170: void AddPendingItem(const GURL& url, On 2017/02/15 23:02:06, kkhorimoto_ wrote: > Why not just expose this in the public interface instead of creating two > functions above? I feel like having a boolean value as a function parameter is error prone in C++. In Objective C, by calling the function, you would easily know that the name of the parameter is rendererInitiated, but in C++, you just pass in the parameter, and someone may easily mistaken the parameter name as userInitiated. What do you think? https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.h:173: bool rendererInitiated); On 2017/02/15 23:02:06, kkhorimoto_ wrote: > s/rendererInitiated/renderer_initiated Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl.mm:432: bool rendererInitiated) { On 2017/02/15 23:02:06, kkhorimoto_ wrote: > s/rendererInitiated/renderer_initiated Done. https://codereview.chromium.org/2698773002/diff/1/ios/web/public/navigation_m... File ios/web/public/navigation_manager.h (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/public/navigation_m... ios/web/public/navigation_manager.h:154: // Removes this method once crbug.com/692303 is fixed. On 2017/02/15 23:02:06, kkhorimoto_ wrote: > Please reformat this comment as a todo: > > TODO(crbug.com/692303): Remove this when overriding the user agent doesn't > create a new NavigationItem. Done.
https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:513: self.currentEntry.navigationItem->IsOverridingUserAgent(); nit: How about this?: pushedItem->SetIsOverridingUserAgent(currentItem->IsOverridingUserAgent()); https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:88: // out as pending, and will be lost unless |-commitPendingItem| is called. Please document the difference between UserInitiated and RendererInitiated https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:184: } nit: add a linebreak https://codereview.chromium.org/2698773002/diff/20001/ios/web/public/test/fak... File ios/web/public/test/fakes/test_navigation_manager.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/public/test/fak... ios/web/public/test/fakes/test_navigation_manager.mm:144: return; Drop this https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1572: if (_webStateImpl->GetNavigationManagerImpl().GetPendingItem()) { nit: Do you want to create a local variable for NavigationManagerImpl? https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1917: _webStateImpl->GetNavigationManagerImpl().AddRendererInitiatedPendingItem( Having 2 APIs for adding pendingItem looks error prone. How about creating a enum inside navigation_manager_impl.h?: class enum PendingItemType { USER_INITIATED = 1, RENDERER_INITIATED = 1, };
PTAL! Thank you very much! https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... File ios/web/navigation/navigation_manager_impl_unittest.mm (right): https://codereview.chromium.org/2698773002/diff/1/ios/web/navigation/navigati... ios/web/navigation/navigation_manager_impl_unittest.mm:499: EXPECT_FALSE(visible_item->IsOverridingUserAgent()); On 2017/02/15 23:02:06, kkhorimoto_ wrote: > Does this pass? Shouldn't the second piece of logic from these lines in > navigation_manager_impl persist the visible item's UA overriding flag to > subsequent pending items? > > 438 bool use_desktop_user_agent = > 439 override_desktop_user_agent_for_next_pending_item_ || > 440 (GetVisibleItem() && GetVisibleItem()->IsOverridingUserAgent()); Done. https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:513: self.currentEntry.navigationItem->IsOverridingUserAgent(); On 2017/02/16 02:20:48, Eugene But wrote: > nit: How about this?: > pushedItem->SetIsOverridingUserAgent(currentItem->IsOverridingUserAgent()); Done. https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:88: // out as pending, and will be lost unless |-commitPendingItem| is called. On 2017/02/16 02:20:48, Eugene But wrote: > Please document the difference between UserInitiated and RendererInitiated Done. https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:184: } On 2017/02/16 02:20:48, Eugene But wrote: > nit: add a linebreak Acknowledged. https://codereview.chromium.org/2698773002/diff/20001/ios/web/public/test/fak... File ios/web/public/test/fakes/test_navigation_manager.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/public/test/fak... ios/web/public/test/fakes/test_navigation_manager.mm:144: return; On 2017/02/16 02:20:48, Eugene But wrote: > Drop this Done. https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1572: if (_webStateImpl->GetNavigationManagerImpl().GetPendingItem()) { On 2017/02/16 02:20:48, Eugene But wrote: > nit: Do you want to create a local variable for NavigationManagerImpl? Acknowledged. https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1917: _webStateImpl->GetNavigationManagerImpl().AddRendererInitiatedPendingItem( On 2017/02/16 02:20:48, Eugene But wrote: > Having 2 APIs for adding pendingItem looks error prone. How about creating a > enum inside navigation_manager_impl.h?: > > class enum PendingItemType { > USER_INITIATED = 1, > RENDERER_INITIATED = 1, > }; I like this idea, but I changed the class name to ItemInitiationType, which I think is more informative. Please let me know if you don't like it.
Patchset #3 (id:40001) has been deleted
Thanks you for cleaning up web controller https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1917: _webStateImpl->GetNavigationManagerImpl().AddRendererInitiatedPendingItem( On 2017/02/16 22:04:29, liaoyuke wrote: > On 2017/02/16 02:20:48, Eugene But wrote: > > Having 2 APIs for adding pendingItem looks error prone. How about creating a > > enum inside navigation_manager_impl.h?: > > > > class enum PendingItemType { > > USER_INITIATED = 1, > > RENDERER_INITIATED = 1, > > }; > > I like this idea, but I changed the class name to ItemInitiationType, which I > think is more informative. Please let me know if you don't like it. How about NavigationInitiationType ? https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:29: // |USER_INITIATED| represents pending item that is initiated by actual user Please move comment for enum values and place them before enum values and write a comments to describe the enum itself (f.e. where it is used). // Navigation was initiated by actual user action. USER_INITIATED = 1, // Navigation was initiated by renderer. Examples of renderer-initiated navigations include: // * <a> link click // * changing window.location.href // * redirect via the <meta http-equiv="refresh"> tag // * using window.history.pushState RENDERER_INITIATED Please note that RENDERER_INITIATED is not the same as redirect and there is no need to initialize second item (it is only needed when enum is used in UMA or serialization and values should be stable). https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:94: // type, making it the current item. If pending item is the same as the s/current/pending https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:502: - (web::NavigationManagerImpl*)navigationManagerImpl; Could you please place this after |sessionController| property and make it a property? https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1921: [self navigationManagerImpl]->AddPendingItem( How about this?: Type navigationInitialtionType = params.is_renderer_initiated ? web::ItemInitiationType::RENDERER_INITIATED : web::ItemInitiationType::USER_INITIATED; [self navigationManagerImpl]->AddPendingItem(navUrl, params.referrer, transition,navigationInitialtionType); https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2280: [self nit: Do you want to create a local variable for index to improve the formatting? https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2291: CRWSessionController* sessionController = self.sessionController; nit: Do you need this local variable? https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:3659: return _webStateImpl ? [self navigationManagerImpl]->GetSessionController() return [self navigationManagerImpl] ? [self navigationManagerImpl]->GetSessionController() : nil; We should not assume that [self navigationManagerImpl] returns non-nil pointer if _webStateImpl is not null.
PTAL! Thank you very much! https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:29: // |USER_INITIATED| represents pending item that is initiated by actual user On 2017/02/17 00:00:23, Eugene But wrote: > Please move comment for enum values and place them before enum values and write > a comments to describe the enum itself (f.e. where it is used). > > // Navigation was initiated by actual user action. > USER_INITIATED = 1, > > // Navigation was initiated by renderer. Examples of renderer-initiated > navigations include: > // * <a> link click > // * changing window.location.href > // * redirect via the <meta http-equiv="refresh"> tag > // * using window.history.pushState > RENDERER_INITIATED > > Please note that RENDERER_INITIATED is not the same as redirect and there is no > need to initialize second item (it is only needed when enum is used in UMA or > serialization and values should be stable). Thank you for the detailed explanation! https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:94: // type, making it the current item. If pending item is the same as the On 2017/02/17 00:00:23, Eugene But wrote: > s/current/pending Done. https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:502: - (web::NavigationManagerImpl*)navigationManagerImpl; On 2017/02/17 00:00:24, Eugene But wrote: > Could you please place this after |sessionController| property and make it a > property? Acknowledged. https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1921: [self navigationManagerImpl]->AddPendingItem( On 2017/02/17 00:00:23, Eugene But wrote: > How about this?: > > Type navigationInitialtionType = params.is_renderer_initiated ? > web::ItemInitiationType::RENDERER_INITIATED : > web::ItemInitiationType::USER_INITIATED; > [self navigationManagerImpl]->AddPendingItem(navUrl, params.referrer, > transition,navigationInitialtionType); Done. https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2280: [self On 2017/02/17 00:00:24, Eugene But wrote: > nit: Do you want to create a local variable for index to improve the formatting? Done. https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:2291: CRWSessionController* sessionController = self.sessionController; On 2017/02/17 00:00:24, Eugene But wrote: > nit: Do you need this local variable? Done. https://codereview.chromium.org/2698773002/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:3659: return _webStateImpl ? [self navigationManagerImpl]->GetSessionController() On 2017/02/17 00:00:23, Eugene But wrote: > return [self navigationManagerImpl] ? [self > navigationManagerImpl]->GetSessionController() : nil; > > We should not assume that [self navigationManagerImpl] returns non-nil pointer > if _webStateImpl is not null. Done. And added a local variable to improve formatting.
https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1917: _webStateImpl->GetNavigationManagerImpl().AddRendererInitiatedPendingItem( On 2017/02/17 00:00:23, Eugene But wrote: > On 2017/02/16 22:04:29, liaoyuke wrote: > > On 2017/02/16 02:20:48, Eugene But wrote: > > > Having 2 APIs for adding pendingItem looks error prone. How about creating a > > > enum inside navigation_manager_impl.h?: > > > > > > class enum PendingItemType { > > > USER_INITIATED = 1, > > > RENDERER_INITIATED = 1, > > > }; > > > > I like this idea, but I changed the class name to ItemInitiationType, which I > > think is more informative. Please let me know if you don't like it. > How about NavigationInitiationType ? > > Please address this comment. https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:29: // |USER_INITIATED| represents pending item that is initiated by actual user On 2017/02/17 01:16:51, liaoyuke wrote: > On 2017/02/17 00:00:23, Eugene But wrote: > > Please move comment for enum values and place them before enum values and > write > > a comments to describe the enum itself (f.e. where it is used). > > > > // Navigation was initiated by actual user action. > > USER_INITIATED = 1, > > > > // Navigation was initiated by renderer. Examples of renderer-initiated > > navigations include: > > // * <a> link click > > // * changing window.location.href > > // * redirect via the <meta http-equiv="refresh"> tag > > // * using window.history.pushState > > RENDERER_INITIATED > > > > Please note that RENDERER_INITIATED is not the same as redirect and there is > no > > need to initialize second item (it is only needed when enum is used in UMA or > > serialization and values should be stable). > > Thank you for the detailed explanation! Per my previous comment, please add comments for enum itself and remove |= 2|
PTAL! https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:29: // |USER_INITIATED| represents pending item that is initiated by actual user On 2017/02/17 01:21:11, Eugene But wrote: > On 2017/02/17 01:16:51, liaoyuke wrote: > > On 2017/02/17 00:00:23, Eugene But wrote: > > > Please move comment for enum values and place them before enum values and > > write > > > a comments to describe the enum itself (f.e. where it is used). > > > > > > // Navigation was initiated by actual user action. > > > USER_INITIATED = 1, > > > > > > // Navigation was initiated by renderer. Examples of renderer-initiated > > > navigations include: > > > // * <a> link click > > > // * changing window.location.href > > > // * redirect via the <meta http-equiv="refresh"> tag > > > // * using window.history.pushState > > > RENDERER_INITIATED > > > > > > Please note that RENDERER_INITIATED is not the same as redirect and there is > > no > > > need to initialize second item (it is only needed when enum is used in UMA > or > > > serialization and values should be stable). > > > > Thank you for the detailed explanation! > Per my previous comment, please add comments for enum itself and remove |= 2| Done.
PTAL! https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_web_controller.mm:1917: _webStateImpl->GetNavigationManagerImpl().AddRendererInitiatedPendingItem( On 2017/02/17 01:21:11, Eugene But wrote: > On 2017/02/17 00:00:23, Eugene But wrote: > > On 2017/02/16 22:04:29, liaoyuke wrote: > > > On 2017/02/16 02:20:48, Eugene But wrote: > > > > Having 2 APIs for adding pendingItem looks error prone. How about creating > a > > > > enum inside navigation_manager_impl.h?: > > > > > > > > class enum PendingItemType { > > > > USER_INITIATED = 1, > > > > RENDERER_INITIATED = 1, > > > > }; > > > > > > I like this idea, but I changed the class name to ItemInitiationType, which > I > > > think is more informative. Please let me know if you don't like it. > > How about NavigationInitiationType ? > > > > > Please address this comment. Agreed, NavigationInitiationType is more informative than ItemInitiationType. https://codereview.chromium.org/2698773002/diff/120001/ios/chrome/browser/tab... File ios/chrome/browser/tabs/tab_unittest.mm (right): https://codereview.chromium.org/2698773002/diff/120001/ios/chrome/browser/tab... ios/chrome/browser/tabs/tab_unittest.mm:234: [tab_ webWillAddPendingURL:redirectUrl Simply added a few line breaks to make this piece of code more readable.
Thanks! lgtm
The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run
Thank you very much for reviewing! On Thu, Feb 16, 2017 at 6:04 PM <eugenebut@chromium.org> wrote: > Thanks! lgtm > > https://codereview.chromium.org/2698773002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm with a couple nits https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.h:30: enum class NavigationInitiationType { You can specify the amount of memory required to support an enum class by adding a primitive type at the end. I believe the default is an int, which is larger than needed here. How about we make USER_INITIATED = 0, and define the enum class as enum class NavigationInitiationType : BOOL { ....} https://codereview.chromium.org/2698773002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:502: - (web::NavigationManagerImpl*)navigationManagerImpl; Optional NIT: as long as we're defining a convenience getter like this, why don't we just make it a readonly property? That way we can use dot notation, which is slightly more compact.
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 #7 (id:140001) has been deleted
https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.h:30: enum class NavigationInitiationType { On 2017/02/17 02:26:37, kkhorimoto_ wrote: > You can specify the amount of memory required to support an enum class by adding > a primitive type at the end. I believe the default is an int, which is larger > than needed here. How about we make USER_INITIATED = 0, and define the enum > class as > > enum class NavigationInitiationType : BOOL { ....} Making USER_INITIATED 0 could lead to bugs. 0 is a default value in Objective-C and variable can be 0, because it was not initialized. USER_INITIATED is more privileged navigation, so I'd avoid initializing it with 0. Kurt, why do you think this should start with 0?
https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.h:30: enum class NavigationInitiationType { On 2017/02/17 16:09:46, Eugene But wrote: > On 2017/02/17 02:26:37, kkhorimoto_ wrote: > > You can specify the amount of memory required to support an enum class by > adding > > a primitive type at the end. I believe the default is an int, which is larger > > than needed here. How about we make USER_INITIATED = 0, and define the enum > > class as > > > > enum class NavigationInitiationType : BOOL { ....} > Making USER_INITIATED 0 could lead to bugs. 0 is a default value in Objective-C > and variable can be 0, because it was not initialized. USER_INITIATED is more > privileged navigation, so I'd avoid initializing it with 0. Kurt, why do you > think this should start with 0? I don't understand how this could lead to bugs, enum class is type safe, so there is no way you can compare NavigationInitiationType to any native types in Objective-C or C++. Correct me if I'm wrong. https://codereview.chromium.org/2698773002/diff/120001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_web_controller.mm (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_web_controller.mm:502: - (web::NavigationManagerImpl*)navigationManagerImpl; On 2017/02/17 02:26:37, kkhorimoto_ wrote: > Optional NIT: as long as we're defining a convenience getter like this, why > don't we just make it a readonly property? That way we can use dot notation, > which is slightly more compact. Good point! That does make things more compact.
https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.h:30: enum class NavigationInitiationType { On 2017/02/17 17:44:30, liaoyuke wrote: > On 2017/02/17 16:09:46, Eugene But wrote: > > On 2017/02/17 02:26:37, kkhorimoto_ wrote: > > > You can specify the amount of memory required to support an enum class by > > adding > > > a primitive type at the end. I believe the default is an int, which is > larger > > > than needed here. How about we make USER_INITIATED = 0, and define the enum > > > class as > > > > > > enum class NavigationInitiationType : BOOL { ....} > > Making USER_INITIATED 0 could lead to bugs. 0 is a default value in > Objective-C > > and variable can be 0, because it was not initialized. USER_INITIATED is more > > privileged navigation, so I'd avoid initializing it with 0. Kurt, why do you > > think this should start with 0? > > I don't understand how this could lead to bugs, enum class is type safe, so > there is no way you can compare NavigationInitiationType to any native types in > Objective-C or C++. Correct me if I'm wrong. The following code will lead to URL spoofing, because default value for NavigationInitiationType is 0: enum class NavigationInitiationType { USER_INITIATED = 0, RENDERER_INITIATED, } NavigationInitiationType type; AddPendingItem(url, referrer, PAGE_TRANSITION_LINK, type)
not lgtm. Can you please update NavigationItemImpl to contain the enum class rather than the is_renderer_initiated_ bool? Sorry I hadn't noticed this before! https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.h:30: enum class NavigationInitiationType { On 2017/02/17 17:44:30, liaoyuke wrote: > On 2017/02/17 16:09:46, Eugene But wrote: > > On 2017/02/17 02:26:37, kkhorimoto_ wrote: > > > You can specify the amount of memory required to support an enum class by > > adding > > > a primitive type at the end. I believe the default is an int, which is > larger > > > than needed here. How about we make USER_INITIATED = 0, and define the enum > > > class as > > > > > > enum class NavigationInitiationType : BOOL { ....} > > Making USER_INITIATED 0 could lead to bugs. 0 is a default value in > Objective-C > > and variable can be 0, because it was not initialized. USER_INITIATED is more > > privileged navigation, so I'd avoid initializing it with 0. Kurt, why do you > > think this should start with 0? > > I don't understand how this could lead to bugs, enum class is type safe, so > there is no way you can compare NavigationInitiationType to any native types in > Objective-C or C++. Correct me if I'm wrong. I suggested making it 0 so that it fits in the BOOL type when the enum class is evaluated and translated into its underlying representation. However, since BOOLs are actually just shorts, I guess that wasn't really a valid concern. I'm not sure about how ObjC objects would initialize an enum class with no zero value due to Yuke's point about enum class type safety. However, since the data is eventually owned by NavigationItemImpl, this shouldn't be a concern. In fact, we should just be storing the enum class directly in the NavigationItemImpl, which should alleviate these issues.
https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.h:30: enum class NavigationInitiationType { On 2017/02/17 18:37:40, kkhorimoto_ wrote: > On 2017/02/17 17:44:30, liaoyuke wrote: > > On 2017/02/17 16:09:46, Eugene But wrote: > > > On 2017/02/17 02:26:37, kkhorimoto_ wrote: > > > > You can specify the amount of memory required to support an enum class by > > > adding > > > > a primitive type at the end. I believe the default is an int, which is > > larger > > > > than needed here. How about we make USER_INITIATED = 0, and define the > enum > > > > class as > > > > > > > > enum class NavigationInitiationType : BOOL { ....} > > > Making USER_INITIATED 0 could lead to bugs. 0 is a default value in > > Objective-C > > > and variable can be 0, because it was not initialized. USER_INITIATED is > more > > > privileged navigation, so I'd avoid initializing it with 0. Kurt, why do you > > > think this should start with 0? > > > > I don't understand how this could lead to bugs, enum class is type safe, so > > there is no way you can compare NavigationInitiationType to any native types > in > > Objective-C or C++. Correct me if I'm wrong. > > I suggested making it 0 so that it fits in the BOOL type when the enum class is > evaluated and translated into its underlying representation. However, since > BOOLs are actually just shorts, I guess that wasn't really a valid concern. I'm > not sure about how ObjC objects would initialize an enum class with no zero > value due to Yuke's point about enum class type safety. However, since the data > is eventually owned by NavigationItemImpl, this shouldn't be a concern. In > fact, we should just be storing the enum class directly in the > NavigationItemImpl, which should alleviate these issues. I'm not sure "NavigationInitiationType type;" with no initial value would work for enum classes, as there's no implicit conversion between their numerical representations and the enum class value. I think it's okay to leave USER_INITIATED = 1, but we should be storing the enum class value directly in NavigationItemImpl rather than converting to a bool.
On 2017/02/17 18:41:34, kkhorimoto_ wrote: > https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... > File ios/web/navigation/navigation_manager_impl.h (right): > > https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... > ios/web/navigation/navigation_manager_impl.h:30: enum class > NavigationInitiationType { > On 2017/02/17 18:37:40, kkhorimoto_ wrote: > > On 2017/02/17 17:44:30, liaoyuke wrote: > > > On 2017/02/17 16:09:46, Eugene But wrote: > > > > On 2017/02/17 02:26:37, kkhorimoto_ wrote: > > > > > You can specify the amount of memory required to support an enum class > by > > > > adding > > > > > a primitive type at the end. I believe the default is an int, which is > > > larger > > > > > than needed here. How about we make USER_INITIATED = 0, and define the > > enum > > > > > class as > > > > > > > > > > enum class NavigationInitiationType : BOOL { ....} > > > > Making USER_INITIATED 0 could lead to bugs. 0 is a default value in > > > Objective-C > > > > and variable can be 0, because it was not initialized. USER_INITIATED is > > more > > > > privileged navigation, so I'd avoid initializing it with 0. Kurt, why do > you > > > > think this should start with 0? > > > > > > I don't understand how this could lead to bugs, enum class is type safe, so > > > there is no way you can compare NavigationInitiationType to any native types > > in > > > Objective-C or C++. Correct me if I'm wrong. > > > > I suggested making it 0 so that it fits in the BOOL type when the enum class > is > > evaluated and translated into its underlying representation. However, since > > BOOLs are actually just shorts, I guess that wasn't really a valid concern. > I'm > > not sure about how ObjC objects would initialize an enum class with no zero > > value due to Yuke's point about enum class type safety. However, since the > data > > is eventually owned by NavigationItemImpl, this shouldn't be a concern. In > > fact, we should just be storing the enum class directly in the > > NavigationItemImpl, which should alleviate these issues. > > I'm not sure "NavigationInitiationType type;" with no initial value would work > for enum classes, as there's no implicit conversion between their numerical > representations and the enum class value. I think it's okay to leave > USER_INITIATED = 1, but we should be storing the enum class value directly in > NavigationItemImpl rather than converting to a bool. If |NavigationInitiationType type;| does not compile for enum classes, then I don't see any issues with initializing USER_INITIATED to 0 :)
On 2017/02/17 18:50:14, Eugene But wrote: > On 2017/02/17 18:41:34, kkhorimoto_ wrote: > > > https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... > > File ios/web/navigation/navigation_manager_impl.h (right): > > > > > https://codereview.chromium.org/2698773002/diff/120001/ios/web/navigation/nav... > > ios/web/navigation/navigation_manager_impl.h:30: enum class > > NavigationInitiationType { > > On 2017/02/17 18:37:40, kkhorimoto_ wrote: > > > On 2017/02/17 17:44:30, liaoyuke wrote: > > > > On 2017/02/17 16:09:46, Eugene But wrote: > > > > > On 2017/02/17 02:26:37, kkhorimoto_ wrote: > > > > > > You can specify the amount of memory required to support an enum class > > by > > > > > adding > > > > > > a primitive type at the end. I believe the default is an int, which > is > > > > larger > > > > > > than needed here. How about we make USER_INITIATED = 0, and define > the > > > enum > > > > > > class as > > > > > > > > > > > > enum class NavigationInitiationType : BOOL { ....} > > > > > Making USER_INITIATED 0 could lead to bugs. 0 is a default value in > > > > Objective-C > > > > > and variable can be 0, because it was not initialized. USER_INITIATED is > > > more > > > > > privileged navigation, so I'd avoid initializing it with 0. Kurt, why do > > you > > > > > think this should start with 0? > > > > > > > > I don't understand how this could lead to bugs, enum class is type safe, > so > > > > there is no way you can compare NavigationInitiationType to any native > types > > > in > > > > Objective-C or C++. Correct me if I'm wrong. > > > > > > I suggested making it 0 so that it fits in the BOOL type when the enum class > > is > > > evaluated and translated into its underlying representation. However, since > > > BOOLs are actually just shorts, I guess that wasn't really a valid concern. > > I'm > > > not sure about how ObjC objects would initialize an enum class with no zero > > > value due to Yuke's point about enum class type safety. However, since the > > data > > > is eventually owned by NavigationItemImpl, this shouldn't be a concern. In > > > fact, we should just be storing the enum class directly in the > > > NavigationItemImpl, which should alleviate these issues. > > > > I'm not sure "NavigationInitiationType type;" with no initial value would work > > for enum classes, as there's no implicit conversion between their numerical > > representations and the enum class value. I think it's okay to leave > > USER_INITIATED = 1, but we should be storing the enum class value directly in > > NavigationItemImpl rather than converting to a bool. > If |NavigationInitiationType type;| does not compile for enum classes, then I > don't see any issues with initializing USER_INITIATED to 0 :) Makes sense! I'll just leave NavigationInitiationType unchanged. Hey Kurt, Could you please take another look at the NavigationItemImpl change? Thanks, Yuke
https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/crw... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/crw... ios/web/navigation/crw_session_controller.mm:117: rendererInitiated:(BOOL)rendererInitiated; Let's update this API to use web::NavigationInitiationType rather than the rendererInitiated BOOL, and just pass that directly into NavigationItemImpl::SetNavigationInitiationType() so we can avoid the uneccessary enum => bool => enum conversion. https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... File ios/web/navigation/navigation_item_impl.h (right): https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... ios/web/navigation/navigation_item_impl.h:100: void SetIsRendererInitiated(bool is_renderer_initiated); Instead of passing a bool here, let's just have a SetNavigationInitiationType() that takes a web::NavigationInitiationType enum https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... File ios/web/navigation/navigation_item_impl.mm (right): https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... ios/web/navigation/navigation_item_impl.mm:49: web::NavigationInitiationType::USER_INITIATED), This should be initialized to RENDERER_INITIATED for the reasons that Eugene mentioned before (i.e. USER_INITIATED has higher permissions, so we should default to RENDERER_INITIATED).
Hey Kurt, PTAL! Thank you very much! https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/crw... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/crw... ios/web/navigation/crw_session_controller.mm:117: rendererInitiated:(BOOL)rendererInitiated; On 2017/02/17 20:51:35, kkhorimoto_ wrote: > Let's update this API to use web::NavigationInitiationType rather than the > rendererInitiated BOOL, and just pass that directly into > NavigationItemImpl::SetNavigationInitiationType() so we can avoid the > uneccessary enum => bool => enum conversion. Done. https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... File ios/web/navigation/navigation_item_impl.h (right): https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... ios/web/navigation/navigation_item_impl.h:100: void SetIsRendererInitiated(bool is_renderer_initiated); On 2017/02/17 20:51:35, kkhorimoto_ wrote: > Instead of passing a bool here, let's just have a SetNavigationInitiationType() > that takes a web::NavigationInitiationType enum Done. https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... File ios/web/navigation/navigation_item_impl.mm (right): https://codereview.chromium.org/2698773002/diff/180001/ios/web/navigation/nav... ios/web/navigation/navigation_item_impl.mm:49: web::NavigationInitiationType::USER_INITIATED), On 2017/02/17 20:51:35, kkhorimoto_ wrote: > This should be initialized to RENDERER_INITIATED for the reasons that Eugene > mentioned before (i.e. USER_INITIATED has higher permissions, so we should > default to RENDERER_INITIATED). Per offline discussion, will do this in a separate CL.
lgtm, thanks!
Could you also update the CL description to describe the non-UA changes that are in this CL?
Description was changed from ========== [iOS] Refactoring web user agent code. This CL refactoring web agent code by moving them from CRWSessionController to NavigationManager, and updates corresponding references and unit tests to call methods directly from NavigationManager instead of CRWSessionController. BUG=678047 ========== to ========== [iOS] Refactoring web user agent code. This CL refactoring web agent code by moving them from CRWSessionController to NavigationManager, and updates corresponding references and unit tests to call methods directly from NavigationManager instead of CRWSessionController. This CL also introduces NavigationInitiationType, consisting of USER_INITIATED and RENDERER_INITIATED, to avoid passing boolean values as parameters to C++ functions, which is error prone and un-maintainable. BUG=678047 ==========
On 2017/02/17 22:19:22, kkhorimoto_ wrote: > Could you also update the CL description to describe the non-UA changes that are > in this CL? Done! Thank you so much for reviewing!
The CQ bit was checked by liaoyuke@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...
Description was changed from ========== [iOS] Refactoring web user agent code. This CL refactoring web agent code by moving them from CRWSessionController to NavigationManager, and updates corresponding references and unit tests to call methods directly from NavigationManager instead of CRWSessionController. This CL also introduces NavigationInitiationType, consisting of USER_INITIATED and RENDERER_INITIATED, to avoid passing boolean values as parameters to C++ functions, which is error prone and un-maintainable. BUG=678047 ========== to ========== [iOS] Refactoring CRWSessionController web user agent code. This CL refactoring web agent code by moving them from CRWSessionController to NavigationManager, and updates corresponding references and unit tests to call methods directly from NavigationManager instead of CRWSessionController. This CL also introduces NavigationInitiationType, consisting of USER_INITIATED and RENDERER_INITIATED, to avoid passing boolean values as parameters to C++ functions, which is error prone and un-maintainable. BUG=678047 ==========
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 #10 (id:220001) has been deleted
The CQ bit was checked by liaoyuke@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
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by liaoyuke@chromium.org to run a CQ dry run
The CQ bit was unchecked by liaoyuke@chromium.org
The CQ bit was checked by liaoyuke@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from eugenebut@chromium.org, kkhorimoto@chromium.org Link to the patchset: https://codereview.chromium.org/2698773002/#ps240001 (title: "Fix unit tests and rebase")
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: chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by liaoyuke@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 liaoyuke@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": 240001, "attempt_start_ts": 1487649564665270,
"parent_rev": "d133322623bc4dca3cfcf69e1e83d979cf2dfd9c", "commit_rev":
"6e5d49d75e64a08cbe73b4a39bb0a333417f3170"}
Message was sent while issue was closed.
Description was changed from ========== [iOS] Refactoring CRWSessionController web user agent code. This CL refactoring web agent code by moving them from CRWSessionController to NavigationManager, and updates corresponding references and unit tests to call methods directly from NavigationManager instead of CRWSessionController. This CL also introduces NavigationInitiationType, consisting of USER_INITIATED and RENDERER_INITIATED, to avoid passing boolean values as parameters to C++ functions, which is error prone and un-maintainable. BUG=678047 ========== to ========== [iOS] Refactoring CRWSessionController web user agent code. This CL refactoring web agent code by moving them from CRWSessionController to NavigationManager, and updates corresponding references and unit tests to call methods directly from NavigationManager instead of CRWSessionController. This CL also introduces NavigationInitiationType, consisting of USER_INITIATED and RENDERER_INITIATED, to avoid passing boolean values as parameters to C++ functions, which is error prone and un-maintainable. BUG=678047 Review-Url: https://codereview.chromium.org/2698773002 Cr-Commit-Position: refs/heads/master@{#451698} Committed: https://chromium.googlesource.com/chromium/src/+/6e5d49d75e64a08cbe73b4a39bb0... ==========
Message was sent while issue was closed.
Committed patchset #10 (id:240001) as https://chromium.googlesource.com/chromium/src/+/6e5d49d75e64a08cbe73b4a39bb0... |
