Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794723002:
Create new pending item if UserAgentOverrideOption is not INHERIT. (Closed)
|
Created:
3 years, 8 months ago by liaoyuke Modified:
3 years, 8 months ago CC:
chromium-reviews, Eugene But (OOO till 7-30), cbentzel+watch_chromium.org, ios-reviews+web_chromium.org, ios-reviews_chromium.org, net-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionCreate new pending item if UserAgentOverrideOption is not INHERIT.
The original logic in |AddPendingItem| does nothing if the URL of the
pending item is the same as the last committed item, except that the
pending item is added due to form submission.
However, this doesn't work with the new Request Desktop/Mobile Site
because the new functionality requires adding a pending item with the
same URL, but different UserAgentType.
This CL re-factors |AddPendingItem| so that a pending item with the
same URL can be added successfully as long as the
UserAgentOverrideOption is not INHEIRT.
This CL also adds corresponding unit tests to test the logic.
BUG=678047
Review-Url: https://codereview.chromium.org/2794723002
Cr-Commit-Position: refs/heads/master@{#462932}
Committed: https://chromium.googlesource.com/chromium/src/+/fbb84a320edb0492eb645530d3b20e9c5ab454ad
Patch Set 1 #Patch Set 2 : self review #
Total comments: 2
Patch Set 3 : Address comments #
Total comments: 23
Patch Set 4 : Address comments #
Total comments: 5
Patch Set 5 : Address comments #Patch Set 6 : Rebase #
Messages
Total messages: 26 (13 generated)
Description was changed from ========== should create new pending item if us override option is not INHERIT BUG= ========== to ========== Create new pending item if UserAgentOverrideOption is not INHERIT. The original logic in |AddPendingItem| does nothing if the URL of the pending item is the same as the last committed item, except that the pending item is added due to form submission. However, this doesn't work with the new Request Desktop/Mobile Site because the new functionality requires adding a pending item with the same URL, but different UserAgentType. This CL re-factors |AddPendingItem| so that a pending item with the same URL can be added successfully as long as the UserAgentOverrideOption is not INHEIRT. This CL also adds corresponding unit tests to test the logic. BUG=678047 ==========
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Hey Eugene, Kurt, PTAL. My original intention was to move the shouldCreatePendingItem logic from CRWSessionController to NavigationManagerImpl, but it's non-trivial refactoring work because |-addPendingItem| depends on a number of private methods in CRWSessionController, so instead, I chose to pass the UserAgentOverrideOption parameter from NavigationManagerImpl to CRWSessionController. Thank you very much!
Hey Kurt, Could you please also take a look at this CL? Thank you very much!
On 2017/04/05 22:37:52, liaoyuke wrote: > Hey Kurt, > > Could you please also take a look at this CL? > > Thank you very much! FYI: because of the reasons mentioned here: https://bugs.chromium.org/p/chromium/issues/detail?id=692303, it might be easier to just follow the original approach to call LoadURLWithParams instead of Reload with ORIGINAL_URL_REQUEST.
https://codereview.chromium.org/2794723002/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:318: web::NavigationManager::UserAgentOverrideOption::INHERIT) I think we should also check whether the override option differs from the current item's UA type so that we don't create new items that end up having the same UA type as the previous item. Also, can we define a separate bool after |isCurrentTransitionFormSumbit| that checks this logic, then include it in the || statment where we define |shouldCreatePendingItem|?
PTAL. Thank you very much! https://codereview.chromium.org/2794723002/diff/20001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/20001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:318: web::NavigationManager::UserAgentOverrideOption::INHERIT) On 2017/04/05 23:01:44, kkhorimoto_ wrote: > I think we should also check whether the override option differs from the > current item's UA type so that we don't create new items that end up having the > same UA type as the previous item. Also, can we define a separate bool after > |isCurrentTransitionFormSumbit| that checks this logic, then include it in the > || statment where we define |shouldCreatePendingItem|? Theoretically, override option should always be different from current item's UI type, so adding a DCHECK.
lgtm with a couple nits https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:313: Remove this newline. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:319: currentItem->GetUserAgentType() != web::UserAgentType::MOBILE); Let's move these DCHECKs to the top of this function under the server transition DCHECK, and add comments explaining them. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:320: BOOL hasDifferentUserAgentType = Can we instead have a BOOL |isInheritingUserAgentType| and check |!isInheritingUserAgentType| below? I think that would make the intention of this code a little clearer.
Thank you for awesome test coverage! https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:314: DCHECK(userAgentOverrideOption != Please explain in the comments the purpose of these DCHECKs https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:324: BOOL shouldCreatePendingItem = This logic was already complex and got even more complicated. Do you want to move it to a separate method like this?: - (BOOL)shouldCreatePendingItemWithURL:transition:userAgentOverrideOption: { BOOL hasSameURL = currentItem->GetURL() == url; if (hasSameURL) { // explanation why pending item should not be created for the same URL return NO; } .. other checks... return YES; } https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller_unittest.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller_unittest.mm:73: userAgentOverrideOption:web::NavigationManager::UserAgentOverrideOption:: Do you want to use "using web::NavigationManager::UserAgentOverrideOption" statement to improve the formatting? https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller_unittest.mm:1293: There was a change in session controller logic. Does NavigationManagerImpTest cover that logic? https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl_unittest.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:518: // Tests that when given a pending item, adding a new pending item should s/should replace/replaces ? https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:535: EXPECT_EQ(new_url, navigation_manager()->GetPendingItem()->GetURL()); Do you want to verify that ItemCount has changed? Same comment for all other tests. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:562: // Should replace if new transition is a form submission. This looks like a separate test from "form submission" case. Do you want to split them? https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:707: // Should not add if both the last committed item and the pending item are This looks like a separate test from "form submission" case. Do you want to split them?
Patchset #4 (id:60001) has been deleted
PTAL. Thank you very much! https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:313: On 2017/04/06 22:36:19, kkhorimoto_ wrote: > Remove this newline. Done. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:314: DCHECK(userAgentOverrideOption != On 2017/04/06 22:53:17, Eugene But wrote: > Please explain in the comments the purpose of these DCHECKs Done. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:319: currentItem->GetUserAgentType() != web::UserAgentType::MOBILE); On 2017/04/06 22:36:19, kkhorimoto_ wrote: > Let's move these DCHECKs to the top of this function under the server transition > DCHECK, and add comments explaining them. Acknowledged. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:320: BOOL hasDifferentUserAgentType = On 2017/04/06 22:36:19, kkhorimoto_ wrote: > Can we instead have a BOOL |isInheritingUserAgentType| and check > |!isInheritingUserAgentType| below? I think that would make the intention of > this code a little clearer. Done. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:324: BOOL shouldCreatePendingItem = On 2017/04/06 22:53:17, Eugene But wrote: > This logic was already complex and got even more complicated. Do you want to > move it to a separate method like this?: > > - (BOOL)shouldCreatePendingItemWithURL:transition:userAgentOverrideOption: { > BOOL hasSameURL = currentItem->GetURL() == url; > if (hasSameURL) { > // explanation why pending item should not be created for the same URL > return NO; > } > .. other checks... > return YES; > } yes, that's better. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller_unittest.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller_unittest.mm:73: userAgentOverrideOption:web::NavigationManager::UserAgentOverrideOption:: On 2017/04/06 22:53:17, Eugene But wrote: > Do you want to use "using web::NavigationManager::UserAgentOverrideOption" > statement to improve the formatting? Done. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller_unittest.mm:1293: On 2017/04/06 22:53:17, Eugene But wrote: > There was a change in session controller logic. Does NavigationManagerImpTest > cover that logic? Yes. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl_unittest.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:518: // Tests that when given a pending item, adding a new pending item should On 2017/04/06 22:53:17, Eugene But wrote: > s/should replace/replaces ? Done. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:535: EXPECT_EQ(new_url, navigation_manager()->GetPendingItem()->GetURL()); On 2017/04/06 22:53:17, Eugene But wrote: > Do you want to verify that ItemCount has changed? Same comment for all other > tests. Actually, ItemCount will not change because GetItemCount returns the number of committed items. Since this CL has nothing to do with committed items, would it still be helpful to EXPECT GetItemCount? https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:562: // Should replace if new transition is a form submission. On 2017/04/06 22:53:17, Eugene But wrote: > This looks like a separate test from "form submission" case. Do you want to > split them? Done. https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:707: // Should not add if both the last committed item and the pending item are On 2017/04/06 22:53:17, Eugene But wrote: > This looks like a separate test from "form submission" case. Do you want to > split them? Done.
lgtm! https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl_unittest.mm (right): https://codereview.chromium.org/2794723002/diff/40001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl_unittest.mm:535: EXPECT_EQ(new_url, navigation_manager()->GetPendingItem()->GetURL()); On 2017/04/07 15:49:08, liaoyuke wrote: > On 2017/04/06 22:53:17, Eugene But wrote: > > Do you want to verify that ItemCount has changed? Same comment for all other > > tests. > > Actually, ItemCount will not change because GetItemCount returns the number of > committed items. Since this CL has nothing to do with committed items, would it > still be helpful to EXPECT GetItemCount? Verifying that count does not change seems useful. https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:305: if (_navigationManager && _navigationManager->GetFacadeDelegate()) Kurt, do we still have "FacadeDelegate"? I suspect that we can throw this code away :) https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:320: - (BOOL)shouldCreatePendingItemWithURL:(const GURL&)url s/url/URL
https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:305: if (_navigationManager && _navigationManager->GetFacadeDelegate()) On 2017/04/07 16:16:40, Eugene But wrote: > Kurt, do we still have "FacadeDelegate"? I suspect that we can throw this code > away :) Nope, I've been meaning to get rid of this code for a while. I'll set aside some time to do it today because I'm kinda tired of having to maintain code that isn't actually used :)
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...
Thank you for reviewing! https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... File ios/web/navigation/crw_session_controller.mm (right): https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:305: if (_navigationManager && _navigationManager->GetFacadeDelegate()) On 2017/04/07 17:18:11, kkhorimoto_ wrote: > On 2017/04/07 16:16:40, Eugene But wrote: > > Kurt, do we still have "FacadeDelegate"? I suspect that we can throw this code > > away :) > > Nope, I've been meaning to get rid of this code for a while. I'll set aside > some time to do it today because I'm kinda tired of having to maintain code that > isn't actually used :) Sounds good, and btw, OnNavigationItemPruned doesn't seem to be useful, we had some code that removes navigation items but doesn't call OnNavigationItemPruned, and nothing bad happened. https://codereview.chromium.org/2794723002/diff/80001/ios/web/navigation/crw_... ios/web/navigation/crw_session_controller.mm:320: - (BOOL)shouldCreatePendingItemWithURL:(const GURL&)url On 2017/04/07 16:16:40, Eugene But wrote: > s/url/URL Done.
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
The patchset sent to the CQ was uploaded after l-g-t-m from kkhorimoto@chromium.org, eugenebut@chromium.org Link to the patchset: https://codereview.chromium.org/2794723002/#ps120001 (title: "Rebase")
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": 120001, "attempt_start_ts": 1491589132945880,
"parent_rev": "e63e28f00da6f1670b2a72d8511f70259b294e4c", "commit_rev":
"fbb84a320edb0492eb645530d3b20e9c5ab454ad"}
Message was sent while issue was closed.
Description was changed from ========== Create new pending item if UserAgentOverrideOption is not INHERIT. The original logic in |AddPendingItem| does nothing if the URL of the pending item is the same as the last committed item, except that the pending item is added due to form submission. However, this doesn't work with the new Request Desktop/Mobile Site because the new functionality requires adding a pending item with the same URL, but different UserAgentType. This CL re-factors |AddPendingItem| so that a pending item with the same URL can be added successfully as long as the UserAgentOverrideOption is not INHEIRT. This CL also adds corresponding unit tests to test the logic. BUG=678047 ========== to ========== Create new pending item if UserAgentOverrideOption is not INHERIT. The original logic in |AddPendingItem| does nothing if the URL of the pending item is the same as the last committed item, except that the pending item is added due to form submission. However, this doesn't work with the new Request Desktop/Mobile Site because the new functionality requires adding a pending item with the same URL, but different UserAgentType. This CL re-factors |AddPendingItem| so that a pending item with the same URL can be added successfully as long as the UserAgentOverrideOption is not INHEIRT. This CL also adds corresponding unit tests to test the logic. BUG=678047 Review-Url: https://codereview.chromium.org/2794723002 Cr-Commit-Position: refs/heads/master@{#462932} Committed: https://chromium.googlesource.com/chromium/src/+/fbb84a320edb0492eb645530d3b2... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/fbb84a320edb0492eb645530d3b2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
