Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779263002:
Set user agent type of transient item the same as pending item. (Closed)
|
Created:
3 years, 8 months ago by liaoyuke Modified:
3 years, 8 months ago CC:
chromium-reviews, ios-reviews+chrome_chromium.org, ios-reviews_chromium.org, pkl (ping after 24h if needed), noyau+watch_chromium.org, marq+watch_chromium.org, sdefresne+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSet user agent type of transient item the same as pending item.
In current code, when deciding which user agent type to use for
pre-loading next item, the user agent of transient item is also taken
into consideration, and because the user agent type of transient item
is always MOBILE, this results in the following behavior:
Goto www.facebook.com -> Request Desktop Site ->
Goto https://expired.badssl.com -> Goto www.amazon.com.
Now, instead of visiting the desktop version of www.amazon.com, it's
visiting the mobile version.
This CL fixes the issue by accurately set the user agent type of
transient item as the user agent type of corresponding pending item.
BUG=706446
Review-Url: https://codereview.chromium.org/2779263002
Cr-Commit-Position: refs/heads/master@{#461856}
Committed: https://chromium.googlesource.com/chromium/src/+/0c1d4036c5f4aafb0530816aafce288cc56a691b
Patch Set 1 #Patch Set 2 : Addressed self review #Patch Set 3 : Addressed comments #
Total comments: 4
Patch Set 4 : removed unused includes #
Total comments: 10
Patch Set 5 : Address comments and add unit tests #
Total comments: 2
Patch Set 6 : Remove code duplication #
Total comments: 2
Patch Set 7 : Rebase #Patch Set 8 : Fix unit tests #
Total comments: 4
Patch Set 9 : Address comments #
Messages
Total messages: 50 (33 generated)
Description was changed from ========== Ignore transient items while accessing user agent type. There are two problems with the current code: 1. When at a transient page, "Request Desktop Site" in the tools menu is visible and enabled. 2. When deciding which user agent type to use for pre-loading next item, the user agent of transient item is also taken into consideration, and because the user agent type of transient item is always MOBILE, this results in the following behavior: Goto www.facebook.com -> Request Desktop Site -> Goto https://expired.badssl.com -> Goto www.amazon.com. Now, instead of visiting the desktop version of www.amazon.com, it's visiting the mobile version. This CL fixes the issue by ignoring transient item when deciding which user agent type to use for pre-loading next item, and make the user agent type of transient item NONE by default. BUG=706446 ========== to ========== Ignore transient items while accessing user agent type. There are two problems with the current code: 1. When at a transient page, "Request Desktop Site" in the tools menu is visible and enabled. 2. When deciding which user agent type to use for pre-loading next item, the user agent of transient item is also taken into consideration, and because the user agent type of transient item is always MOBILE, this results in the following behavior: Goto www.facebook.com -> Request Desktop Site -> Goto https://expired.badssl.com -> Goto www.amazon.com. Now, instead of visiting the desktop version of www.amazon.com, it's visiting the mobile version. This CL fixes the issue by ignoring transient item when deciding which user agent type to use for pre-loading next item, and make the user agent type of transient item NONE by default. BUG=706446 ==========
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...
liaoyuke@chromium.org changed reviewers: + eugenebut@chromium.org, kkhorimoto@chromium.org
Hey Eugene, Kurt, PTAL. Thank you very much!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I think a better solution would be to accurately represent the UA type of the transient item. Instead of your current approach, could we just reset the UA type to that of the last non-native NavigationItem when we create the transient item in CRWSessionController's |-addTransientItemWithURL:|?
Description was changed from ========== Ignore transient items while accessing user agent type. There are two problems with the current code: 1. When at a transient page, "Request Desktop Site" in the tools menu is visible and enabled. 2. When deciding which user agent type to use for pre-loading next item, the user agent of transient item is also taken into consideration, and because the user agent type of transient item is always MOBILE, this results in the following behavior: Goto www.facebook.com -> Request Desktop Site -> Goto https://expired.badssl.com -> Goto www.amazon.com. Now, instead of visiting the desktop version of www.amazon.com, it's visiting the mobile version. This CL fixes the issue by ignoring transient item when deciding which user agent type to use for pre-loading next item, and make the user agent type of transient item NONE by default. BUG=706446 ========== to ========== Set user agent type of transient item the same as pending item. In current code, when deciding which user agent type to use for pre-loading next item, the user agent of transient item is also taken into consideration, and because the user agent type of transient item is always MOBILE, this results in the following behavior: Goto www.facebook.com -> Request Desktop Site -> Goto https://expired.badssl.com -> Goto www.amazon.com. Now, instead of visiting the desktop version of www.amazon.com, it's visiting the mobile version. This CL fixes the issue by accurately set the user agent type of transient item as the user agent type of corresponding pending item. BUG=706446 ==========
PTAL. Thank you very much! https://codereview.chromium.org/2779263002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2779263002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:1481: When I changed from the current item to the visible item a few weeks ago, I didn't realize our |Reload| function always blindly reloads the current item. Now, for the sake of consistency, I think we should use current item everywhere related to user agent.
https://codereview.chromium.org/2779263002/diff/40001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2779263002/diff/40001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:1481: On 2017/03/29 22:41:05, liaoyuke wrote: > When I changed from the current item to the visible item a few weeks ago, I > didn't realize our |Reload| function always blindly reloads the current item. > Now, for the sake of consistency, I think we should use current item everywhere > related to user agent. As I mentioned in your other CL, it doesn't look like we're using NavigationManager::Reload() anymore. I'll review this CL again again once I have a better understanding of what your plan is WRT Reload() vs. LoadWithParams() with INHERIT. https://codereview.chromium.org/2779263002/diff/40001/ios/web/interstitials/w... File ios/web/interstitials/web_interstitial_impl.mm (right): https://codereview.chromium.org/2779263002/diff/40001/ios/web/interstitials/w... ios/web/interstitials/web_interstitial_impl.mm:9: #import "ios/web/navigation/crw_session_controller.h" I think we can remove this now.
PTAL. Thank you very much! https://codereview.chromium.org/2779263002/diff/40001/ios/web/interstitials/w... File ios/web/interstitials/web_interstitial_impl.mm (right): https://codereview.chromium.org/2779263002/diff/40001/ios/web/interstitials/w... ios/web/interstitials/web_interstitial_impl.mm:9: #import "ios/web/navigation/crw_session_controller.h" On 2017/03/30 02:36:46, kkhorimoto_ wrote: > I think we can remove this now. Done.
lgtm
https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:1482: web::NavigationItem* item = nullptr; How about this?: web::NavigationItem* item = self.navigationManager->GetTransientItem(); if (!item) item = self.navigationManager->GetPendingItem(); if (!item) item = self.navigationManager->GetLastCommittedItem(); https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1099: web::NavigationItem* item = nullptr; ditto https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1105: item = webState->GetNavigationManager()->GetLastCommittedItem(); Copying this logic everywhere is quite error prone. This is ok for a temporary solution, but then we need a plan for maintainable permanent implementation https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:100: void AddTransientItem(const GURL& url); Please add tests for this new method https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:179: DCHECK(GetPendingItem()->GetUserAgentType() != UserAgentType::NONE); DCHECK_NE
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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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...
Patchset #5 (id:80001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #5 (id:100001) has been deleted
PTAL. Thank you very much! https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/tabs... File ios/chrome/browser/tabs/tab.mm (right): https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/tabs... ios/chrome/browser/tabs/tab.mm:1482: web::NavigationItem* item = nullptr; On 2017/03/30 23:01:49, Eugene But wrote: > How about this?: > > web::NavigationItem* item = self.navigationManager->GetTransientItem(); > if (!item) > item = self.navigationManager->GetPendingItem(); > if (!item) > item = self.navigationManager->GetLastCommittedItem(); Done. https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/ui/b... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1099: web::NavigationItem* item = nullptr; On 2017/03/30 23:01:49, Eugene But wrote: > ditto Done. https://codereview.chromium.org/2779263002/diff/60001/ios/chrome/browser/ui/b... ios/chrome/browser/ui/browser_view_controller.mm:1105: item = webState->GetNavigationManager()->GetLastCommittedItem(); On 2017/03/30 23:01:49, Eugene But wrote: > Copying this logic everywhere is quite error prone. This is ok for a temporary > solution, but then we need a plan for maintainable permanent implementation Acknowledged. https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.h (right): https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.h:100: void AddTransientItem(const GURL& url); On 2017/03/30 23:01:49, Eugene But wrote: > Please add tests for this new method Thank you for catching this! https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2779263002/diff/60001/ios/web/navigation/navi... ios/web/navigation/navigation_manager_impl.mm:179: DCHECK(GetPendingItem()->GetUserAgentType() != UserAgentType::NONE); On 2017/03/30 23:01:49, Eugene But wrote: > DCHECK_NE Done.
https://codereview.chromium.org/2779263002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2779263002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1099: // TODO(crbug.com/707081): Clean this up once the bug is fixed. We should not land this code duplication, unless we have a short term plan to fix this. crbug.com/707081 is about finding a replacement for currentItem, but there will be no replacement. As it stands, crbug.com/707081 is WAI.
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
PTAL. Thank you very much! https://codereview.chromium.org/2779263002/diff/120001/ios/chrome/browser/ui/... File ios/chrome/browser/ui/browser_view_controller.mm (right): https://codereview.chromium.org/2779263002/diff/120001/ios/chrome/browser/ui/... ios/chrome/browser/ui/browser_view_controller.mm:1099: // TODO(crbug.com/707081): Clean this up once the bug is fixed. On 2017/03/31 18:06:40, Eugene But wrote: > We should not land this code duplication, unless we have a short term plan to > fix this. crbug.com/707081 is about finding a replacement for currentItem, but > there will be no replacement. As it stands, crbug.com/707081 is WAI. Makes sense! I'll just keep this piece of code as visibleItem for now, it makes a difference in some edge cases when there is a renderer-initiated pending item, but I think both behaviors are fine, I made the change only for the sake of consistency.
lgtm! https://codereview.chromium.org/2779263002/diff/180001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2779263002/diff/180001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.mm:178: DCHECK(GetPendingItem()); nit: No need to DCHECK, the next line will crash anyway.
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: 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 #8 (id:220001) has been deleted
Patchset #8 (id:240001) 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
Dry run: This issue passed the CQ dry run.
PTAL at the last patch. Thank you very much! https://codereview.chromium.org/2779263002/diff/180001/ios/web/navigation/nav... File ios/web/navigation/navigation_manager_impl.mm (right): https://codereview.chromium.org/2779263002/diff/180001/ios/web/navigation/nav... ios/web/navigation/navigation_manager_impl.mm:178: DCHECK(GetPendingItem()); On 2017/03/31 18:33:00, Eugene But wrote: > nit: No need to DCHECK, the next line will crash anyway. Done.
lgtm https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... File ios/web/public/test/web_test_with_web_state.h (right): https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... ios/web/public/test/web_test_with_web_state.h:54: void AddPendingItem(const GURL& url, ui::PageTransition transition); Could you please move this closer to LoadHtml. These seems more related that AddPendingItem() and web_state() https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... File ios/web/public/test/web_test_with_web_state.mm (right): https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... ios/web/public/test/web_test_with_web_state.mm:155: web_state->GetNavigationManagerImpl().AddPendingItem( GetWebController(web_state())->GetNavigationManagerImpl().AddPendingItem(
Thank you very much for reviewing! https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... File ios/web/public/test/web_test_with_web_state.h (right): https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... ios/web/public/test/web_test_with_web_state.h:54: void AddPendingItem(const GURL& url, ui::PageTransition transition); On 2017/04/04 18:22:31, Eugene But wrote: > Could you please move this closer to LoadHtml. These seems more related that > AddPendingItem() and web_state() Done. https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... File ios/web/public/test/web_test_with_web_state.mm (right): https://codereview.chromium.org/2779263002/diff/260001/ios/web/public/test/we... ios/web/public/test/web_test_with_web_state.mm:155: web_state->GetNavigationManagerImpl().AddPendingItem( On 2017/04/04 18:22:31, Eugene But wrote: > GetWebController(web_state())->GetNavigationManagerImpl().AddPendingItem( Done.
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/2779263002/#ps280001 (title: "Address comments")
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": 280001, "attempt_start_ts": 1491340552361250,
"parent_rev": "48c123ade773aed895db3a0a2141136b5ec4e2b4", "commit_rev":
"0c1d4036c5f4aafb0530816aafce288cc56a691b"}
Message was sent while issue was closed.
Description was changed from ========== Set user agent type of transient item the same as pending item. In current code, when deciding which user agent type to use for pre-loading next item, the user agent of transient item is also taken into consideration, and because the user agent type of transient item is always MOBILE, this results in the following behavior: Goto www.facebook.com -> Request Desktop Site -> Goto https://expired.badssl.com -> Goto www.amazon.com. Now, instead of visiting the desktop version of www.amazon.com, it's visiting the mobile version. This CL fixes the issue by accurately set the user agent type of transient item as the user agent type of corresponding pending item. BUG=706446 ========== to ========== Set user agent type of transient item the same as pending item. In current code, when deciding which user agent type to use for pre-loading next item, the user agent of transient item is also taken into consideration, and because the user agent type of transient item is always MOBILE, this results in the following behavior: Goto www.facebook.com -> Request Desktop Site -> Goto https://expired.badssl.com -> Goto www.amazon.com. Now, instead of visiting the desktop version of www.amazon.com, it's visiting the mobile version. This CL fixes the issue by accurately set the user agent type of transient item as the user agent type of corresponding pending item. BUG=706446 Review-Url: https://codereview.chromium.org/2779263002 Cr-Commit-Position: refs/heads/master@{#461856} Committed: https://chromium.googlesource.com/chromium/src/+/0c1d4036c5f4aafb0530816aafce... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:280001) as https://chromium.googlesource.com/chromium/src/+/0c1d4036c5f4aafb0530816aafce...
Message was sent while issue was closed.
A revert of this CL (patchset #9 id:280001) has been created in https://codereview.chromium.org/2798813002/ by liaoyuke@chromium.org. The reason for reverting is: Revert because this CL is breaking downstream tests.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
