[iOS] Refactoring web CRWSessionController user agent code.

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 349 matching lines @@
   // 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:.
   CRWSessionEntry* currentEntry = self.currentEntry;
   if (currentEntry) {
     web::NavigationItem* item = [currentEntry navigationItem];
-    if (item->GetURL() == url &&
-        (!PageTransitionCoreTypeIs(trans, ui::PAGE_TRANSITION_FORM_SUBMIT) ||
-         PageTransitionCoreTypeIs(item->GetTransitionType(),
-                                  ui::PAGE_TRANSITION_FORM_SUBMIT))) {
+    BOOL isPendingItemEqualsCurrentItem = NO;

[kkhorimoto, 2017/02/15 23:02:05]

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;

[liaoyuke, 2017/02/16 01:49:04]

Done.

[liaoyuke, 2017/02/16 01:49:04]

Makes sense!

+
+    BOOL hasSameUrl = item->GetURL() == url;

[kkhorimoto, 2017/02/15 23:02:06]

s/hasSameUrl/hasSameURL

[liaoyuke, 2017/02/16 01:49:04]

Done.

+    BOOL isPendingItemPageTransitionFormSubmit =
+        PageTransitionCoreTypeIs(trans, ui::PAGE_TRANSITION_FORM_SUBMIT);
+    BOOL isCurrentItemPageTransitionFormSubmit = PageTransitionCoreTypeIs(

[kkhorimoto, 2017/02/15 23:02:05]

You can make these variable names a little less verbose:

isPendingTransitionFormSubmit
isCurrentTransitionFormSubmit

[liaoyuke, 2017/02/16 01:49:05]

Done.

+        item->GetTransitionType(), ui::PAGE_TRANSITION_FORM_SUBMIT);
+

[liaoyuke, 2017/02/15 22:26:51]

No structural changes. Just happened to find this piece of code extremely hard
to read, so did a little bit refactoring.

[liaoyuke, 2017/02/16 01:49:04]

Done.

+    if (hasSameUrl) {
+      if (!isPendingItemPageTransitionFormSubmit ||
+          isCurrentItemPageTransitionFormSubmit) {
+        isPendingItemPageTransitionFormSubmit = YES;
+      }
+    }
+
+    if (isPendingItemEqualsCurrentItem) {
       // 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;
     }
   }
 
-  BOOL useDesktopUserAgent =
-      _useDesktopUserAgentForNextPendingItem ||
-      (self.currentEntry.navigationItem &&
-       self.currentEntry.navigationItem->IsOverridingUserAgent());
-  _useDesktopUserAgentForNextPendingItem = NO;
   _pendingEntry.reset([self sessionEntryWithURL:url
                                        referrer:ref
                                      transition:trans
-                            useDesktopUserAgent:useDesktopUserAgent
+                            useDesktopUserAgent:NO

[kkhorimoto, 2017/02/15 23:02:05]

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.

[liaoyuke, 2017/02/16 01:49:04]

Done.

                               rendererInitiated:rendererInitiated]);
 
   if (_navigationManager && _navigationManager->GetFacadeDelegate()) {
     _navigationManager->GetFacadeDelegate()->OnNavigationItemPending();
   }
 }
 
 - (void)updatePendingItem:(const GURL&)url {
   // If there is no pending entry, navigation is probably happening within the
   // session history. Don't modify the entry list.
@@ skipping 268 matching lines @@
 
   NSInteger index = _currentNavigationIndex;
   // This will return the first session entry if all other entries are
   // redirects, regardless of the transition state of the first entry.
   while (index > 0 && [self isRedirectTransitionForItemAtIndex:index]) {
     --index;
   }
   return [_entries objectAtIndex:index];
 }
 
-- (void)useDesktopUserAgentForNextPendingItem {
-  if (_pendingEntry)
-    [_pendingEntry navigationItem]->SetIsOverridingUserAgent(true);
-  else
-    _useDesktopUserAgentForNextPendingItem = YES;
-}
-
 - (NSInteger)indexOfItem:(const web::NavigationItem*)item {
   web::NavigationItemList items = self.items;
   for (NSInteger i = 0; i < static_cast<NSInteger>(items.size()); ++i) {
     if (items[i] == item)
       return i;
   }
   return NSNotFound;
 }
 
 #pragma mark -
@@ skipping 46 matching lines @@
 }
 
 - (web::NavigationItemList)itemListForEntryList:(NSArray*)entries {
   web::NavigationItemList list(entries.count);
   for (size_t index = 0; index < entries.count; ++index)
     list[index] = [entries[index] navigationItem];
   return list;
 }
 
 @end