Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(408)

Unified Diff: ios/web/navigation/navigation_manager_impl.mm

Issue 2766453002: Implement Reload for ORIGINAL_REQUEST_URL in NavigationManagerImpl. (Closed)
Patch Set: Addressed comments Created 3 years, 9 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
Index: ios/web/navigation/navigation_manager_impl.mm
diff --git a/ios/web/navigation/navigation_manager_impl.mm b/ios/web/navigation/navigation_manager_impl.mm
index a059c1d0e3c19f4421442ccc06de66ddcc44dd38..537aab0a0e9b7f87ff56819713cdb270321f932c 100644
--- a/ios/web/navigation/navigation_manager_impl.mm
+++ b/ios/web/navigation/navigation_manager_impl.mm
@@ -339,6 +339,22 @@ bool AreURLsInPageNavigation(const GURL& existing_url, const GURL& new_url) {
void NavigationManagerImpl::Reload(ReloadType reload_type,
bool check_for_reposts) {
+ // Reload with ORIGINAL_REQUEST_URL type should reload with the original
+ // request url of the current item because a server side redirect may change
Eugene But (OOO till 7-30) 2017/03/27 17:15:47 Could you please avoid referring to "current item"
liaoyuke 2017/03/28 00:16:40 Done.
+ // its url. For example, an user visits www.youtube.com and is then redirected
Eugene But (OOO till 7-30) 2017/03/27 17:15:47 nit: s/www.youtube.com/www.chromium.org and s/m.yo
liaoyuke 2017/03/28 00:16:40 Done.
+ // to m.youtube.com, when the user wants to refresh the page with a different
+ // configuration (e.g. user agent), the user would be expecting to visit
+ // www.youtube.com instead of m.youtube.com.
+ // NOTE: this logic has the following two assumptions:
+ // 1. |-reload| in web controller always reloads the current item.
+ // 2. Generally, a website would not issue a client side redirect
+ // (e.g. window.location) to redirect an user to its mobile version.
+ if (reload_type == web::ReloadType::ORIGINAL_REQUEST_URL) {
+ NavigationItem* current_item = [session_controller_ currentItem];
Eugene But (OOO till 7-30) 2017/03/27 17:15:47 The end goal is to remove |currentItem|, so it wil
liaoyuke 2017/03/28 00:16:40 Addressed per offline discussion.
+ DCHECK(current_item);
+ current_item->SetURL(current_item->GetOriginalRequestURL());
Eugene But (OOO till 7-30) 2017/03/27 17:15:47 Should we crash if NM is empty? Does this code cra
liaoyuke 2017/03/28 00:16:40 This function should do nothing if NM is empty. Ad
+ }
+
delegate_->Reload();
}
@@ -434,7 +450,7 @@ bool AreURLsInPageNavigation(const GURL& existing_url, const GURL& new_url) {
}
bool NavigationManagerImpl::IsRedirectItemAtIndex(int index) const {
- DCHECK_GT(index, 0);
+ DCHECK_GE(index, 0);
DCHECK_LT(index, GetItemCount());
ui::PageTransition transition = GetItemAtIndex(index)->GetTransitionType();
return transition & ui::PAGE_TRANSITION_IS_REDIRECT_MASK;

Powered by Google App Engine
This is Rietveld 408576698