Create new pending item if UserAgentOverrideOption is not INHERIT.

ios/web/navigation/crw_session_controller.mm

 // Copyright 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #import "ios/web/navigation/crw_session_controller.h"
 
 #include <stddef.h>
 
 #include <algorithm>
 #include <utility>
@@ skipping 264 matching lines @@
   }
 }
 
 - (void)setBrowserState:(web::BrowserState*)browserState {
   _browserState = browserState;
   DCHECK(!_navigationManager ||
          _navigationManager->GetBrowserState() == _browserState);
 }
 
 - (void)addPendingItem:(const GURL&)url
-              referrer:(const web::Referrer&)ref
-            transition:(ui::PageTransition)trans
-        initiationType:(web::NavigationInitiationType)initiationType {
+                   referrer:(const web::Referrer&)ref
+                 transition:(ui::PageTransition)trans
+             initiationType:(web::NavigationInitiationType)initiationType
+    userAgentOverrideOption:(web::NavigationManager::UserAgentOverrideOption)
+                                userAgentOverrideOption {
   // Server side redirects are handled by updating existing pending item instead
   // of adding a new item.
   DCHECK((trans & ui::PAGE_TRANSITION_SERVER_REDIRECT) == 0);
 
   [self discardTransientItem];
   self.pendingItemIndex = -1;
 
   // Don't create a new item if it's already the same as the current item,
   // allowing this routine to be called multiple times in a row without issue.
   // Note: CRWSessionController currently has the responsibility to distinguish
   // between new navigations and history stack navigation, hence the inclusion
   // of specific transiton type logic here, in order to make it reliable with
   // real-world observed behavior.
   // TODO(crbug.com/676129): Fix the way changes are detected/reported elsewhere
   // in the web layer so that this hack can be removed.
   // Remove the workaround code from -presentSafeBrowsingWarningForResource:.
   web::NavigationItemImpl* currentItem = self.currentItem;
   if (currentItem) {
     BOOL hasSameURL = currentItem->GetURL() == url;
     BOOL isPendingTransitionFormSubmit =
         PageTransitionCoreTypeIs(trans, ui::PAGE_TRANSITION_FORM_SUBMIT);
     BOOL isCurrentTransitionFormSubmit = PageTransitionCoreTypeIs(
         currentItem->GetTransitionType(), ui::PAGE_TRANSITION_FORM_SUBMIT);
+

[kkhorimoto, 2017/04/06 22:36:19]

Remove this newline.

[liaoyuke, 2017/04/07 15:49:08]

Done.

+    DCHECK(userAgentOverrideOption !=

[Eugene But (OOO till 7-30), 2017/04/06 22:53:17]

Please explain in the comments the purpose of these DCHECKs

[liaoyuke, 2017/04/07 15:49:08]

Done.

+               web::NavigationManager::UserAgentOverrideOption::DESKTOP ||
+           currentItem->GetUserAgentType() != web::UserAgentType::DESKTOP);
+    DCHECK(userAgentOverrideOption !=
+               web::NavigationManager::UserAgentOverrideOption::MOBILE ||
+           currentItem->GetUserAgentType() != web::UserAgentType::MOBILE);

[kkhorimoto, 2017/04/06 22:36:19]

Let's move these DCHECKs to the top of this function under the server transition
DCHECK, and add comments explaining them.

[liaoyuke, 2017/04/07 15:49:08]

Acknowledged.

+    BOOL hasDifferentUserAgentType =

[kkhorimoto, 2017/04/06 22:36:19]

Can we instead have a BOOL |isInheritingUserAgentType| and check
|!isInheritingUserAgentType| below?  I think that would make the intention of
this code a little clearer.

[liaoyuke, 2017/04/07 15:49:08]

Done.

+        userAgentOverrideOption !=
+        web::NavigationManager::UserAgentOverrideOption::INHERIT;
+
     BOOL shouldCreatePendingItem =

[Eugene But (OOO till 7-30), 2017/04/06 22:53:17]

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;
}

[liaoyuke, 2017/04/07 15:49:08]

yes, that's better.

         !hasSameURL ||
-        (isPendingTransitionFormSubmit && !isCurrentTransitionFormSubmit);
+        (isPendingTransitionFormSubmit && !isCurrentTransitionFormSubmit) ||
+        hasDifferentUserAgentType;
 
     if (!shouldCreatePendingItem) {
       // Send the notification anyway, to preserve old behavior. It's unknown
       // whether anything currently relies on this, but since both this whole
       // hack and the content facade will both be going away, it's not worth
       // trying to unwind.
       if (_navigationManager && _navigationManager->GetFacadeDelegate())
         _navigationManager->GetFacadeDelegate()->OnNavigationItemPending();
       return;
     }
@@ skipping 305 matching lines @@
   return item;
 }
 
 - (BOOL)isRedirectTransitionForItemAtIndex:(size_t)index {
   DCHECK_LT(index, self.items.size());
   ui::PageTransition transition = self.items[index]->GetTransitionType();
   return (transition & ui::PAGE_TRANSITION_IS_REDIRECT_MASK) ? YES : NO;
 }
 
 @end