[iOS] Refactoring web CRWSessionController user agent code.

ios/web/web_state/ui/crw_web_controller.mm

Index: ios/web/web_state/ui/crw_web_controller.mm
diff --git a/ios/web/web_state/ui/crw_web_controller.mm b/ios/web/web_state/ui/crw_web_controller.mm
index 371ef69279882da8eb20b4ecd1cc1899c08f154f..b0b1d17cba57416d79eb335c80589287c264e9bd 100644
--- a/ios/web/web_state/ui/crw_web_controller.mm
+++ b/ios/web/web_state/ui/crw_web_controller.mm
@@ -498,6 +498,9 @@ - (void)addPlaceholderOverlay;
 // Removes placeholder overlay.
 - (void)removePlaceholderOverlay;
 
+// Gets the associated NavigationManagerImpl.
+- (web::NavigationManagerImpl*)navigationManagerImpl;

[Eugene But (OOO till 7-30), 2017/02/17 00:00:24]

Could you please place this after |sessionController| property and make it a
property?

[liaoyuke, 2017/02/17 01:16:51]

Acknowledged.

+
 // Returns the current entry from the underlying session controller.
 // TODO(stuartmorgan): Audit all calls to these methods; these are just wrappers
 // around the same logic as GetActiveEntry, so should probably not be used for
@@ -1373,8 +1376,8 @@ - (void)setDocumentURL:(const GURL&)newURL {
 
 - (void)setNavigationItemTitle:(NSString*)title {
   DCHECK(title);
-  auto& navigationManager = _webStateImpl->GetNavigationManagerImpl();
-  web::NavigationItem* item = navigationManager.GetLastCommittedItem();
+  web::NavigationItem* item =
+      [self navigationManagerImpl]->GetLastCommittedItem();
   if (!item)
     return;
 
@@ -1386,7 +1389,7 @@ - (void)setNavigationItemTitle:(NSString*)title {
   // TODO(crbug.com/546218): See if this can be removed; it's not clear that
   // other platforms send this (tab sync triggers need to be compared against
   // upstream).
-  navigationManager.OnNavigationItemChanged();
+  [self navigationManagerImpl]->OnNavigationItemChanged();
 
   if ([_delegate respondsToSelector:@selector(webController:titleDidChange:)]) {
     [_delegate webController:self titleDidChange:title];
@@ -1569,16 +1572,15 @@ - (void)registerLoadRequest:(const GURL&)requestURL
 
   [_delegate webWillAddPendingURL:requestURL transition:transition];
   // Add or update pending url.
-  if (_webStateImpl->GetNavigationManagerImpl().GetPendingItem()) {
+  if ([self navigationManagerImpl]->GetPendingItem()) {
     // Update the existing pending entry.
     // Typically on PAGE_TRANSITION_CLIENT_REDIRECT.
     [[self sessionController] updatePendingItem:requestURL];
   } else {
     // A new session history entry needs to be created.
-    [[self sessionController] addPendingItem:requestURL
-                                    referrer:referrer
-                                  transition:transition
-                           rendererInitiated:YES];
+    [self navigationManagerImpl]->AddPendingItem(
+        requestURL, referrer, transition,
+        web::ItemInitiationType::RENDERER_INITIATED);
   }
   _webStateImpl->SetIsLoading(true);
   _webStateImpl->OnProvisionalNavigationStarted(requestURL);
@@ -1616,7 +1618,7 @@ - (void)updateHTML5HistoryState {
 
   // TODO(stuartmorgan): Make CRWSessionController manage this internally (or
   // remove it; it's not clear this matches other platforms' behavior).
-  _webStateImpl->GetNavigationManagerImpl().OnNavigationItemCommitted();
+  [self navigationManagerImpl]->OnNavigationItemCommitted();
   // Record that a same-document hashchange event will be fired.  This flag will
   // be reset when resonding to the hashchange message.  Note that resetting the
   // flag in the completion block below is too early, as that block is called
@@ -1912,14 +1914,19 @@ - (void)loadWithParams:(const NavigationManager::WebLoadParams&)originalParams {
     // forward/back transitions?
     [self recordStateInHistory];
 
-    CRWSessionController* history =
-        _webStateImpl->GetNavigationManagerImpl().GetSessionController();
     if (!self.currentSessionEntry)
       initialNavigation = YES;
-    [history addPendingItem:navUrl
-                   referrer:params.referrer
-                 transition:transition
-          rendererInitiated:params.is_renderer_initiated];
+
+    if (params.is_renderer_initiated) {
+      [self navigationManagerImpl]->AddPendingItem(

[Eugene But (OOO till 7-30), 2017/02/17 00:00:23]

How about this?:

Type navigationInitialtionType = params.is_renderer_initiated ?
    web::ItemInitiationType::RENDERER_INITIATED :
web::ItemInitiationType::USER_INITIATED;
[self navigationManagerImpl]->AddPendingItem(navUrl, params.referrer,
transition,navigationInitialtionType);

[liaoyuke, 2017/02/17 01:16:51]

Done.

+          navUrl, params.referrer, transition,
+          web::ItemInitiationType::RENDERER_INITIATED);
+    } else {
+      [self navigationManagerImpl]->AddPendingItem(
+          navUrl, params.referrer, transition,
+          web::ItemInitiationType::USER_INITIATED);
+    }
+
     web::NavigationItemImpl* addedItem =
         [self currentSessionEntry].navigationItemImpl;
     DCHECK(addedItem);
@@ -2058,8 +2065,7 @@ - (void)triggerPendingLoad {
 - (BOOL)shouldReload:(const GURL&)destinationURL
           transition:(ui::PageTransition)transition {
   // Do a reload if the user hits enter in the address bar or re-types a URL.
-  web::NavigationItem* item =
-      _webStateImpl->GetNavigationManagerImpl().GetVisibleItem();
+  web::NavigationItem* item = [self navigationManagerImpl]->GetVisibleItem();
   return (transition & ui::PAGE_TRANSITION_FROM_ADDRESS_BAR) && item &&
          (destinationURL == item->GetURL() ||
           destinationURL == item->GetOriginalRequestURL());
@@ -2076,7 +2082,7 @@ - (void)reloadInternal {
   base::RecordAction(UserMetricsAction("Reload"));
   if (_webView) {
     web::NavigationItem* transientItem =
-        _webStateImpl->GetNavigationManagerImpl().GetTransientItem();
+        [self navigationManagerImpl]->GetTransientItem();
     if (transientItem) {
       // If there's a transient item, a reload is considered a new navigation to
       // the transient item's URL (as on other platforms).
@@ -2270,10 +2276,9 @@ - (void)goDelta:(int)delta {
     return;
   }
 
-  web::NavigationManagerImpl& navigationManager =
-      _webStateImpl->GetNavigationManagerImpl();
-  if (navigationManager.CanGoToOffset(delta)) {
-    [self goToItemAtIndex:navigationManager.GetIndexForOffset(delta)];
+  if ([self navigationManagerImpl]->CanGoToOffset(delta)) {
+    [self

[Eugene But (OOO till 7-30), 2017/02/17 00:00:24]

nit: Do you want to create a local variable for index to improve the formatting?

[liaoyuke, 2017/02/17 01:16:51]

Done.

+        goToItemAtIndex:[self navigationManagerImpl]->GetIndexForOffset(delta)];
   }
 }
 
@@ -2283,10 +2288,10 @@ - (void)finishHistoryNavigationFromEntry:(CRWSessionEntry*)fromEntry {
   // Only load the new URL if it has a different document than |fromEntry| to
   // prevent extra page loads from NavigationItems created by hash changes or
   // calls to window.history.pushState().
-  BOOL shouldLoadURL =
-      ![_webStateImpl->GetNavigationManagerImpl().GetSessionController()
-          isSameDocumentNavigationBetweenItem:fromEntry.navigationItem
-                                      andItem:self.currentNavItem];
+  CRWSessionController* sessionController = self.sessionController;

[Eugene But (OOO till 7-30), 2017/02/17 00:00:24]

nit: Do you need this local variable?

[liaoyuke, 2017/02/17 01:16:51]

Done.

+  BOOL shouldLoadURL = ![sessionController
+      isSameDocumentNavigationBetweenItem:fromEntry.navigationItem
+                                  andItem:self.currentNavItem];
   web::NavigationItemImpl* currentItem =
       self.currentSessionEntry.navigationItemImpl;
   GURL endURL = [self URLForHistoryNavigationFromItem:fromEntry.navigationItem
@@ -2355,8 +2360,7 @@ - (BOOL)shouldClosePageOnNativeApplicationLoad {
   // opened from another application.
   BOOL rendererInitiatedWithoutInteraction =
       self.sessionController.openedByDOM && !_userInteractedWithWebController;
-  BOOL noNavigationItems =
-      !_webStateImpl->GetNavigationManagerImpl().GetItemCount();
+  BOOL noNavigationItems = ![self navigationManagerImpl]->GetItemCount();
   return rendererInitiatedWithoutInteraction || noNavigationItems;
 }
 
@@ -3000,13 +3004,12 @@ - (BOOL)handleWindowHistoryDidReplaceStateMessage:
   replaceURL = URLEscapedForHistory(replaceURL);
   if (!replaceURL.is_valid())
     return YES;
-  const NavigationManagerImpl& navigationManager =
-      _webStateImpl->GetNavigationManagerImpl();
+
   web::NavigationItem* navItem = [self currentNavItem];
   // ReplaceState happened before first navigation entry or called right
   // after window.open when the url is empty/not valid.
-  if (!navItem ||
-      (navigationManager.GetItemCount() <= 1 && navItem->GetURL().is_empty()))
+  if (!navItem || ([self navigationManagerImpl]->GetItemCount() <= 1 &&
+                   navItem->GetURL().is_empty()))
     return YES;
   if (!web::history_state_util::IsHistoryStateChangeValid(
           [self currentNavItem]->GetURL(), replaceURL)) {
@@ -3182,8 +3185,7 @@ - (BOOL)shouldAllowLoadWithNavigationAction:(WKNavigationAction*)action {
   // Check if the link navigation leads to a launch of an external app.
   // TODO(crbug.com/607780): Revise the logic of allowing external app launch
   // and move it to externalAppLauncher.
-  BOOL isOpenInNewTabNavigation =
-      !_webStateImpl->GetNavigationManager()->GetItemCount();
+  BOOL isOpenInNewTabNavigation = ![self navigationManagerImpl]->GetItemCount();
   BOOL isPossibleLinkClick = [self isLinkNavigation:action.navigationType];
   if (isPossibleLinkClick || isOpenInNewTabNavigation ||
       PageTransitionCoreTypeIs([self currentTransition],
@@ -3654,9 +3656,12 @@ - (void)setOverlayPreviewMode:(BOOL)overlayPreviewMode {
 #pragma mark Session Information
 
 - (CRWSessionController*)sessionController {
-  return _webStateImpl
-      ? _webStateImpl->GetNavigationManagerImpl().GetSessionController()
-      : nil;
+  return _webStateImpl ? [self navigationManagerImpl]->GetSessionController()

[Eugene But (OOO till 7-30), 2017/02/17 00:00:23]

return [self navigationManagerImpl] ? [self
navigationManagerImpl]->GetSessionController() : nil;

We should not assume that [self navigationManagerImpl] returns non-nil pointer
if _webStateImpl is not null.

[liaoyuke, 2017/02/17 01:16:51]

Done. And added a local variable to improve formatting.

+                       : nil;
+}
+
+- (NavigationManagerImpl*)navigationManagerImpl {
+  return _webStateImpl ? &(_webStateImpl->GetNavigationManagerImpl()) : nil;
 }
 
 - (CRWSessionEntry*)currentSessionEntry {